diff --git a/src/Common/ProfileEvents.cpp b/src/Common/ProfileEvents.cpp index ddf2efa2b823..9eb422464190 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 " \ + "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) \ 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..92ab3282aac5 100644 --- a/src/Interpreters/Cache/QueryConditionCache.cpp +++ b/src/Interpreters/Cache/QueryConditionCache.cpp @@ -9,6 +9,7 @@ namespace ProfileEvents { extern const Event QueryConditionCacheHits; extern const Event QueryConditionCacheMisses; + extern const Event QueryConditionCacheLayoutMismatch; } namespace CurrentMetrics @@ -24,7 +25,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 +36,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 +57,45 @@ 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) + { + 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; + } + + for (const auto & mark_range : mark_ranges) + { + if (mark_range.begin > mark_range.end || 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; + } + } + + /// 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, + /// 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 +104,31 @@ void QueryConditionCache::write( { std::shared_lock shared_lock(entry->mutex); /// cheap + /// 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 + /// 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. + chassert(entry->matching_marks.size() == marks_count); + 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 +150,23 @@ void QueryConditionCache::write( { std::lock_guard lock(entry->mutex); /// (*) - chassert(marks_count == entry->matching_marks.size()); + /// 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); + 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,16 +188,38 @@ 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)) { - ProfileEvents::increment(ProfileEvents::QueryConditionCacheHits); - std::shared_lock lock(entry->mutex); + /// 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 + /// 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. + chassert(entry->matching_marks.size() == marks_count); + 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: {}", @@ -121,6 +227,7 @@ std::optional QueryConditionCache::read(cons 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 729ad667fbf5..efcae6644016 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,12 @@ 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); + /// 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); /// For debugging and system tables std::vector dump() const; 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 100644 index 000000000000..93ba594e51d4 --- /dev/null +++ b/src/Interpreters/Cache/tests/gtest_query_condition_cache.cpp @@ -0,0 +1,48 @@ +#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); + + /// 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 new file mode 100644 index 000000000000..67a67dd2bb1c --- /dev/null +++ b/src/Interpreters/Cache/tests/gtest_query_condition_cache_defense_in_depth.cpp @@ -0,0 +1,90 @@ +/// 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 +#include +#include + +#include +#include + +using namespace DB; + +/// 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); + /// 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); + 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"; +} 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/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; 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); } }