Do not disable the query condition cache for materialized lightweight deletes - #112947
Conversation
… deletes `appliesMutationsBeforePrewhere` treated a non-empty `mutation_steps` as proof that a mutation filtered rows ahead of PREWHERE. That list also holds the step applying an already materialized `_row_exists` mask, which is committed part data rather than a pending mutation, so a single lightweight-deleted row disabled the query condition cache for the whole table. Key the check off the source of the filter instead: patch parts and mutations taken from the mutations snapshot (including an unmaterialized lightweight delete) stay disqualifying, a materialized delete mask does not. This matches the read path, which bypasses the cache on `hasDataMutations()` / `hasPatchParts()` but not on `hasLightweightDeletedMask()`. `apply_deleted_mask = 0` is the one case where the mask does vary between queries, so such queries no longer read or write the cache.
|
Workflow [PR], commit [4c06f45] Summary: ❌
AI ReviewSummaryThis PR correctly narrows the original lightweight-delete regression and keeps Findings
Final Verdict
LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 25/25 (100.00%) · Uncovered code |
clickhouse-gh
Bot
added
the
pr-performance
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
…lete test The flaky check failed 10/10 runs with the reuse row reading every mark. Replaying the exact randomized settings of a failing run against the PR-built binary showed the prime query logging `Part all_1_1_0_2 pruned by statistics`: the randomized `auto_statistics_types` MergeTree setting (together with `materialize_statistics_on_insert`) builds column statistics that prune the whole part for the never-matching predicate, so nothing is read, nothing is written to the query condition cache, and the granule accounting becomes vacuous. Pinning `auto_statistics_types = ''` on the table removes the statistics; the full failing settings combination now passes. CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=112947&sha=45d5f5dce00ca07ccf1c321e8be3d9bc14484c7c&name_0=PR&name_1=Stateless%20tests%20%28amd_asan_ubsan%2C%20flaky%20check%29 PR: #112947
…ery does not read `appliesMutationsBeforePrewhere` keyed the write-side disqualification off `AlterConversions::hasMutations`, which is true for any pending on-fly mutation. But `AlterConversions::filterMutationCommands` drops `UPDATE` commands whose assignments touch none of the columns the query reads, so such a mutation contributes no read-chain step, rewrites nothing the query observes, and caching its marks is sound. Keying off `hasMutations` therefore suppressed cache writes more broadly than the read chain it models: with `apply_mutations_on_fly = 1` a pending `UPDATE` of an unrelated column disabled cache population for every predicate on the table. Track whether any steps produced from the mutations snapshot actually made it into `mutation_steps` (`has_on_fly_mutation_steps`) and disqualify on that instead. DELETE-typed commands are always kept by `filterMutationCommands`, so the poisoning direction of #107145 stays disqualifying, as does an unmaterialized lightweight delete via `hasLightweightDelete`. The new test section proves the corner: with a pending `UPDATE w` and a query reading only `v`, the `apply_mutations_on_fly = 1` prime must populate the cache and an `apply_mutations_on_fly = 0` reuse must consume it (the read path skips the cache while a data mutation is pending, so the `= 0` side is the one that can hit). The section fails before this commit (verified against the PR-built binary: no entry is written) and the read side was verified to hit once an entry exists. PR: #112947
|
Pushed two commits addressing the flaky check and the review finding. Flaky check root cause. Replaying the exact randomized settings of a failing run against the PR-built CI binary reproduced the miss deterministically, and delta-debugging the option list pointed at the table, not the session: the part directory of the failing table contained so the never-matching predicate reads nothing, writes nothing into the query condition cache, and the reuse row degrades to Review finding (over-broad The new test section covers exactly that corner: with a pending |
|
📊 Cloud Performance Report ✅ AI verdict: no significant changes detected. K_source=6 K_base=30 flagged=0/65 clickbench🟢 No significant changes tpch_adapted_1_official🟢 No significant changes Debug info
|
The query condition cache requires the analyzer on both the write side (`MergeTreeReaderSettings::createFromContext`) and the read side (`filterPartsByQueryConditionCache`), so in the old-analyzer CI configuration the cache never functions and both reuse assertions degrade to `0 0`. Reproduced locally with `--allow_experimental_analyzer 0` and fixed by an explicit `SET enable_analyzer = 1`, same as `03229_query_condition_cache_profile_events`. CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=112947&sha=bc1862263fc1e3bf6bb3fdcaebe8804f1d9dca5f&name_0=PR&name_1=Stateless%20tests%20%28amd_llvm_coverage%2C%20old%20analyzer%2C%20s3%20storage%2C%20DBReplicated%2C%20WasmEdge%2C%20sequential%2C%202%2F2%29 PR: #112947
| /// commands touch none of the columns this query reads is filtered out entirely (see | ||
| /// `AlterConversions::filterMutationCommands`), so it rewrites nothing this query observes and | ||
| /// must not disable the cache. `hasMutations()` would be too broad here. | ||
| if (info->has_on_fly_mutation_steps) |
There was a problem hiding this comment.
has_on_fly_mutation_steps is still broader than the invariant this helper is enforcing. AlterConversions::filterMutationCommands keeps any UPDATE whose assignment target is read anywhere in the query, but MutationsInterpreter lowers those UPDATEs to if(cond, new, old) expressions rather than row filters. So a pending UPDATE that only rewrites post-filter output columns can still make this return true even though it cannot be the reason a mark became non-matching before PREWHERE.
A concrete case is a pending UPDATE w = 0 WHERE id = 1 with SELECT w FROM t WHERE v = 123456789 SETTINGS apply_mutations_on_fly = 1: the zero-row result is still caused entirely by the predicate on v, so warming QCC here is safe and a later apply_mutations_on_fly = 0 query could reuse it. With the new boolean we still suppress that write, so the cache remains disabled for a real mixed-setting case. I think this gate needs to distinguish row-dropping / filter-affecting mutation steps from output-only rewrites instead of treating any surviving mutation step as disqualifying.
LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 26/26 (100.00%) · Uncovered code |
…te fix The comments were far longer than the code they explain and narrated the reasoning instead of stating the rule. Cut them roughly in half and keep only the facts a reader of these lines needs.
Assert that the repeated query reads no marks at all instead of merely fewer marks than the first run. Only the part holding the deleted row carries the mask, so if the insert ever lands in more than one part, the weaker assertion is satisfied by the other parts pruning and the test passes with the bug present. Pin the one behaviour this change gives up: an `apply_deleted_mask = 0` query neither writes nor consumes the cache, so a repeated one does not prune. A follow-up that keys entries by `apply_deleted_mask` rather than disabling them has to update that block deliberately. Add 04670_query_condition_cache_unique_key for the interaction with UNIQUE KEY tables. Building a part's dense index is the one internal reader that turns the mask off (`UniqueKeyDenseIndexOps::readUniqueKeyColumns` sets `apply_deleted_mask = 0`), and mutation-class commands - `DELETE FROM` included - are rejected on such tables, so no UNIQUE KEY part can carry a materialized mask. The test pins both halves of that, so the day either one changes the cache write path gets revisited.
…elete reference The apply_deleted_mask = 0 block added in the previous commit prints two counts before the ProfileEvents rows; the reference listed only the rows. https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=112947&sha=810129a2d8561d78c9b8098df49e579226873dde&name_0=PR&name_1=Fast%20test
| /// Not `alter_conversions->hasMutations()`: a pending mutation that touches no column this | ||
| /// query reads produces no step and rewrites nothing the query observes. | ||
| if (info->has_on_fly_mutation_steps) | ||
| return true; |
There was a problem hiding this comment.
getPatchesForColumns is already query-specific, so !info->patch_parts.empty() is still too broad for the same reason the old hasMutations() check was. A lightweight update patch on an output-only column makes patch_parts non-empty whenever that column is selected, even though it cannot make any mark stop matching the query predicate.
Concrete case: ALTER TABLE t UPDATE w = 0 WHERE id = 1 SETTINGS alter_update_mode = 'lightweight_force', enable_lightweight_update = 1, then SELECT w FROM t WHERE v = 123456789 SETTINGS apply_patch_parts = 1. The patch is kept because w is read, but row eligibility for v = 123456789 is unchanged, so an apply_patch_parts = 1 prime should still be able to warm QCC for a later apply_patch_parts = 0 query. This unconditional return keeps the cache disabled for that mixed-setting case.
I think this needs the same refinement as the on-fly-mutation path: distinguish row-affecting patches (_row_exists or predicate columns) from output-only rewrites, and add a focused regression test.
The test asserted that an ordinary query on a UNIQUE KEY table still prunes on a repeat. It does not, and that is deliberate: `ReadFromMergeTree` turns the query condition cache off for UNIQUE KEY reads on both the write and the consult side, because the cache is CSN-oblivious while the delete bitmap is not, so a mark recorded as non-matching after a bitmap drop could be skipped by a reader pinned at an older snapshot whose rows are live. Assert that instead. Together with the `DELETE FROM` rejection it is the full reason such tables cannot reach the materialized-mask handling, and re-enabling the cache for UNIQUE KEY reads now has to come past this test. Also add the two count rows the reference was missing. https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=112947&sha=c65d4acc47b740369199c3be82a8656495c9efd0&name_0=PR
robot-ch-test-poll3
added
the
pr-must-backport-synced
robot-clickhouse-ci-1
added
the
pr-synced-to-cloud
Backport #112947 to 26.6: Do not disable the query condition cache for materialized lightweight deletes
Backport #112947 to 26.7: Do not disable the query condition cache for materialized lightweight deletes
4 participants
Related: #107145
Related: #113239
Related: #83259
Related: #104985
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fixed a performance regression where a single lightweight
DELETEdisabled the query condition cache for the whole table. Repeated selective queries over a table that had ever been touched by a lightweight delete stopped pruning granules and fell back to reading every mark.Description
The regression
Deleting one row stops the query condition cache from pruning anything, for the entire table, permanently.
ProfileEvents['SelectedMarks']per run:Same behaviour with
mutations_sync = 2, so this is not a pending-mutation window: it persists after the delete is fully materialized.This was found on a production table (24M rows, 5 parts) where exactly one part carried
has_lightweight_delete = 1. That part has 4318 marks; the four small parts have 6 between them. The affected version pruned the four small parts and read all 4318 marks of the big one on every repetition, turning a 0.03 s query into 0.8-2.0 s. AnINSERT ... SELECT *copy of the same table into a fresh table did not reproduce, which is what pointed at_row_existsrather than the schema or the data.Bisected to #107145.
Root cause
MergeTreeReadPoolBaseappends two different kinds of thing to the samemutation_stepslist:and
appliesMutationsBeforePrewhere()was!info->mutation_steps.empty() || !info->patch_parts.empty().The reasoning in #107145 is correct for anything whose effect varies between queries: a mark emptied by an on-fly mutation must not be attributed to the predicate, or a later
apply_mutations_on_fly = 0query reusing that entry loses rows. A materialized_row_existsmask is not that. It is committed part data, every query reading the part observes exactly the same rows, and a mark it empties is attributable to the predicate like any other.The read path already draws this line correctly - it bypasses on
hasDataMutations()andhasPatchParts(), but not onhasLightweightDeletedMask(). So the write side was not actually made symmetric with the read side; it was made stricter.The fix
Part 1 - key the check off the source of the filter, not the resulting step list (
MergeTreeReadTask.cpp).Patch parts and mutations coming from the mutations snapshot stay disqualifying.
alter_conversions->hasLightweightDelete()keeps an unmaterialized lightweight delete disqualifying, because that one really is applied from the snapshot at read time and varies withapply_mutations_on_fly. A materialized mask no longer disables the cache. This is a narrowing of an over-broad condition; every case #107145 set out to cover is still covered.Part 2 - keep
apply_deleted_mask = 0out of the cache (MergeTreeIOSettings.cpp,MergeTreeDataSelectExecutor.cpp).Part 1 leaves exactly one way for the mask to vary between queries. Only one direction is unsound:
apply_deleted_mask = 1writes,= 0reads: unsound. The writer saw fewer rows and may record "no match" for a granule whose only matching rows are deleted; the reader must return those rows.= 0writes,= 1reads: sound. The writer saw a superset, so its verdict is conservative.apply_deleted_maskis a debugging aid, so such queries simply do not read or write the cache. That is cheaper and easier to reason about than splitting the key space, and it costs nothing on the default path.Tests
New
04669_query_condition_cache_lightweight_delete:a materialized lightweight delete still prunes on a repeat query (fails before this change: the second run reads every mark)
an
apply_deleted_mask = 0read does not consume an entry written by a normal read, and the reverse order is also correctresults stay correct with the cache warm
a repeated
apply_deleted_mask = 0query does not prune, which is the one behaviour Part 2 gives up. Pinned deliberately: keying entries byapply_deleted_maskinstead of excluding them (Key query condition cache entries by apply_deleted_mask #113239) has to update that block.The effectiveness assertions demand that the repeat reads no marks rather than fewer marks than the first run. Only the part holding the deleted row carries the mask, so with a weaker assertion a multi-part table would satisfy it through the other parts pruning and the test would pass with the bug present.
New
04670_query_condition_cache_unique_keycoversUNIQUE KEYtables, which is where the one internal reader that turns the mask off lives (building a part's dense index reads itsUNIQUE KEYcolumns withapply_deleted_mask = 0, inUniqueKeyDenseIndexOps::readUniqueKeyColumns). Two independent reasons keep such tables away from anything this PR changes, and the test pins both:UNIQUE KEYread never uses the cache.ReadFromMergeTreedisables it for both the write and the consult side, because the cache is CSN-oblivious while the delete bitmap is not; there is aTODO(unique-key)to revisit it with a snapshot-aware cache.UNIQUE KEYpart can carry a materialized mask, because mutation-class commands are rejected on such tables,DELETE FROMincluded.Re-enabling the cache for
UNIQUE KEYreads now has to come past this test, which is the point: that work has to look at the mask and bitmap interaction rather than only flipping the flag.The unmaterialized/on-fly direction is already covered by
03229_query_condition_cache_on_fly_mutations, added by #107145, which must keep passing.Backports
#107145 was backported to
26.5.6.46,26.4.5.134and26.3.17.50, so every one of those lines is affected and needs this fix. Note26.3is LTS and is affected from26.3.17.50onward.Version info
26.7.3.13,26.6.3.2