Revert "Fix several optimizations after lightweight deletes" by alesapin · Pull Request #102125 · ClickHouse/ClickHouse · GitHub
Skip to content

Revert "Fix several optimizations after lightweight deletes"#102125

Merged
alexey-milovidov merged 1 commit into
masterfrom
revert-101212-fix-lwd-optimizations
Apr 8, 2026
Merged

Revert "Fix several optimizations after lightweight deletes"#102125
alexey-milovidov merged 1 commit into
masterfrom
revert-101212-fix-lwd-optimizations

Conversation

@alesapin

@alesapin alesapin commented Apr 8, 2026 •
edited by robot-clickhouse
Loading

Copy link
Copy Markdown
Member

Reverts #101212

Version info

  • Merged into: 26.4.1.688

@alesapin

alesapin commented Apr 8, 2026

Copy link
Copy Markdown
Member Author

@clickhouse-gh

clickhouse-gh Bot commented Apr 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [c8fbcb3]

Summary: ⏳

job_name test_name status info comment
Stateless tests (amd_msan, flaky check) failure
02835_drop_user_during_session FAIL cidb
02835_drop_user_during_session FAIL cidb
02835_drop_user_during_session FAIL cidb
02835_drop_user_during_session FAIL cidb
02835_drop_user_during_session FAIL cidb
Stateless tests (amd_asan_ubsan, distributed plan, parallel, 2/2) failure
02346_text_index_bug101913 FAIL cidb

AI Review

Summary

This PR reverts the previous lightweight-delete optimization fix and removes the corresponding stateless regression tests. I found two blocker-level issues: the revert reintroduces a correctness regression where optimization eligibility can remain permanently disabled after masked parts are merged away, and it removes existing coverage for that exact scenario. PR template metadata is also missing required changelog fields.

PR Metadata
  • ⚠️ Changelog category is missing from the PR body and cannot be validated against the actual change type.
  • ⚠️ Changelog entry is missing. For this change, a user-readable entry is required unless category is explicitly Not for changelog/CI Fix or Improvement/Documentation.
  • ⚠️ The PR body does not follow .github/PULL_REQUEST_TEMPLATE.md, so changelog quality cannot be assessed.

Exact replacement text suggestion:

  • Changelog category: Backward Incompatible Change
  • Changelog entry: Reverted the lightweight-delete optimization fix from #101212 due to regressions; this restores previous behavior where projection/trivial-count optimization availability may differ after lightweight delete workflows.
Findings

❌ Blockers

  • [src/Processors/QueryPlan/Optimizations/optimizeUseAggregateProjection.cpp:366] has_lightweight_delete_parts is used as a sticky boolean. It is set to true when any masked part appears, but normal runtime removal/replacement paths do not recompute it back to false. After OPTIMIZE ... FINAL removes all masked parts, projection eligibility can stay incorrectly disabled until restart/reload.
    • Suggested fix: restore exact active-part accounting (e.g. counter updated on all add/remove/swap/commit state transitions) or recompute from active parts under lock before using the optimization gate.
  • [tests/queries/0_stateless/04068_lwd_minmax_projection_recovery.sql:1] The PR deletes the targeted regression test (and replicated variant) that validates recovery after masked parts are merged away.
    • Suggested fix: keep these tests, or replace with equivalent/new stronger coverage proving the reverted behavior is intentionally safe.
Tests
  • ⚠️ The PR removes existing stateless tests 04068_lwd_minmax_projection_recovery* and does not add replacement coverage for the same behavior.
ClickHouse Rules
Item Status Notes
Deletion logging ➖
Serialization versioning ➖
Core-area scrutiny ✅
No test removal ❌ Existing regression tests were deleted without equivalent replacement.
Experimental gate ➖
No magic constants ✅
Backward compatibility ❌ Revert reintroduces sticky lightweight-delete state behavior affecting optimization availability after merges.
SettingsChangesHistory.cpp ➖
PR metadata quality ⚠️ PR template fields (Changelog category, Changelog entry) are missing.
Safe rollout ⚠️ Reverted behavior lacks runtime guard and loses regression coverage.
Compilation time ✅
No large/binary files ✅
Final Verdict
  • Status: ❌ Block
  • Minimum required actions:
    • Restore correct non-sticky accounting for lightweight-delete presence (or an equivalent correct gate) so optimization eligibility matches current active parts.
    • Restore deleted regression tests (or add equivalent coverage) for both non-replicated and replicated cases.
    • Fill PR template metadata with valid Changelog category and required Changelog entry.

@clickhouse-gh clickhouse-gh Bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Apr 8, 2026
clickhouse-gh[bot]
clickhouse-gh Bot reviewed Apr 8, 2026
View reviewed changes
Comment thread src/Processors/QueryPlan/Optimizations/optimizeUseAggregateProjection.cpp
&& metadata->minmax_count_projection
&& !reading.getMergeTreeData().hasLightweightDeletedMask();
bool can_use_minmax_projection = allow_implicit_projections && metadata->minmax_count_projection
&& !reading.getMergeTreeData().has_lightweight_delete_parts.load();

@clickhouse-gh clickhouse-gh Bot Apr 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reintroduces the sticky lightweight-delete flag regression.

has_lightweight_delete_parts is set to true when any part with lightweight delete appears, but it is not decremented/recomputed on normal part replacement paths. After OPTIMIZE ... FINAL merges away all masked parts, this check can still stay true, so minmax_count_projection remains disabled permanently until reload/restart.

Please restore an exact runtime accounting (e.g. counter of active masked parts updated on add/remove/swap/commit paths) instead of a sticky bool.

clickhouse-gh[bot]
clickhouse-gh Bot reviewed Apr 8, 2026
View reviewed changes
Comment thread tests/queries/0_stateless/04068_lwd_minmax_projection_recovery.sql
@@ -1,78 +0,0 @@
-- Verify that minmax_count_projection is re-enabled after all lightweight-deleted parts are merged away.

@clickhouse-gh clickhouse-gh Bot Apr 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This removes a focused regression test that verifies recovery after lightweight-delete parts are merged away.

Given this PR reverts behavior in has_lightweight_delete_parts, deleting this test eliminates the guard for the exact bug scenario (OPTIMIZE ... FINAL should re-enable projection/trivial-count optimizations once masked parts disappear).

Per ClickHouse review rules, existing tests should not be removed to mask a behavior regression. Please keep the test (or replace it with equivalent coverage).

@alexey-milovidov alexey-milovidov self-assigned this Apr 8, 2026
@alexey-milovidov alexey-milovidov merged commit 940e3af into master Apr 8, 2026
58 of 140 checks passed
@alexey-milovidov alexey-milovidov deleted the revert-101212-fix-lwd-optimizations branch April 8, 2026 21:46
@alexey-milovidov

alexey-milovidov commented Apr 8, 2026

Copy link
Copy Markdown
Member

@robot-ch-test-poll robot-ch-test-poll added the pr-synced-to-cloud The PR is synced to the cloud repo label Apr 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@clickhouse-gh clickhouse-gh[bot] clickhouse-gh[bot] left review comments

Assignees

@alexey-milovidov alexey-milovidov

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@alesapin @alexey-milovidov @robot-ch-test-poll