From c71fd213badb55277ca639e9ed0aa699650d5451 Mon Sep 17 00:00:00 2001 From: Coreground Technologies Automation Account Date: Sat, 12 Sep 2026 13:42:41 -0500 Subject: [PATCH 1/5] Fix QueryConditionCache OOB write on part-name/layout reuse (Altinity/ClickHouse#2342) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The real v25.8.28.10001.altinitystable tag did NOT have the Key fix upstream (qcc-package's investigation snapshots were captured from a later revision that already had it — see docs/superpowers/plans/ 2026-09-12-qcc-2342-findings.md). This commit adds it: Key now includes marks_count/has_final_mark so a layout mismatch on getOrSet() is structurally impossible, updates read()'s signature and its call site(s) accordingly, and layers the defense-in-depth patch on top: the three size-mismatch checks in QueryConditionCache::write()/read() become LOG_ERROR + skip instead of throw, so a residual mismatch degrades to a cache miss instead of failing the user's query, and a new QueryConditionCacheLayoutMismatch ProfileEvent surfaces if that check ever fires. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_013NAYye5ozisLN7WBqzYfAn --- src/Common/ProfileEvents.cpp | 4 + .../Cache/QueryConditionCache.cpp | 114 +++++++++++++++++- src/Interpreters/Cache/QueryConditionCache.h | 8 +- .../MergeTree/MergeTreeDataSelectExecutor.cpp | 7 +- 4 files changed, 126 insertions(+), 7 deletions(-) diff --git a/src/Common/ProfileEvents.cpp b/src/Common/ProfileEvents.cpp index ddf2efa2b823..1ff769925c64 100644 --- a/src/Common/ProfileEvents.cpp +++ b/src/Common/ProfileEvents.cpp @@ -96,6 +96,10 @@ M(VectorSimilarityIndexCacheWeightLost, "Approximate number of bytes evicted from the vector index cache.", ValueType::Number) \ M(QueryConditionCacheHits, "Number of times an entry has been found in the query condition cache (and reading of marks can be skipped). Only updated for SELECT queries with SETTING use_query_condition_cache = 1.", ValueType::Number) \ M(QueryConditionCacheMisses, "Number of times an entry has not been found in the query condition cache (and reading of mark cannot be skipped). Only updated for SELECT queries with SETTING use_query_condition_cache = 1.", ValueType::Number) \ + M(QueryConditionCacheLayoutMismatch, "Number of times a query condition cache entry was " \ + "skipped because its mark layout did not match the current part's layout. Should be 0 after " \ + "the Key fix; a non-zero count indicates a residual hazard (see Altinity/ClickHouse#2342).", \ + ValueType::Number) \ M(QueryCacheHits, "Number of times a query result has been found in the query cache (and query computation was avoided). Only updated for SELECT queries with SETTING use_query_cache = 1.", ValueType::Number) \ M(QueryCacheMisses, "Number of times a query result has not been found in the query cache (and required query computation). Only updated for SELECT queries with SETTING use_query_cache = 1.", ValueType::Number) \ M(CreatedReadBufferOrdinary, "Number of times ordinary read buffer was created for reading data (while choosing among other read methods).", ValueType::Number) \ diff --git a/src/Interpreters/Cache/QueryConditionCache.cpp b/src/Interpreters/Cache/QueryConditionCache.cpp index f7d3df3c8816..3bfca3bf1011 100644 --- a/src/Interpreters/Cache/QueryConditionCache.cpp +++ b/src/Interpreters/Cache/QueryConditionCache.cpp @@ -1,4 +1,5 @@ #include +#include #include #include #include @@ -9,6 +10,11 @@ namespace ProfileEvents { extern const Event QueryConditionCacheHits; extern const Event QueryConditionCacheMisses; + /// TODO(#2342): register this event in src/Common/ProfileEvents.cpp: + /// M(QueryConditionCacheLayoutMismatch, "Number of times a query condition cache entry was skipped " + /// "because its mark layout did not match the current part's layout. A non-zero count here is direct " + /// "evidence of the use-after-free / OOB-write hazard described in Altinity/ClickHouse#2342.", ValueType::Number) \ + extern const Event QueryConditionCacheLayoutMismatch; } namespace CurrentMetrics @@ -17,6 +23,11 @@ namespace CurrentMetrics extern const Metric QueryConditionCacheEntries; } +namespace ErrorCodes +{ + extern const int LOGICAL_ERROR; +} + namespace DB { @@ -24,7 +35,9 @@ bool QueryConditionCache::Key::operator==(const Key & other) const { return table_id == other.table_id && part_name == other.part_name - && condition_hash == other.condition_hash; + && condition_hash == other.condition_hash + && marks_count == other.marks_count + && has_final_mark == other.has_final_mark; } size_t QueryConditionCache::KeyHasher::operator()(const Key & key) const @@ -33,6 +46,8 @@ size_t QueryConditionCache::KeyHasher::operator()(const Key & key) const hash.update(key.table_id); hash.update(key.part_name); hash.update(key.condition_hash); + hash.update(key.marks_count); + hash.update(key.has_final_mark); return hash.get64(); } @@ -52,7 +67,34 @@ void QueryConditionCache::write( const UUID & table_id, const String & part_name, UInt64 condition_hash, const String & condition, const MarkRanges & mark_ranges, size_t marks_count, bool has_final_mark) { - Key key = {table_id, part_name, condition_hash, condition}; + if (has_final_mark && marks_count == 0) + throw Exception(ErrorCodes::LOGICAL_ERROR, "A part with a final mark must have at least one mark"); + + /// This validates ranges against the *caller-supplied* marks_count, which is a genuine caller + /// bug if it fails -- this throw is unchanged from upstream and should stay a throw. + for (const auto & mark_range : mark_ranges) + { + if (mark_range.begin > mark_range.end || mark_range.end > marks_count) + throw Exception( + ErrorCodes::LOGICAL_ERROR, + "Invalid mark range [{}, {}) for query condition cache entry with {} marks", + mark_range.begin, + mark_range.end, + marks_count); + } + + /// TODO(#2342): marks_count and has_final_mark are now part of Key (see QueryConditionCache.h). + /// Before this change, Key was {table_id, part_name, condition_hash} only. A part_name can be + /// reused after its underlying data (and therefore its mark layout) has changed -- e.g. after a + /// mutation, a merge that produces a part with a name collision under certain replay scenarios, + /// or (per the linked issue) some other part-lifecycle path we have not fully identified. + /// getOrSet() below could then return a pre-existing Entry sized for the OLD layout, which the + /// code proceeded to index using the NEW marks_count/mark_ranges. On std::vector's + /// bit-packed storage this is an out-of-bounds *write*, not just a read -- see the (*) fill() + /// and matching_marks[marks_count - 1] writes below. Keying on the layout as well eliminates the + /// possibility of this mismatch entirely; the alternative (validate-before-use, kept as a defense + /// in depth below) only stops the write after the mismatched Entry has already been returned. + Key key = {table_id, part_name, condition_hash, marks_count, has_final_mark, condition}; auto load_func = [&](){ return std::make_shared(marks_count); }; auto [entry, inserted] = cache.getOrSet(key, load_func); @@ -61,6 +103,30 @@ void QueryConditionCache::write( { std::shared_lock shared_lock(entry->mutex); /// cheap + /// TODO(#2342): defense in depth. With marks_count in the Key, this branch should now be + /// unreachable in normal operation -- cache.getOrSet() cannot return an Entry keyed to a + /// different marks_count than the one just constructed. If this ever fires, it means either + /// (a) a hash collision between two distinct Keys (extremely unlikely with SipHash64 over + /// table_id + part_name + condition_hash + marks_count + has_final_mark), or + /// (b) a residual bug in Key equality/hashing, or + /// (c) the underlying corruption in #2342 has already occurred elsewhere and this Entry's + /// bookkeeping is not trustworthy regardless of what the key comparison says. + /// Silently returning (skip the cache write) is the only safe response here -- do NOT throw + /// from inside the query pipeline for what may be a caching-layer inconsistency, and do NOT + /// proceed to index a vector we now know is a mismatched size. This is intentionally the + /// last line of defense, not the fix: the fix is the Key change above. + if (entry->matching_marks.size() != marks_count) + { + ProfileEvents::increment(ProfileEvents::QueryConditionCacheLayoutMismatch); + LOG_ERROR( + logger, + "Query condition cache layout mismatch on write for table_id: {}, part_name: {}, " + "condition_hash: {}: entry has {} marks, current task has {} marks (has_final_mark: {}). " + "Skipping cache update for this entry. See Altinity/ClickHouse#2342.", + table_id, part_name, condition_hash, entry->matching_marks.size(), marks_count, has_final_mark); + return; + } + bool need_not_update_marks = true; for (const auto & mark_range : mark_ranges) { @@ -82,7 +148,22 @@ void QueryConditionCache::write( { std::lock_guard lock(entry->mutex); /// (*) - chassert(marks_count == entry->matching_marks.size()); + /// TODO(#2342): same defense-in-depth check as above, now under the exclusive lock. Kept + /// deliberately duplicated rather than factored into a helper: the shared_lock check above + /// is an optimization (avoid taking the exclusive lock), and the entry could theoretically + /// change between releasing the shared lock and acquiring the exclusive one in some future + /// refactor. Re-checking here costs nothing and removes that assumption. + if (entry->matching_marks.size() != marks_count) + { + ProfileEvents::increment(ProfileEvents::QueryConditionCacheLayoutMismatch); + LOG_ERROR( + logger, + "Query condition cache layout mismatch on write (exclusive path) for table_id: {}, " + "part_name: {}, condition_hash: {}: entry has {} marks, current task has {} marks " + "(has_final_mark: {}). Skipping cache update for this entry. See Altinity/ClickHouse#2342.", + table_id, part_name, condition_hash, entry->matching_marks.size(), marks_count, has_final_mark); + return; + } /// The input mark ranges are the areas which the scan can skip later on. for (const auto & mark_range : mark_ranges) @@ -104,9 +185,10 @@ void QueryConditionCache::write( has_final_mark); } -std::optional QueryConditionCache::read(const UUID & table_id, const String & part_name, UInt64 condition_hash) +std::optional QueryConditionCache::read( + const UUID & table_id, const String & part_name, UInt64 condition_hash, size_t marks_count, bool has_final_mark) { - Key key = {table_id, part_name, condition_hash, ""}; + Key key = {table_id, part_name, condition_hash, marks_count, has_final_mark, ""}; if (auto entry = cache.get(key)) { @@ -114,6 +196,28 @@ std::optional QueryConditionCache::read(cons std::shared_lock lock(entry->mutex); + /// TODO(#2342): defense in depth, mirrors the write-path checks above. On the read path a + /// mismatch is comparatively low-risk -- the caller only reads matching_marks, it does not + /// index into it with the *caller's* marks_count anywhere we've audited (see + /// MergeTreeDataSelectExecutor::filterPartsByQueryConditionCache, which indexes with + /// mark_range.begin/end drawn from part_with_ranges.ranges, i.e. the part's own state at scan + /// time, not from this returned vector's size). We still guard here because we have NOT + /// confirmed every caller across all supported versions respects that invariant, and because + /// returning a mismatched-size MatchingMarks to a caller that isn't expecting one is exactly + /// the kind of latent hazard this whole issue is about. Treat as a cache miss on mismatch. + if (entry->matching_marks.size() != marks_count) + { + ProfileEvents::increment(ProfileEvents::QueryConditionCacheLayoutMismatch); + LOG_ERROR( + logger, + "Query condition cache layout mismatch on read for table_id: {}, part_name: {}, " + "condition_hash: {}: entry has {} marks, requested {} marks (has_final_mark: {}). " + "Treating as a cache miss. See Altinity/ClickHouse#2342.", + table_id, part_name, condition_hash, entry->matching_marks.size(), marks_count, has_final_mark); + ProfileEvents::increment(ProfileEvents::QueryConditionCacheMisses); + return {}; + } + LOG_TEST( logger, "Read entry for table_uuid: {}, part: {}, condition_hash: {}", diff --git a/src/Interpreters/Cache/QueryConditionCache.h b/src/Interpreters/Cache/QueryConditionCache.h index 729ad667fbf5..b593ee875637 100644 --- a/src/Interpreters/Cache/QueryConditionCache.h +++ b/src/Interpreters/Cache/QueryConditionCache.h @@ -34,6 +34,11 @@ class QueryConditionCache const String part_name; const UInt64 condition_hash; + /// A part name can be reused after its data has been replaced. The mark layout is + /// part of the cached value's shape, so it must be part of the cache identity too. + const size_t marks_count; + const bool has_final_mark; + /// -- Additional members, conceptually not part of the key. Only included for pretty-printing /// in system.query_condition_cache: const String condition; @@ -80,7 +85,8 @@ class QueryConditionCache const MarkRanges & mark_ranges, size_t marks_count, bool has_final_mark); /// Check the cache if it contains an entry for the given table + part id and predicate hash. - std::optional read(const UUID & table_id, const String & part_name, UInt64 condition_hash); + std::optional read( + const UUID & table_id, const String & part_name, UInt64 condition_hash, size_t marks_count, bool has_final_mark); /// For debugging and system tables std::vector dump() const; diff --git a/src/Storages/MergeTree/MergeTreeDataSelectExecutor.cpp b/src/Storages/MergeTree/MergeTreeDataSelectExecutor.cpp index c6e8b7bac16e..1721dda4e725 100644 --- a/src/Storages/MergeTree/MergeTreeDataSelectExecutor.cpp +++ b/src/Storages/MergeTree/MergeTreeDataSelectExecutor.cpp @@ -1092,7 +1092,12 @@ void MergeTreeDataSelectExecutor::filterPartsByQueryConditionCache( const auto & data_part = part_with_ranges.data_part; auto storage_id = data_part->storage.getStorageID(); - auto matching_marks_opt = query_condition_cache->read(storage_id.uuid, data_part->name, condition_hash); + auto matching_marks_opt = query_condition_cache->read( + storage_id.uuid, + data_part->name, + condition_hash, + data_part->index_granularity->getMarksCount(), + data_part->index_granularity->hasFinalMark()); if (!matching_marks_opt) { ++it; From 696a1e9d81033d206b20b2e77cf4715d6b039322 Mon Sep 17 00:00:00 2001 From: Coreground Technologies Automation Account Date: Sat, 12 Sep 2026 13:56:58 -0500 Subject: [PATCH 2/5] Add gtest coverage for QueryConditionCache layout-mismatch defense-in-depth (Altinity/ClickHouse#2342) Adds two gtest files under src/Interpreters/Cache/tests/ (new directory, picked up automatically by the existing GLOB_RECURSE "gtest*.cpp" rule in src/CMakeLists.txt, no CMake changes needed): - gtest_query_condition_cache.cpp: baseline regression tests for the Key layout-mismatch fix (2 tests). - gtest_query_condition_cache_defense_in_depth.cpp: additional coverage for reused part names and concurrent writes (3 tests). Added , , , and includes proactively since the literals/types used (1_MiB, UUID(1), std::thread, std::vector) are not guaranteed to be visible transitively. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_013NAYye5ozisLN7WBqzYfAn --- .../tests/gtest_query_condition_cache.cpp | 42 +++++++ ...query_condition_cache_defense_in_depth.cpp | 116 ++++++++++++++++++ 2 files changed, 158 insertions(+) create mode 100755 src/Interpreters/Cache/tests/gtest_query_condition_cache.cpp create mode 100755 src/Interpreters/Cache/tests/gtest_query_condition_cache_defense_in_depth.cpp diff --git a/src/Interpreters/Cache/tests/gtest_query_condition_cache.cpp b/src/Interpreters/Cache/tests/gtest_query_condition_cache.cpp new file mode 100755 index 000000000000..000c95f38a49 --- /dev/null +++ b/src/Interpreters/Cache/tests/gtest_query_condition_cache.cpp @@ -0,0 +1,42 @@ +#include +#include +#include +#include + +using namespace DB; + +TEST(QueryConditionCache, KeepsEntriesWithDifferentMarkLayoutsSeparate) +{ + QueryConditionCache cache("LRU", 1_MiB, 0.5); + const UUID table_id(1); + const String part_name = "part"; + constexpr UInt64 condition_hash = 42; + + MarkRanges one_mark; + one_mark.emplace_back(0, 1); + cache.write(table_id, part_name, condition_hash, "", one_mark, 1, false); + + MarkRanges two_marks; + two_marks.emplace_back(1, 2); + cache.write(table_id, part_name, condition_hash, "", two_marks, 2, false); + + auto first_layout = cache.read(table_id, part_name, condition_hash, 1, false); + ASSERT_TRUE(first_layout); + ASSERT_EQ(first_layout->size(), 1); + EXPECT_FALSE((*first_layout)[0]); + + auto second_layout = cache.read(table_id, part_name, condition_hash, 2, false); + ASSERT_TRUE(second_layout); + ASSERT_EQ(second_layout->size(), 2); + EXPECT_TRUE((*second_layout)[0]); + EXPECT_FALSE((*second_layout)[1]); +} + +TEST(QueryConditionCache, RejectsRangesOutsideEntryMarkLayout) +{ + QueryConditionCache cache("LRU", 1_MiB, 0.5); + MarkRanges invalid_range; + invalid_range.emplace_back(0, 2); + + EXPECT_THROW(cache.write(UUID(1), "part", 42, "", invalid_range, 1, false), Exception); +} diff --git a/src/Interpreters/Cache/tests/gtest_query_condition_cache_defense_in_depth.cpp b/src/Interpreters/Cache/tests/gtest_query_condition_cache_defense_in_depth.cpp new file mode 100755 index 000000000000..1fb9473c7842 --- /dev/null +++ b/src/Interpreters/Cache/tests/gtest_query_condition_cache_defense_in_depth.cpp @@ -0,0 +1,116 @@ +/// TODO(#2342): These tests exercise the defense-in-depth paths added in QueryConditionCache.patched.cpp. +/// They cannot currently trigger a *genuine* Key-hash collision (SipHash64 over table_id + part_name + +/// condition_hash + marks_count + has_final_mark is not practically collidable in a unit test), so +/// "mismatch" here is necessarily synthetic. These tests document and lock in the *behavior* (log + +/// skip, no throw, no OOB access) rather than proving the mismatch is unreachable in production -- +/// that claim rests on the Key change itself, not on these tests. +/// +/// NOT YET DONE (see package README, "Not completed" section): compiled, linked, or run. Written +/// against the same gtest/API shape as the provided gtest_query_condition_cache.cpp. + +#include +#include +#include +#include + +#include +#include + +using namespace DB; + +/// Baseline from the provided test file, reproduced here so this file is runnable standalone. +/// This is the primary regression test for the actual fix: two different mark layouts under the +/// same table_id/part_name/condition_hash must be kept as distinct cache entries. +TEST(QueryConditionCache, KeepsEntriesWithDifferentMarkLayoutsSeparate_DuplicateFromProvidedFile) +{ + QueryConditionCache cache("LRU", 1_MiB, 0.5); + const UUID table_id(1); + const String part_name = "part"; + constexpr UInt64 condition_hash = 42; + + MarkRanges one_mark; + one_mark.emplace_back(0, 1); + cache.write(table_id, part_name, condition_hash, "", one_mark, 1, false); + + MarkRanges two_marks; + two_marks.emplace_back(1, 2); + cache.write(table_id, part_name, condition_hash, "", two_marks, 2, false); + + auto first_layout = cache.read(table_id, part_name, condition_hash, 1, false); + ASSERT_TRUE(first_layout); + ASSERT_EQ(first_layout->size(), 1); + EXPECT_FALSE((*first_layout)[0]); + + auto second_layout = cache.read(table_id, part_name, condition_hash, 2, false); + ASSERT_TRUE(second_layout); + ASSERT_EQ(second_layout->size(), 2); + EXPECT_TRUE((*second_layout)[0]); + EXPECT_FALSE((*second_layout)[1]); +} + +/// A part_name reused with a completely different mark count (simulating the part-lifecycle +/// scenario this issue hypothesizes -- e.g. a part name recycled after data changed underneath it) +/// must not throw and must not corrupt either entry. With the Key fix this is definitionally two +/// separate keys, so this mostly re-confirms the same property as the test above from a different +/// angle (larger layout difference, has_final_mark also varied). +TEST(QueryConditionCache, ReusedPartNameWithFinalMarkDoesNotThrowOrCorrupt) +{ + QueryConditionCache cache("LRU", 1_MiB, 0.5); + const UUID table_id(1); + const String part_name = "reused_part"; + constexpr UInt64 condition_hash = 7; + + MarkRanges small_ranges; + small_ranges.emplace_back(0, 3); + EXPECT_NO_THROW(cache.write(table_id, part_name, condition_hash, "", small_ranges, 3, false)); + + /// Simulates the same part_name later associated with a much larger, differently-shaped part + /// (e.g. after a merge or mutation), including a final mark this time. + MarkRanges large_ranges; + large_ranges.emplace_back(0, 50); + large_ranges.emplace_back(60, 99); + EXPECT_NO_THROW(cache.write(table_id, part_name, condition_hash, "", large_ranges, 100, true)); + + auto small_layout = cache.read(table_id, part_name, condition_hash, 3, false); + ASSERT_TRUE(small_layout); + ASSERT_EQ(small_layout->size(), 3); + + auto large_layout = cache.read(table_id, part_name, condition_hash, 100, true); + ASSERT_TRUE(large_layout); + ASSERT_EQ(large_layout->size(), 100); + /// has_final_mark=true means marks_count-1 (index 99) must be forced to false (i.e. "must scan"). + EXPECT_FALSE((*large_layout)[99]); +} + +/// Concurrent-scan case referenced in the Entry class comment (*): multiple threads writing +/// overlapping/adjacent ranges for the same key must not race or corrupt the entry. This does not +/// specifically target #2342, but is adjacent: the ticket raises prefer_localhost_replica's effect +/// on crash rate as unexplained by the layout-mismatch fix alone, and increased local-read +/// concurrency against this cache is one candidate mechanism worth having coverage for. +TEST(QueryConditionCache, ConcurrentWritesToSameKeyDoNotCorruptEntry) +{ + QueryConditionCache cache("LRU", 1_MiB, 0.5); + const UUID table_id(2); + const String part_name = "concurrent_part"; + constexpr UInt64 condition_hash = 99; + constexpr size_t marks_count = 1000; + + std::vector threads; + for (size_t t = 0; t < 8; ++t) + { + threads.emplace_back([&, t]() + { + MarkRanges ranges; + ranges.emplace_back(t * 100, (t + 1) * 100); + cache.write(table_id, part_name, condition_hash, "", ranges, marks_count, false); + }); + } + for (auto & th : threads) + th.join(); + + auto result = cache.read(table_id, part_name, condition_hash, marks_count, false); + ASSERT_TRUE(result); + ASSERT_EQ(result->size(), marks_count); + for (size_t i = 0; i < 800; ++i) + EXPECT_FALSE((*result)[i]) << "mark " << i << " should have been marked non-matching"; +} From 5c98e7362558ec1bfdd39ee542af07c1468c82fc Mon Sep 17 00:00:00 2001 From: Coreground Technologies Automation Account Date: Sat, 12 Sep 2026 14:45:08 -0500 Subject: [PATCH 3/5] Fix backslash-newline line splice swallowing extern declaration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The stale TODO comment above `extern const Event QueryConditionCacheLayoutMismatch;` (copied verbatim from qcc-package's patched .cpp per Task 2's Step 2) had a trailing " \" at the end of its last `///` line. A trailing backslash performs line splicing at the preprocessing/translation-phase level even inside line comments, before tokenization — so that spliced the next physical line (the extern declaration itself) into the same comment, silently deleting it from compilation and breaking the build wherever this event is referenced. Found by Task 5 while building the checkout. Removes the whole 5-line stale TODO block (it was dead text anyway: the ProfileEvent it describes was already registered in src/Common/ProfileEvents.cpp in the original Task 2 commit), restoring the extern declaration as live code. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_013NAYye5ozisLN7WBqzYfAn --- src/Interpreters/Cache/QueryConditionCache.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/Interpreters/Cache/QueryConditionCache.cpp b/src/Interpreters/Cache/QueryConditionCache.cpp index 3bfca3bf1011..7e676041b3e5 100644 --- a/src/Interpreters/Cache/QueryConditionCache.cpp +++ b/src/Interpreters/Cache/QueryConditionCache.cpp @@ -10,10 +10,6 @@ namespace ProfileEvents { extern const Event QueryConditionCacheHits; extern const Event QueryConditionCacheMisses; - /// TODO(#2342): register this event in src/Common/ProfileEvents.cpp: - /// M(QueryConditionCacheLayoutMismatch, "Number of times a query condition cache entry was skipped " - /// "because its mark layout did not match the current part's layout. A non-zero count here is direct " - /// "evidence of the use-after-free / OOB-write hazard described in Altinity/ClickHouse#2342.", ValueType::Number) \ extern const Event QueryConditionCacheLayoutMismatch; } From 15846cb1ea812bf217832e759ffcc430c259b24b Mon Sep 17 00:00:00 2001 From: Coreground Technologies Automation Account Date: Sat, 12 Sep 2026 16:37:01 -0500 Subject: [PATCH 4/5] Fix final review findings 1-11 for QueryConditionCache layout fix Addresses all findings from the final whole-branch code review of the QueryConditionCache marks_count/has_final_mark key fix (Altinity/ClickHouse#2342): 1. Move the file-scope `namespace ErrorCodes` block inside `namespace DB`, matching the convention used everywhere else in the codebase. 2. Convert the two throws added by this branch in write() (final-mark/zero-marks check, and mark-range bounds validation) to the same log-and-skip pattern used by the other defense-in-depth checks in this file, and delete the inaccurate "unchanged from upstream" comment that justified the throw. 3. Add back `chassert(entry->matching_marks.size() == marks_count)` immediately above each of the three log-and-skip mismatch checks, so debug/CI builds still abort on a regression while release builds degrade gracefully. 4. Move the QueryConditionCacheHits counter increment in read() to just before the final successful return, so a layout mismatch no longer double-counts as both a hit and a miss. 5. In FilterTransform::writeIntoQueryConditionCache, also compare marks_count and has_final_mark (not just table_uuid/part_name) when deciding whether to append to the buffered MarkRangesInfo or flush it, since a part_name can be reused with a different mark layout. 6. Add marks_count and has_final_mark columns to system.query_condition_cache so entries that differ only in these key fields are distinguishable. 7. Remove the stale/inaccurate header comment in gtest_query_condition_cache_defense_in_depth.cpp and delete its verbatim duplicate of KeepsEntriesWithDifferentMarkLayoutsSeparate (already covered by gtest_query_condition_cache.cpp and linked into the same test binary). 8. chmod 644 both gtest_query_condition_cache*.cpp files to match every other gtest source file in the tree. 9. Reword the QueryConditionCacheLayoutMismatch ProfileEvents description to describe the observable behavior instead of reading as an internal PR note. 10. Strengthen ReusedPartNameWithFinalMarkDoesNotThrowOrCorrupt to also assert on the small layout's actual mark values, not just its size. 11. Reword completed-work comments from TODO(#2342) to NOTE(#2342), and update QueryConditionCache::read()'s doc comment to describe the marks_count/ has_final_mark parameters added when its signature grew from 3 to 5 args. Also updated the RejectsRangesOutsideEntryMarkLayout test to match the write() throw -> log-and-skip conversion from finding 2 (EXPECT_NO_THROW + read-miss assertion instead of EXPECT_THROW). Verified via `unit_tests_dbms --gtest_filter="QueryConditionCache.*"`: 4/4 tests pass (5 tests minus the duplicate removed in finding 7). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_013NAYye5ozisLN7WBqzYfAn --- src/Common/ProfileEvents.cpp | 6 +-- .../Cache/QueryConditionCache.cpp | 49 ++++++++++------- src/Interpreters/Cache/QueryConditionCache.h | 4 ++ .../tests/gtest_query_condition_cache.cpp | 8 ++- ...query_condition_cache_defense_in_depth.cpp | 52 +++++-------------- src/Processors/Transforms/FilterTransform.cpp | 15 ++++-- .../StorageSystemQueryConditionCache.cpp | 6 ++- 7 files changed, 74 insertions(+), 66 deletions(-) mode change 100755 => 100644 src/Interpreters/Cache/tests/gtest_query_condition_cache.cpp mode change 100755 => 100644 src/Interpreters/Cache/tests/gtest_query_condition_cache_defense_in_depth.cpp diff --git a/src/Common/ProfileEvents.cpp b/src/Common/ProfileEvents.cpp index 1ff769925c64..9eb422464190 100644 --- a/src/Common/ProfileEvents.cpp +++ b/src/Common/ProfileEvents.cpp @@ -96,9 +96,9 @@ M(VectorSimilarityIndexCacheWeightLost, "Approximate number of bytes evicted from the vector index cache.", ValueType::Number) \ M(QueryConditionCacheHits, "Number of times an entry has been found in the query condition cache (and reading of marks can be skipped). Only updated for SELECT queries with SETTING use_query_condition_cache = 1.", ValueType::Number) \ M(QueryConditionCacheMisses, "Number of times an entry has not been found in the query condition cache (and reading of mark cannot be skipped). Only updated for SELECT queries with SETTING use_query_condition_cache = 1.", ValueType::Number) \ - M(QueryConditionCacheLayoutMismatch, "Number of times a query condition cache entry was " \ - "skipped because its mark layout did not match the current part's layout. Should be 0 after " \ - "the Key fix; a non-zero count indicates a residual hazard (see Altinity/ClickHouse#2342).", \ + M(QueryConditionCacheLayoutMismatch, "Number of times a query condition cache entry was skipped " \ + "on write or read because its mark layout (marks_count/has_final_mark) did not match the " \ + "current part's layout (see Altinity/ClickHouse#2342).", \ ValueType::Number) \ M(QueryCacheHits, "Number of times a query result has been found in the query cache (and query computation was avoided). Only updated for SELECT queries with SETTING use_query_cache = 1.", ValueType::Number) \ M(QueryCacheMisses, "Number of times a query result has not been found in the query cache (and required query computation). Only updated for SELECT queries with SETTING use_query_cache = 1.", ValueType::Number) \ diff --git a/src/Interpreters/Cache/QueryConditionCache.cpp b/src/Interpreters/Cache/QueryConditionCache.cpp index 7e676041b3e5..2df15bb17651 100644 --- a/src/Interpreters/Cache/QueryConditionCache.cpp +++ b/src/Interpreters/Cache/QueryConditionCache.cpp @@ -19,14 +19,14 @@ namespace CurrentMetrics extern const Metric QueryConditionCacheEntries; } +namespace DB +{ + namespace ErrorCodes { extern const int LOGICAL_ERROR; } -namespace DB -{ - bool QueryConditionCache::Key::operator==(const Key & other) const { return table_id == other.table_id @@ -64,22 +64,33 @@ void QueryConditionCache::write( const MarkRanges & mark_ranges, size_t marks_count, bool has_final_mark) { if (has_final_mark && marks_count == 0) - throw Exception(ErrorCodes::LOGICAL_ERROR, "A part with a final mark must have at least one mark"); + { + ProfileEvents::increment(ProfileEvents::QueryConditionCacheLayoutMismatch); + LOG_ERROR( + logger, + "Query condition cache write for table_id: {}, part_name: {}, condition_hash: {}: " + "has_final_mark is set but marks_count is 0, which is not a valid mark layout. " + "Skipping cache update for this entry. See Altinity/ClickHouse#2342.", + table_id, part_name, condition_hash); + return; + } - /// This validates ranges against the *caller-supplied* marks_count, which is a genuine caller - /// bug if it fails -- this throw is unchanged from upstream and should stay a throw. for (const auto & mark_range : mark_ranges) { if (mark_range.begin > mark_range.end || mark_range.end > marks_count) - throw Exception( - ErrorCodes::LOGICAL_ERROR, - "Invalid mark range [{}, {}) for query condition cache entry with {} marks", - mark_range.begin, - mark_range.end, - marks_count); + { + ProfileEvents::increment(ProfileEvents::QueryConditionCacheLayoutMismatch); + LOG_ERROR( + logger, + "Query condition cache write for table_id: {}, part_name: {}, condition_hash: {}: " + "invalid mark range [{}, {}) for entry with {} marks. Skipping cache update for this " + "entry. See Altinity/ClickHouse#2342.", + table_id, part_name, condition_hash, mark_range.begin, mark_range.end, marks_count); + return; + } } - /// TODO(#2342): marks_count and has_final_mark are now part of Key (see QueryConditionCache.h). + /// NOTE(#2342): marks_count and has_final_mark are now part of Key (see QueryConditionCache.h). /// Before this change, Key was {table_id, part_name, condition_hash} only. A part_name can be /// reused after its underlying data (and therefore its mark layout) has changed -- e.g. after a /// mutation, a merge that produces a part with a name collision under certain replay scenarios, @@ -99,7 +110,7 @@ void QueryConditionCache::write( { std::shared_lock shared_lock(entry->mutex); /// cheap - /// TODO(#2342): defense in depth. With marks_count in the Key, this branch should now be + /// NOTE(#2342): defense in depth. With marks_count in the Key, this branch should now be /// unreachable in normal operation -- cache.getOrSet() cannot return an Entry keyed to a /// different marks_count than the one just constructed. If this ever fires, it means either /// (a) a hash collision between two distinct Keys (extremely unlikely with SipHash64 over @@ -111,6 +122,7 @@ void QueryConditionCache::write( /// from inside the query pipeline for what may be a caching-layer inconsistency, and do NOT /// proceed to index a vector we now know is a mismatched size. This is intentionally the /// last line of defense, not the fix: the fix is the Key change above. + chassert(entry->matching_marks.size() == marks_count); if (entry->matching_marks.size() != marks_count) { ProfileEvents::increment(ProfileEvents::QueryConditionCacheLayoutMismatch); @@ -144,11 +156,12 @@ void QueryConditionCache::write( { std::lock_guard lock(entry->mutex); /// (*) - /// TODO(#2342): same defense-in-depth check as above, now under the exclusive lock. Kept + /// NOTE(#2342): same defense-in-depth check as above, now under the exclusive lock. Kept /// deliberately duplicated rather than factored into a helper: the shared_lock check above /// is an optimization (avoid taking the exclusive lock), and the entry could theoretically /// change between releasing the shared lock and acquiring the exclusive one in some future /// refactor. Re-checking here costs nothing and removes that assumption. + chassert(entry->matching_marks.size() == marks_count); if (entry->matching_marks.size() != marks_count) { ProfileEvents::increment(ProfileEvents::QueryConditionCacheLayoutMismatch); @@ -188,11 +201,9 @@ std::optional QueryConditionCache::read( if (auto entry = cache.get(key)) { - ProfileEvents::increment(ProfileEvents::QueryConditionCacheHits); - std::shared_lock lock(entry->mutex); - /// TODO(#2342): defense in depth, mirrors the write-path checks above. On the read path a + /// NOTE(#2342): defense in depth, mirrors the write-path checks above. On the read path a /// mismatch is comparatively low-risk -- the caller only reads matching_marks, it does not /// index into it with the *caller's* marks_count anywhere we've audited (see /// MergeTreeDataSelectExecutor::filterPartsByQueryConditionCache, which indexes with @@ -201,6 +212,7 @@ std::optional QueryConditionCache::read( /// confirmed every caller across all supported versions respects that invariant, and because /// returning a mismatched-size MatchingMarks to a caller that isn't expecting one is exactly /// the kind of latent hazard this whole issue is about. Treat as a cache miss on mismatch. + chassert(entry->matching_marks.size() == marks_count); if (entry->matching_marks.size() != marks_count) { ProfileEvents::increment(ProfileEvents::QueryConditionCacheLayoutMismatch); @@ -221,6 +233,7 @@ std::optional QueryConditionCache::read( part_name, condition_hash); + ProfileEvents::increment(ProfileEvents::QueryConditionCacheHits); return {entry->matching_marks}; } else diff --git a/src/Interpreters/Cache/QueryConditionCache.h b/src/Interpreters/Cache/QueryConditionCache.h index b593ee875637..efcae6644016 100644 --- a/src/Interpreters/Cache/QueryConditionCache.h +++ b/src/Interpreters/Cache/QueryConditionCache.h @@ -85,6 +85,10 @@ class QueryConditionCache const MarkRanges & mark_ranges, size_t marks_count, bool has_final_mark); /// Check the cache if it contains an entry for the given table + part id and predicate hash. + /// marks_count/has_final_mark describe the caller's current mark layout for the part; they are + /// part of the cache key and are also checked against the found entry's stored layout, since a + /// part_name can be reused with a different layout after the underlying data has changed. On a + /// layout mismatch, the entry is treated as a cache miss (see Altinity/ClickHouse#2342). std::optional read( const UUID & table_id, const String & part_name, UInt64 condition_hash, size_t marks_count, bool has_final_mark); diff --git a/src/Interpreters/Cache/tests/gtest_query_condition_cache.cpp b/src/Interpreters/Cache/tests/gtest_query_condition_cache.cpp old mode 100755 new mode 100644 index 000c95f38a49..93ba594e51d4 --- a/src/Interpreters/Cache/tests/gtest_query_condition_cache.cpp +++ b/src/Interpreters/Cache/tests/gtest_query_condition_cache.cpp @@ -38,5 +38,11 @@ TEST(QueryConditionCache, RejectsRangesOutsideEntryMarkLayout) MarkRanges invalid_range; invalid_range.emplace_back(0, 2); - EXPECT_THROW(cache.write(UUID(1), "part", 42, "", invalid_range, 1, false), Exception); + /// An invalid mark range (out of bounds for the given marks_count) is a caller bug, but write() + /// logs and skips rather than throwing from inside the query pipeline (see Altinity/ClickHouse#2342). + EXPECT_NO_THROW(cache.write(UUID(1), "part", 42, "", invalid_range, 1, false)); + + /// Nothing should have been cached for this key. + auto result = cache.read(UUID(1), "part", 42, 1, false); + EXPECT_FALSE(result); } diff --git a/src/Interpreters/Cache/tests/gtest_query_condition_cache_defense_in_depth.cpp b/src/Interpreters/Cache/tests/gtest_query_condition_cache_defense_in_depth.cpp old mode 100755 new mode 100644 index 1fb9473c7842..67a67dd2bb1c --- a/src/Interpreters/Cache/tests/gtest_query_condition_cache_defense_in_depth.cpp +++ b/src/Interpreters/Cache/tests/gtest_query_condition_cache_defense_in_depth.cpp @@ -1,12 +1,10 @@ -/// TODO(#2342): These tests exercise the defense-in-depth paths added in QueryConditionCache.patched.cpp. -/// They cannot currently trigger a *genuine* Key-hash collision (SipHash64 over table_id + part_name + -/// condition_hash + marks_count + has_final_mark is not practically collidable in a unit test), so -/// "mismatch" here is necessarily synthetic. These tests document and lock in the *behavior* (log + -/// skip, no throw, no OOB access) rather than proving the mismatch is unreachable in production -- -/// that claim rests on the Key change itself, not on these tests. -/// -/// NOT YET DONE (see package README, "Not completed" section): compiled, linked, or run. Written -/// against the same gtest/API shape as the provided gtest_query_condition_cache.cpp. +/// These tests exercise the defense-in-depth paths in QueryConditionCache.cpp (the log-and-skip +/// checks kept alongside the Key fix). They cannot currently trigger a *genuine* Key-hash collision +/// (SipHash64 over table_id + part_name + condition_hash + marks_count + has_final_mark is not +/// practically collidable in a unit test), so "mismatch" here is necessarily synthetic. These tests +/// document and lock in the *behavior* (log + skip, no throw, no OOB access) rather than proving the +/// mismatch is unreachable in production -- that claim rests on the Key change itself, not on these +/// tests. #include #include @@ -18,36 +16,6 @@ using namespace DB; -/// Baseline from the provided test file, reproduced here so this file is runnable standalone. -/// This is the primary regression test for the actual fix: two different mark layouts under the -/// same table_id/part_name/condition_hash must be kept as distinct cache entries. -TEST(QueryConditionCache, KeepsEntriesWithDifferentMarkLayoutsSeparate_DuplicateFromProvidedFile) -{ - QueryConditionCache cache("LRU", 1_MiB, 0.5); - const UUID table_id(1); - const String part_name = "part"; - constexpr UInt64 condition_hash = 42; - - MarkRanges one_mark; - one_mark.emplace_back(0, 1); - cache.write(table_id, part_name, condition_hash, "", one_mark, 1, false); - - MarkRanges two_marks; - two_marks.emplace_back(1, 2); - cache.write(table_id, part_name, condition_hash, "", two_marks, 2, false); - - auto first_layout = cache.read(table_id, part_name, condition_hash, 1, false); - ASSERT_TRUE(first_layout); - ASSERT_EQ(first_layout->size(), 1); - EXPECT_FALSE((*first_layout)[0]); - - auto second_layout = cache.read(table_id, part_name, condition_hash, 2, false); - ASSERT_TRUE(second_layout); - ASSERT_EQ(second_layout->size(), 2); - EXPECT_TRUE((*second_layout)[0]); - EXPECT_FALSE((*second_layout)[1]); -} - /// A part_name reused with a completely different mark count (simulating the part-lifecycle /// scenario this issue hypothesizes -- e.g. a part name recycled after data changed underneath it) /// must not throw and must not corrupt either entry. With the Key fix this is definitionally two @@ -74,6 +42,12 @@ TEST(QueryConditionCache, ReusedPartNameWithFinalMarkDoesNotThrowOrCorrupt) auto small_layout = cache.read(table_id, part_name, condition_hash, 3, false); ASSERT_TRUE(small_layout); ASSERT_EQ(small_layout->size(), 3); + /// small_ranges = [0, 3) covers the whole entry, so every mark must have been forced to false + /// ("must scan"); a corrupted entry (e.g. bleeding over from the differently-shaped large write) + /// could instead leave marks at their initial true default or some other value. + EXPECT_FALSE((*small_layout)[0]); + EXPECT_FALSE((*small_layout)[1]); + EXPECT_FALSE((*small_layout)[2]); auto large_layout = cache.read(table_id, part_name, condition_hash, 100, true); ASSERT_TRUE(large_layout); diff --git a/src/Processors/Transforms/FilterTransform.cpp b/src/Processors/Transforms/FilterTransform.cpp index 79e2a4962e0b..41de0f5ed74f 100644 --- a/src/Processors/Transforms/FilterTransform.cpp +++ b/src/Processors/Transforms/FilterTransform.cpp @@ -296,10 +296,17 @@ void FilterTransform::writeIntoQueryConditionCache(const MarkRangesInfoPtr & mar } else { - /// If the current and the buffer mark range info are from the same table/part, append to the buffer. - /// Otherwise write to the query condition cache and reset the buffer. - - if (buffered_mark_ranges_info->table_uuid != mark_ranges_info->table_uuid || buffered_mark_ranges_info->part_name != mark_ranges_info->part_name) + /// If the current and the buffer mark range info are from the same table/part and have the + /// same mark layout, append to the buffer. Otherwise write to the query condition cache and + /// reset the buffer. Comparing only table_uuid/part_name is not enough: a part_name can be + /// reused with a different mark layout (marks_count/has_final_mark), and appending ranges + /// computed against a different layout into the same buffer would corrupt it (see + /// Altinity/ClickHouse#2342). + + if (buffered_mark_ranges_info->table_uuid != mark_ranges_info->table_uuid + || buffered_mark_ranges_info->part_name != mark_ranges_info->part_name + || buffered_mark_ranges_info->marks_count != mark_ranges_info->marks_count + || buffered_mark_ranges_info->has_final_mark != mark_ranges_info->has_final_mark) { query_condition_cache->write( buffered_mark_ranges_info->table_uuid, diff --git a/src/Storages/System/StorageSystemQueryConditionCache.cpp b/src/Storages/System/StorageSystemQueryConditionCache.cpp index 018094db68ac..ef1fdf31ddc7 100644 --- a/src/Storages/System/StorageSystemQueryConditionCache.cpp +++ b/src/Storages/System/StorageSystemQueryConditionCache.cpp @@ -19,7 +19,9 @@ ColumnsDescription StorageSystemQueryConditionCache::getColumnsDescription() {"condition", std::make_shared(), "The hashed filter condition. Only set if setting query_condition_cache_store_conditions_as_plaintext = true."}, {"condition_hash", std::make_shared(), "The hash of the filter condition."}, {"entry_size", std::make_shared(), "The size of the entry in bytes."}, - {"matching_marks", std::make_shared(), "Matching marks."} + {"matching_marks", std::make_shared(), "Matching marks."}, + {"marks_count", std::make_shared(), "The number of marks in the part this entry was cached for."}, + {"has_final_mark", std::make_shared(), "Whether the part this entry was cached for has a final mark."} }; } @@ -55,6 +57,8 @@ void StorageSystemQueryConditionCache::fillData(MutableColumns & res_columns, Co std::shared_lock lock(entry->mutex); res_columns[5]->insert(to_string(entry->matching_marks)); + res_columns[6]->insert(key.marks_count); + res_columns[7]->insert(key.has_final_mark); } } From 87e5f311963a6970f32792fa69526d46ea34d041 Mon Sep 17 00:00:00 2001 From: Coreground Technologies Automation Account Date: Sat, 12 Sep 2026 18:22:02 -0500 Subject: [PATCH 5/5] Delete unused ErrorCodes::LOGICAL_ERROR declaration Both throws that used it were converted to log-and-skip in the previous commit (15846cb1), leaving this extern declaration and the Common/Exception.h include unused. ClickHouse's style-check CI job flags unused ErrorCodes declarations, so this would otherwise fail CI on this PR. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_013NAYye5ozisLN7WBqzYfAn --- src/Interpreters/Cache/QueryConditionCache.cpp | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/Interpreters/Cache/QueryConditionCache.cpp b/src/Interpreters/Cache/QueryConditionCache.cpp index 2df15bb17651..92ab3282aac5 100644 --- a/src/Interpreters/Cache/QueryConditionCache.cpp +++ b/src/Interpreters/Cache/QueryConditionCache.cpp @@ -1,5 +1,4 @@ #include -#include #include #include #include @@ -22,11 +21,6 @@ namespace CurrentMetrics namespace DB { -namespace ErrorCodes -{ - extern const int LOGICAL_ERROR; -} - bool QueryConditionCache::Key::operator==(const Key & other) const { return table_id == other.table_id