From d664da1d809efb087acd5eb5218bf48ed79099b1 Mon Sep 17 00:00:00 2001 From: Mikhail Filimonov Date: Sun, 27 Sep 2026 13:55:39 +0200 Subject: [PATCH 1/8] CAS: route a path inside a part to its own directory shape `classifyDirectory` gains `DirShape::PartFile` for `//` (live, detached, moving, non-Atomic); the two `TableSubdir` case bodies become `tableSubdirExists`/`tableSubdirChildren` and the new shape answers through them for now, so no answer changes in this commit. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01V8mZSGiD8iJumpJMiQnrmC Signed-off-by: Mikhail Filimonov --- .../ContentAddressedMetadataStorage.cpp | 67 +++++++++++++------ .../ContentAddressedMetadataStorage.h | 7 ++ src/Disks/tests/gtest_ca_wiring.cpp | 17 +++++ 3 files changed, 71 insertions(+), 20 deletions(-) diff --git a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/ContentAddressedMetadataStorage.cpp b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/ContentAddressedMetadataStorage.cpp index 8fb5a31b467c..d16beff225a5 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/ContentAddressedMetadataStorage.cpp +++ b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/ContentAddressedMetadataStorage.cpp @@ -1394,6 +1394,30 @@ bool ContentAddressedMetadataStorage::liveTreeDirHasChildren(const std::string & return !store()->listMirroredChildren(scope).empty(); } +bool ContentAddressedMetadataStorage::tableSubdirExists(const Cas::TableFilePath & tf) const +{ + /// At least one verbatim file under it. + const auto life = readableNamespaceFilesLife(liveNamespace(tf.table_uuid)); + if (!life) + return false; + const std::string prefix = tf.tail + "/"; + for (const auto & name : store()->listNamespaceFiles(*life)) + if (name.starts_with(prefix)) + return true; + return false; +} + +std::vector ContentAddressedMetadataStorage::tableSubdirChildren(const Cas::TableFilePath & tf) const +{ + /// Verbatim files under /, first-component collapsed. + std::unordered_set result; + if (const auto life = readableNamespaceFilesLife(liveNamespace(tf.table_uuid))) + for (const auto & name : store()->listNamespaceFiles(*life)) + if (name.starts_with(tf.tail + "/")) + addFirstComponent(result, name.substr(tf.tail.size() + 1)); + return toVector(std::move(result)); +} + Cas::RootNamespace ContentAddressedMetadataStorage::liveNamespace(const std::string & table_uuid) const { /// Path mirroring: the namespace is the table's canonical disk path with the @@ -1605,6 +1629,21 @@ ContentAddressedMetadataStorage::DirRoute ContentAddressedMetadataStorage::class return dr; } } + /// A path with a part-shaped component followed by more components: a file or nested + /// directory of a live, detached or moving part IF that ref resolves (shadow is routed + /// above). The parser calls every first component after the table root except + /// `deduplication_logs` the part component, so whether this really is a part is decided by + /// the ref at answer time, not by the path: `existsDirectory`/`listDirectory` take the old + /// table-subdirectory branch when it does not resolve. Classification stays pure path + /// computation, so the parse that branch needs travels with the shape. + if (r && !r->ref.empty() && !r->file.empty()) + { + dr.shape = DirShape::PartFile; + dr.p = std::move(p); + dr.r = std::move(r); + dr.tf = Cas::parseTableFilePath(path); + return dr; + } /// No sub-shape matched: fall through, identical to today's post-`if (p)` continuation. } @@ -1700,17 +1739,11 @@ bool ContentAddressedMetadataStorage::existsDirectory(const std::string & path) return view && view->hasDirectory(*dr.projection_prefix); } case DirShape::TableSubdir: - { - /// At least one verbatim file under it. - const auto life = readableNamespaceFilesLife(liveNamespace(dr.tf->table_uuid)); - if (!life) - return false; - const std::string prefix = dr.tf->tail + "/"; - for (const auto & name : store()->listNamespaceFiles(*life)) - if (name.starts_with(prefix)) - return true; - return false; - } + return tableSubdirExists(*dr.tf); + case DirShape::PartFile: + /// Answered as the table subdirectory or generic directory the same path was classified + /// as before this shape existed (the view-based answer replaces this in the next commit). + return dr.tf ? tableSubdirExists(*dr.tf) : liveTreeDirHasChildren(path); case DirShape::GenericIntermediate: /// Exists iff a server-root-scoped mirrored LIST finds any object. Keeps `cd`/existence /// consistent with listDirectory so `clickhouse-disks` traversal behaves like a normal disk. @@ -1894,15 +1927,9 @@ std::vector ContentAddressedMetadataStorage::listDirectory(const st return view ? view->listChildren(*dr.projection_prefix) : std::vector{}; } case DirShape::TableSubdir: - { - /// Verbatim files under /, first-component collapsed. - std::unordered_set result; - if (const auto life = readableNamespaceFilesLife(liveNamespace(dr.tf->table_uuid))) - for (const auto & name : store()->listNamespaceFiles(*life)) - if (name.starts_with(dr.tf->tail + "/")) - addFirstComponent(result, name.substr(dr.tf->tail.size() + 1)); - return toVector(std::move(result)); - } + return tableSubdirChildren(*dr.tf); + case DirShape::PartFile: + return dr.tf ? tableSubdirChildren(*dr.tf) : listLiveTreeChildren(path); case DirShape::GenericIntermediate: /// The disk root "", `store`, or any loose-file container above a table dir: a /// server-root-scoped mirrored LIST. (`store/` is handled by AtomicShard above, diff --git a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/ContentAddressedMetadataStorage.h b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/ContentAddressedMetadataStorage.h index bffb608688cc..72862e1b59da 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/ContentAddressedMetadataStorage.h +++ b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/ContentAddressedMetadataStorage.h @@ -474,6 +474,12 @@ class ContentAddressedMetadataStorage final : public IMetadataStorage, public IC /// always considered present. bool liveTreeDirHasChildren(const std::string & path) const; + /// The table-level-subdirectory answers (a LIST of the life's `_files/` prefix filtered by + /// `tf.tail + "/"`), shared by the `TableSubdir` shape and by a `PartFile` whose ref does not + /// resolve, which must answer exactly as it did before that shape existed. + bool tableSubdirExists(const Cas::TableFilePath & tf) const; + std::vector tableSubdirChildren(const Cas::TableFilePath & tf) const; + /// Resolves one parsed path to its namespace, reference, and in-tree file. Detached paths are /// re-split here so their references remain in the table namespace with a `detached/` prefix; /// shadow paths map to a namespace derived from the literal shadow directory. @@ -516,6 +522,7 @@ class ContentAddressedMetadataStorage final : public IMetadataStorage, public IC MovingContainer, PartDir, ProjectionDir, + PartFile, TableSubdir, GenericIntermediate, }; diff --git a/src/Disks/tests/gtest_ca_wiring.cpp b/src/Disks/tests/gtest_ca_wiring.cpp index 8fe21886f780..db32c32b43b5 100644 --- a/src/Disks/tests/gtest_ca_wiring.cpp +++ b/src/Disks/tests/gtest_ca_wiring.cpp @@ -870,6 +870,23 @@ TEST(CASWiringRoute, DirShapeDispatchOrderIsStable) EXPECT_EQ(storage->classifyDirectoryForTest("shadow/bk1").shape, DS::ShadowIntermediate); EXPECT_EQ(storage->classifyDirectoryForTest("a11/a11a11a1-1111-4111-8111-111111111111/deduplication_logs").shape, DS::TableSubdir); EXPECT_EQ(storage->classifyDirectoryForTest("store").shape, DS::GenericIntermediate); + + /// A path INSIDE a part (file or nested directory): its own shape, decided by the ref at answer + /// time. Atomic, detached, moving, non-Atomic and a temporary restore part all route here; a + /// projection dir, a table-level subdir and a shadow part file keep their shapes. + const std::string tbl = "a11/a11a11a1-1111-4111-8111-111111111111"; + EXPECT_EQ(storage->classifyDirectoryForTest(tbl + "/all_1_1_0/columns.txt").shape, DS::PartFile); + EXPECT_EQ(storage->classifyDirectoryForTest(tbl + "/all_1_1_0/sub").shape, DS::PartFile); + EXPECT_EQ(storage->classifyDirectoryForTest(tbl + "/detached/all_1_1_0/columns.txt").shape, DS::PartFile); + EXPECT_EQ(storage->classifyDirectoryForTest(tbl + "/moving/all_1_1_0/columns.txt").shape, DS::PartFile); + EXPECT_EQ(storage->classifyDirectoryForTest(tbl + "/tmp_restore_all_1_1_0-abcdefgh/columns.txt").shape, DS::PartFile); + EXPECT_EQ(storage->classifyDirectoryForTest("data/db/tbl/all_1_1_0/columns.txt").shape, DS::PartFile); + EXPECT_EQ(storage->classifyDirectoryForTest(tbl + "/all_1_1_0/p.proj").shape, DS::ProjectionDir); + EXPECT_EQ(storage->classifyDirectoryForTest(tbl + "/deduplication_logs").shape, DS::TableSubdir); + EXPECT_EQ(storage->classifyDirectoryForTest("shadow/bk1/store/" + tbl + "/all_1_1_0/columns.txt").shape, DS::ShadowIntermediate); + /// The old branch's parse travels with the shape: present on an Atomic path, absent on non-Atomic. + EXPECT_TRUE(storage->classifyDirectoryForTest(tbl + "/all_1_1_0/columns.txt").tf.has_value()); + EXPECT_FALSE(storage->classifyDirectoryForTest("data/db/tbl/all_1_1_0/columns.txt").tf.has_value()); } /// ==== M-W Task 3: the write path through IMetadataTransaction ==== From 1049ff35983c5d9b4a5a97614ad26f17cc369e8c Mon Sep 17 00:00:00 2001 From: Mikhail Filimonov Date: Sun, 27 Sep 2026 14:23:48 +0200 Subject: [PATCH 2/8] CAS: answer directory probes inside a resolved part from the part-folder view `existsDirectory`/`listDirectory` on `
//` used to fall through to the table-subdirectory branch and LIST the life's `_files/` prefix per probe; `MergeTreeDataPartChecksum::checkSize` asks it for every checksum entry of every part at load (77k LISTs on a 1,672-part restart, issue #2439). A resolved ref now answers from its retained view; an unresolved one keeps the old branch. A non-projection nested directory inside a part now reports present, its children and non-empty. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01V8mZSGiD8iJumpJMiQnrmC Signed-off-by: Mikhail Filimonov --- .../ContentAddressedMetadataStorage.cpp | 31 +- .../tests/gtest_cas_directory_probes.cpp | 317 ++++++++++++++++++ 2 files changed, 346 insertions(+), 2 deletions(-) create mode 100644 src/Disks/tests/gtest_cas_directory_probes.cpp diff --git a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/ContentAddressedMetadataStorage.cpp b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/ContentAddressedMetadataStorage.cpp index d16beff225a5..7851251776f1 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/ContentAddressedMetadataStorage.cpp +++ b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/ContentAddressedMetadataStorage.cpp @@ -1550,6 +1550,20 @@ bool ContentAddressedMetadataStorage::existsFile(const std::string & path) const return view && view->findFile(r->file); } +namespace +{ + +/// The manifest prefix of a directory inside a part: the route's file with exactly one trailing +/// slash, whether or not the probe path carried one. +std::string dirPrefixOf(const std::string & file) +{ + if (file.ends_with('/')) + return file; + return file + "/"; +} + +} + ContentAddressedMetadataStorage::DirRoute ContentAddressedMetadataStorage::classifyDirectory(const std::string & path) const { DirRoute dr; @@ -1741,9 +1755,17 @@ bool ContentAddressedMetadataStorage::existsDirectory(const std::string & path) case DirShape::TableSubdir: return tableSubdirExists(*dr.tf); case DirShape::PartFile: - /// Answered as the table subdirectory or generic directory the same path was classified - /// as before this shape existed (the view-based answer replaces this in the next commit). + { + /// A resolved part answers from its folder view: a plain file has no entries under + /// its own name with a trailing slash, a nested directory has. An unresolved ref is not + /// a part we know (a table subdirectory path, or a non-Atomic part-shaped table + /// component) and answers exactly as before this shape existed. A failed resolution or + /// manifest read propagates. + auto view = partAccess()->getView(dr.r->refKey(), Cas::Freshness::CachedForLoad); + if (view) + return view->hasDirectory(dirPrefixOf(dr.r->file)); return dr.tf ? tableSubdirExists(*dr.tf) : liveTreeDirHasChildren(path); + } case DirShape::GenericIntermediate: /// Exists iff a server-root-scoped mirrored LIST finds any object. Keeps `cd`/existence /// consistent with listDirectory so `clickhouse-disks` traversal behaves like a normal disk. @@ -1929,7 +1951,12 @@ std::vector ContentAddressedMetadataStorage::listDirectory(const st case DirShape::TableSubdir: return tableSubdirChildren(*dr.tf); case DirShape::PartFile: + { + auto view = partAccess()->getView(dr.r->refKey(), Cas::Freshness::CachedForLoad); + if (view) + return view->listChildren(dirPrefixOf(dr.r->file)); return dr.tf ? tableSubdirChildren(*dr.tf) : listLiveTreeChildren(path); + } case DirShape::GenericIntermediate: /// The disk root "", `store`, or any loose-file container above a table dir: a /// server-root-scoped mirrored LIST. (`store/` is handled by AtomicShard above, diff --git a/src/Disks/tests/gtest_cas_directory_probes.cpp b/src/Disks/tests/gtest_cas_directory_probes.cpp new file mode 100644 index 000000000000..4eb05ba1aacc --- /dev/null +++ b/src/Disks/tests/gtest_cas_directory_probes.cpp @@ -0,0 +1,317 @@ +#include +#include "cas_test_helpers.h" + +#include +#include +#include +#include +#include + +#include + +#include +#include +#include +#include +#include + +namespace DB::ErrorCodes +{ + extern const int CANNOT_READ_ALL_DATA; +} + +/// Per-TU declarations of the settings this file overrides, the pattern `cas_test_helpers.h` +/// documents: defined once in `ContentAddressedSettings.cpp`, declared by each consumer. +namespace DB::ContentAddressedSetting +{ + extern const ContentAddressedSettingsBool gc_enabled; + extern const ContentAddressedSettingsUInt64 part_folder_cache_bytes; + extern const ContentAddressedSettingsUInt64 manifest_decode_cache_bytes; +} + +/// Directory probes on a path INSIDE a part (`
//`) must be answered from the +/// part-folder view, never by a LIST of the table's `_files/` prefix. The metadata storage builds its +/// own backend from an `ObjectStoragePtr`, so the instrument sits at the `IObjectStorage` layer: a +/// `LocalObjectStorage` that counts every operation it is asked, by kind and key. Every method +/// `CasObjectStorageBackend` reaches is overridden, so "zero LISTs" is satisfied by the behaviour, not +/// by an unrecorded path. + +using namespace DB::Cas::tests; + +namespace +{ + +class CountingObjectStorage : public DB::LocalObjectStorage +{ +public: + using DB::LocalObjectStorage::LocalObjectStorage; + + enum class Kind { List, Get, Head, Put, Delete }; + + bool exists(const DB::StoredObject & object) const override + { + record(Kind::Head, object.remote_path); + return DB::LocalObjectStorage::exists(object); + } + + std::unique_ptr readObject( + const DB::StoredObject & object, const DB::ReadSettings & read_settings, + std::optional read_hint, bool use_external_buffer, + bool restrict_seek) const override + { + record(Kind::Get, object.remote_path); + failIfArmed(object.remote_path); + return DB::LocalObjectStorage::readObject(object, read_settings, read_hint, use_external_buffer, restrict_seek); + } + + std::unique_ptr writeObject( + const DB::StoredObject & object, DB::WriteMode mode, + std::optional attributes, + size_t buf_size, + const DB::WriteSettings & write_settings) override + { + record(Kind::Put, object.remote_path); + return DB::LocalObjectStorage::writeObject(object, mode, attributes, buf_size, write_settings); + } + + void removeObjectIfExists(const DB::StoredObject & object) override + { + record(Kind::Delete, object.remote_path); + DB::LocalObjectStorage::removeObjectIfExists(object); + } + + void removeObjectsIfExist(const DB::StoredObjects & objects) override + { + for (const DB::StoredObject & object : objects) + record(Kind::Delete, object.remote_path); + DB::LocalObjectStorage::removeObjectsIfExist(objects); + } + + DB::ObjectMetadata getObjectMetadata(const std::string & path, bool with_tags) const override + { + record(Kind::Head, path); + return DB::LocalObjectStorage::getObjectMetadata(path, with_tags); + } + + std::optional tryGetObjectMetadata(const std::string & path, bool with_tags) const override + { + record(Kind::Head, path); + return DB::LocalObjectStorage::tryGetObjectMetadata(path, with_tags); + } + + void listObjects(const std::string & path, DB::RelativePathsWithMetadata & children, size_t max_keys) const override + { + record(Kind::List, path); + DB::LocalObjectStorage::listObjects(path, children, max_keys); + } + + bool existsOrHasAnyChild(const std::string & path) const override + { + record(Kind::List, path); + return DB::LocalObjectStorage::existsOrHasAnyChild(path); + } + + void copyObject( + const DB::StoredObject & object_from, const DB::StoredObject & object_to, + const DB::ReadSettings & read_settings, const DB::WriteSettings & write_settings, + std::optional object_to_attributes) override + { + record(Kind::Get, object_from.remote_path); + record(Kind::Put, object_to.remote_path); + DB::LocalObjectStorage::copyObject(object_from, object_to, read_settings, write_settings, object_to_attributes); + } + + size_t listCount(std::string_view prefix) const { return count(Kind::List, prefix); } + size_t getCount(std::string_view prefix) const { return count(Kind::Get, prefix); } + [[maybe_unused]] size_t headCount(std::string_view prefix) const { return count(Kind::Head, prefix); } + + /// Every recorded key of `kind` containing `needle`, so a failure names the offender. + std::vector keys(Kind kind, std::string_view needle) const + { + std::lock_guard lock(mutex); + std::vector out; + for (const auto & [k, key] : records) + if (k == kind && key.find(needle) != String::npos) + out.push_back(key); + return out; + } + + void reset() + { + std::lock_guard lock(mutex); + records.clear(); + } + + /// Task 3: every GET of a key containing `needle` throws, so a resolved ref whose manifest + /// cannot be read is an error, never a fall-through. Unused by this task's tests; the next + /// task's tests in this file exercise it. + [[maybe_unused]] void failReadsContaining(String needle) + { + std::lock_guard lock(mutex); + fail_reads_containing = std::move(needle); + } + +private: + void record(Kind kind, const std::string & key) const + { + std::lock_guard lock(mutex); + records.emplace_back(kind, key); + } + + void failIfArmed(const std::string & key) const + { + std::lock_guard lock(mutex); + if (!fail_reads_containing.empty() && key.find(fail_reads_containing) != String::npos) + throw DB::Exception(DB::ErrorCodes::CANNOT_READ_ALL_DATA, "CountingObjectStorage: injected read failure on '{}'", key); + } + + size_t count(Kind kind, std::string_view prefix) const + { + std::lock_guard lock(mutex); + size_t n = 0; + for (const auto & [k, key] : records) + if (k == kind && key.starts_with(prefix)) + ++n; + return n; + } + + mutable std::mutex mutex; + mutable std::vector> records; + String fail_reads_containing; +}; + +const std::string kTbl = "a11/a11a11a1-1111-4111-8111-111111111111"; +const std::string kNonAtomicTbl = "data/db/tbl"; + +std::shared_ptr openCountingStorage( + std::shared_ptr & out_object_storage, bool disable_caches) +{ + static std::atomic counter{0}; + const String unique = std::to_string(::getpid()) + "_" + std::to_string(counter.fetch_add(1)); + const auto root = (std::filesystem::temp_directory_path() / ("cas_dir_probes_" + unique)).string(); + std::error_code ec; + std::filesystem::remove_all(root, ec); + std::filesystem::create_directories(root, ec); + + out_object_storage = std::make_shared( + DB::LocalObjectStorageSettings("test", root, /*read_only_=*/false)); + + auto settings = makeSettingsForTest( + "test", std::filesystem::temp_directory_path() / ("cas_dir_probes_scratch_" + unique)); + /// A GC round LISTs on its own schedule; a timer is not a fence, so keep it off. + settings[DB::ContentAddressedSetting::gc_enabled] = false; + if (disable_caches) + { + settings[DB::ContentAddressedSetting::part_folder_cache_bytes] = 0; + settings[DB::ContentAddressedSetting::manifest_decode_cache_bytes] = 0; + } + auto storage = std::make_shared( + out_object_storage, "pool", "srv1", "", nullptr, settings); + storage->startup(); + return storage; +} + +/// Publishes one part through the real transaction path: every (relative file, bytes) pair is written +/// and the transaction is committed, exactly as a MergeTree part write does. +void publishPart(DB::ContentAddressedMetadataStorage & storage, const std::string & part_path, + const std::vector> & files) +{ + auto tx = storage.createTransaction(); + auto & ca_tx = dynamic_cast(*tx); + for (const auto & [file, bytes] : files) + { + auto buf = ca_tx.writeFile(part_path + "/" + file, 65536, DB::WriteMode::Rewrite, {}); + buf->write(bytes.data(), bytes.size()); + buf->finalize(); + } + tx->commit(DB::NoCommitOptions{}); +} + +std::string filesPrefixOf(DB::ContentAddressedMetadataStorage & storage, const std::string & table_path) +{ + /// The LIST that must not happen: the life's `_files/` prefix. Resolved through the storage so a + /// layout change moves the assertion instead of silently matching nothing. + const auto uuid = table_path.substr(table_path.find_last_of('/') + 1); + const auto life = storage.readableNamespaceFilesLife(storage.liveNamespace(uuid)); + return life ? storage.store()->layout().namespaceFilesPrefix(*life) : "cas/ns/state/"; +} + +} + +/// A file of a published part is not a directory; a non-projection nested directory is, lists its +/// children and is not empty; a projection directory keeps its deliberate empty answer. None of it +/// LISTs the table's `_files/` prefix, with or without the view caches. +class CASDirectoryProbes : public ::testing::TestWithParam {}; + +TEST_P(CASDirectoryProbes, PartFileAnswersFromTheViewWithoutAList) +{ + const bool disable_caches = GetParam(); + std::shared_ptr os; + auto storage = openCountingStorage(os, disable_caches); + + const std::string part = kTbl + "/all_1_1_0"; + publishPart(*storage, part, { + {"columns.txt", "cols"}, {"data.bin", "data-bytes"}, + {"sub/inner.bin", "inner"}, {"p.proj/data.bin", "proj-bytes"}}); + /// The load has happened; from here on every probe must be answered from what it retained. + ASSERT_TRUE(storage->existsDirectory(part)); + /// `existsDirectory(part)` above resolves the PartDir shape via `existsRef`, which never + /// touches the part-folder view; prime the view/manifest-decode cache explicitly through a + /// PartFile-shaped probe so the warm-cache assertions below measure the REPEAT probes, not the + /// one-time cold decode a commit's cache invalidation leaves behind. + ASSERT_FALSE(storage->existsDirectory(part + "/columns.txt")); + const std::string files_prefix = filesPrefixOf(*storage, kTbl); + os->reset(); + + EXPECT_FALSE(storage->existsDirectory(part + "/columns.txt")); + EXPECT_FALSE(storage->existsDirectory(part + "/data.bin")); + EXPECT_TRUE(storage->existsDirectory(part + "/sub")); + EXPECT_TRUE(storage->existsDirectory(part + "/sub/")); /// Review Focus 1: trailing slash + EXPECT_EQ(storage->listDirectory(part + "/sub"), (std::vector{"inner.bin"})); + EXPECT_FALSE(storage->isDirectoryEmpty(part + "/sub")); + EXPECT_TRUE(storage->isDirectoryEmpty(part + "/p.proj")); /// ProjectionDir keeps its answer + EXPECT_TRUE(storage->existsDirectory(part + "/p.proj")); + EXPECT_FALSE(storage->existsDirectory(part + "/absent")); + + /// A LOCAL-backed pool answers every emulated operation through `StoredObject(emu_root + "/" + + /// key)` (`ObjectStorageBackend::emuPath`), so a recorded key carries the object storage's own + /// key prefix ahead of the logical CAS key that `casManifestsPrefix`/the `_files` prefix name. + const std::string emu_root = os->getCommonKeyPrefix() + "/"; + EXPECT_EQ(os->listCount(emu_root + files_prefix), 0u) << "keys: " << fmt::to_string(fmt::join(os->keys(CountingObjectStorage::Kind::List, "_files"), ", ")); + EXPECT_EQ(os->listCount(""), 0u) << "no LIST of any prefix for probes inside a resolved part"; + if (!disable_caches) + EXPECT_EQ(os->getCount(""), 0u) << "warm view: no GET at all"; + else + EXPECT_GT(os->getCount(emu_root + storage->store()->layout().casManifestsPrefix()), 0u) << "cold caches: the manifest is read, never listed"; +} + +INSTANTIATE_TEST_SUITE_P(Caches, CASDirectoryProbes, ::testing::Bool(), + [](const ::testing::TestParamInfo & param_info) { return param_info.param ? "Disabled" : "Default"; }); + +/// Detached (Review Focus 4) and non-Atomic parts route through the same shape and the same view. +TEST(CASDirectoryProbes, DetachedAndNonAtomicPartFilesAnswerWithoutAList) +{ + std::shared_ptr os; + auto storage = openCountingStorage(os, /*disable_caches=*/false); + + const std::string part = kTbl + "/all_2_2_0"; + publishPart(*storage, part, {{"columns.txt", "cols"}, {"sub/x.bin", "x"}}); + { + auto tx = storage->createTransaction(); + tx->moveDirectory(part, kTbl + "/detached/all_2_2_0"); + tx->commit(DB::NoCommitOptions{}); + } + const std::string detached = kTbl + "/detached/all_2_2_0"; + ASSERT_TRUE(storage->existsDirectory(detached)); + + const std::string na_part = kNonAtomicTbl + "/all_1_1_0"; + publishPart(*storage, na_part, {{"columns.txt", "cols"}, {"sub/y.bin", "y"}}); + ASSERT_TRUE(storage->existsDirectory(na_part)); + + os->reset(); + EXPECT_FALSE(storage->existsDirectory(detached + "/columns.txt")); + EXPECT_TRUE(storage->existsDirectory(detached + "/sub")); + EXPECT_FALSE(storage->existsDirectory(na_part + "/columns.txt")); + EXPECT_TRUE(storage->existsDirectory(na_part + "/sub")); + EXPECT_EQ(os->listCount(""), 0u) << "keys: " << fmt::to_string(fmt::join(os->keys(CountingObjectStorage::Kind::List, ""), ", ")); +} From 028beb10361c645f71902c8fc68ba90576c44da0 Mon Sep 17 00:00:00 2001 From: Mikhail Filimonov Date: Sun, 27 Sep 2026 14:57:51 +0200 Subject: [PATCH 3/8] CAS: pin the unresolved-ref, failure and load-profile contracts of part-file probes Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01V8mZSGiD8iJumpJMiQnrmC Signed-off-by: Mikhail Filimonov --- .../tests/gtest_cas_directory_probes.cpp | 181 ++++++++++++++++-- 1 file changed, 166 insertions(+), 15 deletions(-) diff --git a/src/Disks/tests/gtest_cas_directory_probes.cpp b/src/Disks/tests/gtest_cas_directory_probes.cpp index 4eb05ba1aacc..da0442ea8329 100644 --- a/src/Disks/tests/gtest_cas_directory_probes.cpp +++ b/src/Disks/tests/gtest_cas_directory_probes.cpp @@ -4,15 +4,15 @@ #include #include #include -#include -#include #include #include +#include #include #include #include +#include #include namespace DB::ErrorCodes @@ -142,10 +142,9 @@ class CountingObjectStorage : public DB::LocalObjectStorage records.clear(); } - /// Task 3: every GET of a key containing `needle` throws, so a resolved ref whose manifest - /// cannot be read is an error, never a fall-through. Unused by this task's tests; the next - /// task's tests in this file exercise it. - [[maybe_unused]] void failReadsContaining(String needle) + /// Every GET of a key containing `needle` throws, so a resolved ref whose manifest cannot be + /// read is an error, never a fall-through. + void failReadsContaining(String needle) { std::lock_guard lock(mutex); fail_reads_containing = std::move(needle); @@ -183,12 +182,46 @@ class CountingObjectStorage : public DB::LocalObjectStorage const std::string kTbl = "a11/a11a11a1-1111-4111-8111-111111111111"; const std::string kNonAtomicTbl = "data/db/tbl"; -std::shared_ptr openCountingStorage( +/// Owns the pool's two on-disk temp directories (object-storage root and metadata scratch dir) and +/// removes both once the metadata storage they back is gone, so a test run does not leak two +/// directories under the temp dir per call to `openCountingStorage`. `storage->`/`*storage` forward +/// to the held metadata storage, so callers use it exactly like the `shared_ptr` it replaces. +class CountingStoragePool +{ +public: + CountingStoragePool(std::shared_ptr storage_, + std::string root_, std::string scratch_) + : storage(std::move(storage_)), root(std::move(root_)), scratch(std::move(scratch_)) + { + } + + CountingStoragePool(const CountingStoragePool &) = delete; + CountingStoragePool & operator=(const CountingStoragePool &) = delete; + + ~CountingStoragePool() + { + storage.reset(); + std::error_code ec; + std::filesystem::remove_all(root, ec); + std::filesystem::remove_all(scratch, ec); + } + + DB::ContentAddressedMetadataStorage * operator->() const { return storage.get(); } + DB::ContentAddressedMetadataStorage & operator*() const { return *storage; } + +private: + std::shared_ptr storage; + std::string root; + std::string scratch; +}; + +CountingStoragePool openCountingStorage( std::shared_ptr & out_object_storage, bool disable_caches) { static std::atomic counter{0}; const String unique = std::to_string(::getpid()) + "_" + std::to_string(counter.fetch_add(1)); const auto root = (std::filesystem::temp_directory_path() / ("cas_dir_probes_" + unique)).string(); + const auto scratch = (std::filesystem::temp_directory_path() / ("cas_dir_probes_scratch_" + unique)).string(); std::error_code ec; std::filesystem::remove_all(root, ec); std::filesystem::create_directories(root, ec); @@ -196,8 +229,7 @@ std::shared_ptr openCountingStorage( out_object_storage = std::make_shared( DB::LocalObjectStorageSettings("test", root, /*read_only_=*/false)); - auto settings = makeSettingsForTest( - "test", std::filesystem::temp_directory_path() / ("cas_dir_probes_scratch_" + unique)); + auto settings = makeSettingsForTest("test", scratch); /// A GC round LISTs on its own schedule; a timer is not a fence, so keep it off. settings[DB::ContentAddressedSetting::gc_enabled] = false; if (disable_caches) @@ -208,7 +240,7 @@ std::shared_ptr openCountingStorage( auto storage = std::make_shared( out_object_storage, "pool", "srv1", "", nullptr, settings); storage->startup(); - return storage; + return CountingStoragePool(std::move(storage), root, scratch); } /// Publishes one part through the real transaction path: every (relative file, bytes) pair is written @@ -227,13 +259,24 @@ void publishPart(DB::ContentAddressedMetadataStorage & storage, const std::strin tx->commit(DB::NoCommitOptions{}); } +/// The table's readable namespace-files life, the handle every verbatim (non-part) table file is +/// keyed under. A null life means the test set the table up wrong (no live life to name), so this +/// throws rather than falling back to a literal that would silently match no recorded key and pass +/// a LIST-count assertion vacuously. +DB::Cas::NamespaceLifeId namespaceFilesLifeOf(DB::ContentAddressedMetadataStorage & storage, const std::string & table_path) +{ + const auto uuid = table_path.substr(table_path.find_last_of('/') + 1); + const auto life = storage.readableNamespaceFilesLife(storage.liveNamespace(uuid)); + if (!life) + throw std::runtime_error("namespaceFilesLifeOf: no readable namespace-files life for " + table_path); + return *life; +} + std::string filesPrefixOf(DB::ContentAddressedMetadataStorage & storage, const std::string & table_path) { /// The LIST that must not happen: the life's `_files/` prefix. Resolved through the storage so a /// layout change moves the assertion instead of silently matching nothing. - const auto uuid = table_path.substr(table_path.find_last_of('/') + 1); - const auto life = storage.readableNamespaceFilesLife(storage.liveNamespace(uuid)); - return life ? storage.store()->layout().namespaceFilesPrefix(*life) : "cas/ns/state/"; + return storage.store()->layout().namespaceFilesPrefix(namespaceFilesLifeOf(storage, table_path)); } } @@ -266,8 +309,9 @@ TEST_P(CASDirectoryProbes, PartFileAnswersFromTheViewWithoutAList) EXPECT_FALSE(storage->existsDirectory(part + "/columns.txt")); EXPECT_FALSE(storage->existsDirectory(part + "/data.bin")); EXPECT_TRUE(storage->existsDirectory(part + "/sub")); - EXPECT_TRUE(storage->existsDirectory(part + "/sub/")); /// Review Focus 1: trailing slash + EXPECT_TRUE(storage->existsDirectory(part + "/sub/")); /// trailing slash EXPECT_EQ(storage->listDirectory(part + "/sub"), (std::vector{"inner.bin"})); + EXPECT_TRUE(storage->listDirectory(part + "/columns.txt").empty()); /// a plain file lists empty EXPECT_FALSE(storage->isDirectoryEmpty(part + "/sub")); EXPECT_TRUE(storage->isDirectoryEmpty(part + "/p.proj")); /// ProjectionDir keeps its answer EXPECT_TRUE(storage->existsDirectory(part + "/p.proj")); @@ -288,7 +332,7 @@ TEST_P(CASDirectoryProbes, PartFileAnswersFromTheViewWithoutAList) INSTANTIATE_TEST_SUITE_P(Caches, CASDirectoryProbes, ::testing::Bool(), [](const ::testing::TestParamInfo & param_info) { return param_info.param ? "Disabled" : "Default"; }); -/// Detached (Review Focus 4) and non-Atomic parts route through the same shape and the same view. +/// Detached and non-Atomic parts route through the same shape and the same view. TEST(CASDirectoryProbes, DetachedAndNonAtomicPartFilesAnswerWithoutAList) { std::shared_ptr os; @@ -315,3 +359,110 @@ TEST(CASDirectoryProbes, DetachedAndNonAtomicPartFilesAnswerWithoutAList) EXPECT_TRUE(storage->existsDirectory(na_part + "/sub")); EXPECT_EQ(os->listCount(""), 0u) << "keys: " << fmt::to_string(fmt::join(os->keys(CountingObjectStorage::Kind::List, ""), ", ")); } + +/// An unresolved ref is not a part: the old branch answers, with today's one LIST. Exact oracle: +/// answer and LIST count per probe. +TEST(CASDirectoryProbes, UnresolvedRefKeepsTheTableSubdirBranchAndItsOneList) +{ + std::shared_ptr os; + auto storage = openCountingStorage(os, /*disable_caches=*/false); + /// A real part so the table has a live life and a resident ref table. + publishPart(*storage, kTbl + "/all_1_1_0", {{"columns.txt", "cols"}}); + /// "custom" sits directly under the table's UUID, so the path parser anchors it exactly like a + /// real part component; the disk write path (`tryCreateWriteBuffer`) therefore cannot write + /// through it as a verbatim file (it always routes a part-shaped path into the part-write + /// machinery). Writing straight through the namespace-file primitive `tableSubdirExists` itself + /// reads is what makes "custom" content on disk without ever publishing it as a part ref. + storage->store()->putNamespaceFile(namespaceFilesLifeOf(*storage, kTbl), "custom/sub/x", "x"); + const std::string emu_root = os->getCommonKeyPrefix() + "/"; + const std::string files_prefix = emu_root + filesPrefixOf(*storage, kTbl); + + os->reset(); + EXPECT_TRUE(storage->existsDirectory(kTbl + "/custom/sub")); + EXPECT_EQ(os->listCount(files_prefix), 1u); + + os->reset(); + EXPECT_FALSE(storage->existsDirectory(kTbl + "/custom/nothere")); + EXPECT_EQ(os->listCount(files_prefix), 1u); + + os->reset(); /// a part-shaped name with no such part + EXPECT_FALSE(storage->existsDirectory(kTbl + "/all_9_9_0/columns.txt")); + EXPECT_EQ(os->listCount(files_prefix), 1u); + + os->reset(); /// non-Atomic, unresolved: the generic live-tree probe, not the `_files/` prefix + EXPECT_FALSE(storage->existsDirectory(kNonAtomicTbl + "/all_9_9_0/f")); + EXPECT_EQ(os->listCount(files_prefix), 0u); + EXPECT_EQ(os->listCount(""), 1u) << "keys: " << fmt::to_string(fmt::join(os->keys(CountingObjectStorage::Kind::List, ""), ", ")); +} + +/// A part being written in an open transaction is not published: its ref does not resolve through +/// the storage, so the probe takes today's branch and today's answer. +TEST(CASDirectoryProbes, UnpublishedPartFallsThroughLikeToday) +{ + std::shared_ptr os; + auto storage = openCountingStorage(os, /*disable_caches=*/false); + publishPart(*storage, kTbl + "/all_1_1_0", {{"columns.txt", "cols"}}); + + auto tx = storage->createTransaction(); + auto & ca_tx = dynamic_cast(*tx); + const std::string staged = kTbl + "/tmp_insert_all_2_2_0"; + auto buf = ca_tx.writeFile(staged + "/sub/data.bin", 65536, DB::WriteMode::Rewrite, {}); + buf->write("d", 1); + buf->finalize(); + + const std::string emu_root = os->getCommonKeyPrefix() + "/"; + const std::string files_prefix = emu_root + filesPrefixOf(*storage, kTbl); + os->reset(); + EXPECT_NO_THROW(EXPECT_FALSE(storage->existsDirectory(staged + "/sub"))); + EXPECT_EQ(os->listCount(files_prefix), 1u) << "unpublished: the old branch and its LIST, unchanged"; + tx->commit(DB::NoCommitOptions{}); +} + +/// Failure is not absence: a resolved ref whose manifest cannot be read throws; the old LIST branch +/// is never entered on a failed request. +TEST(CASDirectoryProbes, FailedManifestReadPropagatesAndDoesNotList) +{ + std::shared_ptr os; + auto storage = openCountingStorage(os, /*disable_caches=*/true); + const std::string part = kTbl + "/all_1_1_0"; + publishPart(*storage, part, {{"columns.txt", "cols"}}); + const std::string emu_root = os->getCommonKeyPrefix() + "/"; + const std::string files_prefix = emu_root + filesPrefixOf(*storage, kTbl); + + os->failReadsContaining(emu_root + storage->store()->layout().casManifestsPrefix()); + os->reset(); + EXPECT_THROW(storage->existsDirectory(part + "/columns.txt"), DB::Exception); + EXPECT_EQ(os->listCount(files_prefix), 0u); + EXPECT_EQ(os->listCount(""), 0u); +} + +/// The load profile: after the one legitimate table-directory enumeration, probing every file of +/// every part adds no LIST. +TEST(CASDirectoryProbes, CheckSizeProbesOfFiftyPartsAddNoList) +{ + std::shared_ptr os; + auto storage = openCountingStorage(os, /*disable_caches=*/false); + const std::vector files = {"columns.txt", "checksums.txt", "count.txt", "data.bin", "data.cmrk3", "primary.cidx"}; + for (int i = 1; i <= 50; ++i) + { + std::vector> contents; + for (const auto & f : files) + contents.emplace_back(f, "bytes-" + std::to_string(i)); + publishPart(*storage, kTbl + "/all_" + std::to_string(i) + "_" + std::to_string(i) + "_0", contents); + } + const std::string emu_root = os->getCommonKeyPrefix() + "/"; + const std::string files_prefix = emu_root + filesPrefixOf(*storage, kTbl); + + /// The table directory enumeration a load does once (its one `_files/` LIST is legitimate). + auto names = storage->listDirectory(kTbl); + ASSERT_EQ(std::count_if(names.begin(), names.end(), [](const auto & n) { return n.starts_with("all_"); }), 50); + os->reset(); + + for (const auto & name : names) + if (name.starts_with("all_")) + for (const auto & f : files) + EXPECT_FALSE(storage->existsDirectory(kTbl + "/" + name + "/" + f)); + + EXPECT_EQ(os->listCount(files_prefix), 0u) << "keys: " << fmt::to_string(fmt::join(os->keys(CountingObjectStorage::Kind::List, "_files"), ", ")); + EXPECT_EQ(os->listCount(""), 0u); +} From fa16853d391ba1c2f2d27555fec1073a785b59e4 Mon Sep 17 00:00:00 2001 From: Mikhail Filimonov Date: Sun, 27 Sep 2026 15:40:49 +0200 Subject: [PATCH 4/8] CAS: stateless test, ATTACH LIST count independent of the part count Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01V8mZSGiD8iJumpJMiQnrmC Signed-off-by: Mikhail Filimonov --- ...053_cas_part_file_probes_no_list.reference | 6 +++ .../05053_cas_part_file_probes_no_list.sh | 48 +++++++++++++++++++ 2 files changed, 54 insertions(+) create mode 100644 tests/queries/0_stateless/05053_cas_part_file_probes_no_list.reference create mode 100755 tests/queries/0_stateless/05053_cas_part_file_probes_no_list.sh diff --git a/tests/queries/0_stateless/05053_cas_part_file_probes_no_list.reference b/tests/queries/0_stateless/05053_cas_part_file_probes_no_list.reference new file mode 100644 index 000000000000..cccdd12dc23c --- /dev/null +++ b/tests/queries/0_stateless/05053_cas_part_file_probes_no_list.reference @@ -0,0 +1,6 @@ +parts_20 20 +rows_after_attach_20 20 +parts_200 200 +rows_after_attach_200 200 +lists_equal 1 lists_below_parts 1 +dropped_ok diff --git a/tests/queries/0_stateless/05053_cas_part_file_probes_no_list.sh b/tests/queries/0_stateless/05053_cas_part_file_probes_no_list.sh new file mode 100755 index 000000000000..5ac9d8383cfe --- /dev/null +++ b/tests/queries/0_stateless/05053_cas_part_file_probes_no_list.sh @@ -0,0 +1,48 @@ +#!/usr/bin/env bash +# Tags: no-fasttest +# ^ cas is an object-storage metadata type; keep it off the minimal fasttest image. + +# Loading a table on a cas disk must not issue one S3 LIST per part file: checkSize asks +# existsDirectory for every checksum entry of every part, and that probe used to LIST the table's +# _files/ prefix each time. ATTACH reloads every part synchronously inside the query, so the ATTACH +# query's own ProfileEvents (unaffected by parallel tests and by background work) are the oracle: +# the LIST count of a 200-part table equals that of a 20-part table and stays below the part count. + +CUR_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +# shellcheck source=../shell_config.sh +. "$CUR_DIR"/../shell_config.sh + +DISK="disk(type = object_storage, object_storage_type = local, metadata_type = cas, + cas_server_root_id = '${CLICKHOUSE_DATABASE}_cas95', + name = '${CLICKHOUSE_DATABASE}_cas95_cas', + path = '${CLICKHOUSE_DATABASE}_cas95_cas_pool/')" + +for n in 20 200; do + ${CLICKHOUSE_CLIENT} -q "DROP TABLE IF EXISTS t_$n" + ${CLICKHOUSE_CLIENT} -q "CREATE TABLE t_$n (a UInt64) ENGINE = MergeTree ORDER BY a + SETTINGS disk = $DISK, max_bytes_to_merge_at_max_space_in_pool = 1" + # One row per block, one block per part: n parts from one INSERT. + ${CLICKHOUSE_CLIENT} -q "INSERT INTO t_$n SELECT number FROM numbers($n) + SETTINGS max_block_size = 1, min_insert_block_size_rows = 1, min_insert_block_size_bytes = 1" + ${CLICKHOUSE_CLIENT} -q "SELECT 'parts_$n', count() FROM system.parts + WHERE database = currentDatabase() AND table = 't_$n' AND active" + ${CLICKHOUSE_CLIENT} -q "DETACH TABLE t_$n" + ${CLICKHOUSE_CLIENT} --query_id "${CLICKHOUSE_DATABASE}_attach_$n" -q "ATTACH TABLE t_$n" + ${CLICKHOUSE_CLIENT} -q "SELECT 'rows_after_attach_$n', count() FROM t_$n" +done + +${CLICKHOUSE_CLIENT} -q "SYSTEM FLUSH LOGS query_log" + +${CLICKHOUSE_CLIENT} -q " +WITH + (SELECT ProfileEvents['CASRootList'] FROM system.query_log + WHERE current_database = currentDatabase() AND type = 'QueryFinish' + AND query_id = '${CLICKHOUSE_DATABASE}_attach_20' ORDER BY event_time DESC LIMIT 1) AS l20, + (SELECT ProfileEvents['CASRootList'] FROM system.query_log + WHERE current_database = currentDatabase() AND type = 'QueryFinish' + AND query_id = '${CLICKHOUSE_DATABASE}_attach_200' ORDER BY event_time DESC LIMIT 1) AS l200 +SELECT 'lists_equal', l200 = l20, 'lists_below_parts', l200 < 20" + +${CLICKHOUSE_CLIENT} -q "DROP TABLE t_20" +${CLICKHOUSE_CLIENT} -q "DROP TABLE t_200" +${CLICKHOUSE_CLIENT} -q "SELECT 'dropped_ok'" From 74bbac342458a12f493a0559a7a81ede0afc503a Mon Sep 17 00:00:00 2001 From: Mikhail Filimonov Date: Sun, 27 Sep 2026 15:51:41 +0200 Subject: [PATCH 5/8] CAS: decommission the ad-hoc disk in the part-file-probes stateless test SYSTEM CAS FORGET was missing from 05053_cas_part_file_probes_no_list.sh, unlike 04278_cas_disk.sh's model; without it the custom disk and its GC thread stay registered for the server process's lifetime. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01V8mZSGiD8iJumpJMiQnrmC Signed-off-by: Mikhail Filimonov --- .../0_stateless/05053_cas_part_file_probes_no_list.sh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/queries/0_stateless/05053_cas_part_file_probes_no_list.sh b/tests/queries/0_stateless/05053_cas_part_file_probes_no_list.sh index 5ac9d8383cfe..414e98f6400f 100755 --- a/tests/queries/0_stateless/05053_cas_part_file_probes_no_list.sh +++ b/tests/queries/0_stateless/05053_cas_part_file_probes_no_list.sh @@ -46,3 +46,8 @@ SELECT 'lists_equal', l200 = l20, 'lists_below_parts', l200 < 20" ${CLICKHOUSE_CLIENT} -q "DROP TABLE t_20" ${CLICKHOUSE_CLIENT} -q "DROP TABLE t_200" ${CLICKHOUSE_CLIENT} -q "SELECT 'dropped_ok'" + +# FORGET logs an operator WARNING (the decommission is deliberately prominent in the server log); the +# clickhouse-test harness runs the client at --send_logs_level=warning, which would stream that expected +# warning to stderr and be flagged as a failure. Suppress it for the FORGET call only. +${CLICKHOUSE_CLIENT} --send_logs_level=fatal -q "SYSTEM CAS FORGET '${CLICKHOUSE_DATABASE}_cas95_cas'" From c7a2ed5cde8d88fbf91f3acbd98883ad8a54bcc1 Mon Sep 17 00:00:00 2001 From: Mikhail Filimonov Date: Sun, 27 Sep 2026 15:56:31 +0200 Subject: [PATCH 6/8] docs(cas): part-level directory probes are answered from the part manifest Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01V8mZSGiD8iJumpJMiQnrmC Signed-off-by: Mikhail Filimonov --- docs/en/antalya/cas/architecture/read-path.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/docs/en/antalya/cas/architecture/read-path.md b/docs/en/antalya/cas/architecture/read-path.md index c85524ad4ea3..93e38d36a042 100644 --- a/docs/en/antalya/cas/architecture/read-path.md +++ b/docs/en/antalya/cas/architecture/read-path.md @@ -12,7 +12,11 @@ doc_type: 'reference' A `CAS` read never touches a classical local-metadata path: there is no local directory listing to consult, only a ref resolve followed by object-store reads. This page covers the three ways a file access is served, the full chain for the common case, the two caches that sit on that chain, and -how a part still open inside a write transaction serves its own reads. +how a part still open inside a write transaction serves its own reads. A directory probe on a path +inside a part (`
//`, which `MergeTree` issues for every checksum entry at load) +is answered from the part's retained folder manifest: a plain file is not a directory, a nested +directory is and lists its children. No object-store LIST is involved; only a probe whose part does +not resolve falls back to the table-level file listing. ## How a file access is served {#access-kinds} From 9f6bb8d939bef3221dddf948d791756ed2c19af0 Mon Sep 17 00:00:00 2001 From: Mikhail Filimonov Date: Sun, 27 Sep 2026 18:19:56 +0200 Subject: [PATCH 7/8] =?UTF-8?q?CAS:=20fix=20wave=20after=20the=20final=20r?= =?UTF-8?q?eview=20=E2=80=94=20drop=20a=20dead=20helper=20arm,=20tighten?= =?UTF-8?q?=20three=20test=20oracles,=20fix=20the=20gtest=20gate=20filter?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes from the final whole-branch review and the codex review of the part-file directory probes: - Drop `dirPrefixOf`. Its trailing-slash arm never runs — `PartPathParser`'s `splitNonEmpty` drops empty components, so `Route::file` never ends in `/` — and its comment claimed work it never did. Both `PartFile` call sites now use `dr.r->file + "/"` directly, as `existsFileOrDirectory` already did. - `PartFileAnswersFromTheViewWithoutAList`'s cold-cache case now asserts the manifest `GET` count exactly (nine probes reach `getView`; the tenth, `isDirectoryEmpty` on a projection directory, short-circuits) instead of merely `> 0`. - `FailedManifestReadPropagatesAndDoesNotList` now injects `CORRUPTED_DATA` instead of `CANNOT_READ_ALL_DATA`. `CORRUPTED_DATA` is a deterministic local failure, so the read engine propagates it on the first attempt instead of retrying it to the lease budget (~20s) like a transport fault; the test now asserts the propagated code, that the message names the manifest key, and that exactly one manifest `GET` and zero `LIST`s were issued. - `INSTANTIATE_TEST_SUITE_P`'s instance name is now `CASCaches` (was `Caches`), so the two parameterized `PartFileAnswersFromTheViewWithoutAList` cases match the `CAS*` gate filter used elsewhere; they were silently excluded from it before. - `openCountingStorage`'s directory-owning guard is now constructed before `storage->startup()`, so a throwing `startup()` still cleans up the two temp directories; `create_directories`'s error is now propagated instead of ignored. - The stateless test's oracle now also asserts `l20 > 0`, so it cannot pass vacuously when the `CASRootList` counter reads zero for both `ATTACH`es. - Two comments that framed the unresolved-ref invariant as history ("answers exactly as before this shape existed") now state the invariant directly; the `PartFile` classification comment's parser description is now qualified to Atomic paths, since a non-Atomic path anchors on the rightmost part-shaped component instead. Verified: `ninja -C build unit_tests_dbms clickhouse` clean; `unit_tests_dbms --gtest_filter='CAS*'` — 2536 tests, all passed (`PartFileAnswersFromTheViewWithoutAList/Default` and `/Disabled` now included; `FailedManifestReadPropagatesAndDoesNotList` down from ~20s to 14ms); `05053_cas_part_file_probes_no_list` with `--test-runs 5` against a standalone server — 5/5 passed. Related: https://github.com/Altinity/ClickHouse/issues/2439 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01V8mZSGiD8iJumpJMiQnrmC Signed-off-by: Mikhail Filimonov --- .../ContentAddressedMetadataStorage.cpp | 34 ++++------- .../ContentAddressedMetadataStorage.h | 3 +- .../tests/gtest_cas_directory_probes.cpp | 60 +++++++++++++++---- ...053_cas_part_file_probes_no_list.reference | 2 +- .../05053_cas_part_file_probes_no_list.sh | 2 +- 5 files changed, 64 insertions(+), 37 deletions(-) diff --git a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/ContentAddressedMetadataStorage.cpp b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/ContentAddressedMetadataStorage.cpp index 7851251776f1..48db51308009 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/ContentAddressedMetadataStorage.cpp +++ b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/ContentAddressedMetadataStorage.cpp @@ -1550,20 +1550,6 @@ bool ContentAddressedMetadataStorage::existsFile(const std::string & path) const return view && view->findFile(r->file); } -namespace -{ - -/// The manifest prefix of a directory inside a part: the route's file with exactly one trailing -/// slash, whether or not the probe path carried one. -std::string dirPrefixOf(const std::string & file) -{ - if (file.ends_with('/')) - return file; - return file + "/"; -} - -} - ContentAddressedMetadataStorage::DirRoute ContentAddressedMetadataStorage::classifyDirectory(const std::string & path) const { DirRoute dr; @@ -1645,11 +1631,13 @@ ContentAddressedMetadataStorage::DirRoute ContentAddressedMetadataStorage::class } /// A path with a part-shaped component followed by more components: a file or nested /// directory of a live, detached or moving part IF that ref resolves (shadow is routed - /// above). The parser calls every first component after the table root except - /// `deduplication_logs` the part component, so whether this really is a part is decided by - /// the ref at answer time, not by the path: `existsDirectory`/`listDirectory` take the old - /// table-subdirectory branch when it does not resolve. Classification stays pure path - /// computation, so the parse that branch needs travels with the shape. + /// above). For an Atomic table path, the parser calls every first component after the + /// table root except `deduplication_logs` the part component; a non-Atomic table path + /// anchors on the rightmost part-shaped component instead. Either way, whether this really + /// is a part is decided by the ref at answer time, not by the path: + /// `existsDirectory`/`listDirectory` take the old table-subdirectory branch when it does + /// not resolve. Classification stays pure path computation, so the parse that branch needs + /// travels with the shape. if (r && !r->ref.empty() && !r->file.empty()) { dr.shape = DirShape::PartFile; @@ -1759,11 +1747,11 @@ bool ContentAddressedMetadataStorage::existsDirectory(const std::string & path) /// A resolved part answers from its folder view: a plain file has no entries under /// its own name with a trailing slash, a nested directory has. An unresolved ref is not /// a part we know (a table subdirectory path, or a non-Atomic part-shaped table - /// component) and answers exactly as before this shape existed. A failed resolution or - /// manifest read propagates. + /// component), so it answers as the table subdirectory or generic directory that the + /// same path denotes. A failed resolution or manifest read propagates. auto view = partAccess()->getView(dr.r->refKey(), Cas::Freshness::CachedForLoad); if (view) - return view->hasDirectory(dirPrefixOf(dr.r->file)); + return view->hasDirectory(dr.r->file + "/"); return dr.tf ? tableSubdirExists(*dr.tf) : liveTreeDirHasChildren(path); } case DirShape::GenericIntermediate: @@ -1954,7 +1942,7 @@ std::vector ContentAddressedMetadataStorage::listDirectory(const st { auto view = partAccess()->getView(dr.r->refKey(), Cas::Freshness::CachedForLoad); if (view) - return view->listChildren(dirPrefixOf(dr.r->file)); + return view->listChildren(dr.r->file + "/"); return dr.tf ? tableSubdirChildren(*dr.tf) : listLiveTreeChildren(path); } case DirShape::GenericIntermediate: diff --git a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/ContentAddressedMetadataStorage.h b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/ContentAddressedMetadataStorage.h index 72862e1b59da..646df4ec2f40 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/ContentAddressedMetadataStorage.h +++ b/src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/ContentAddressedMetadataStorage.h @@ -476,7 +476,8 @@ class ContentAddressedMetadataStorage final : public IMetadataStorage, public IC /// The table-level-subdirectory answers (a LIST of the life's `_files/` prefix filtered by /// `tf.tail + "/"`), shared by the `TableSubdir` shape and by a `PartFile` whose ref does not - /// resolve, which must answer exactly as it did before that shape existed. + /// resolve: an unresolved ref answers as the table subdirectory or generic directory that the + /// same path denotes. bool tableSubdirExists(const Cas::TableFilePath & tf) const; std::vector tableSubdirChildren(const Cas::TableFilePath & tf) const; diff --git a/src/Disks/tests/gtest_cas_directory_probes.cpp b/src/Disks/tests/gtest_cas_directory_probes.cpp index da0442ea8329..41f664527eef 100644 --- a/src/Disks/tests/gtest_cas_directory_probes.cpp +++ b/src/Disks/tests/gtest_cas_directory_probes.cpp @@ -17,7 +17,7 @@ namespace DB::ErrorCodes { - extern const int CANNOT_READ_ALL_DATA; + extern const int CORRUPTED_DATA; } /// Per-TU declarations of the settings this file overrides, the pattern `cas_test_helpers.h` @@ -143,7 +143,9 @@ class CountingObjectStorage : public DB::LocalObjectStorage } /// Every GET of a key containing `needle` throws, so a resolved ref whose manifest cannot be - /// read is an error, never a fall-through. + /// read is an error, never a fall-through. `CORRUPTED_DATA` is a deterministic local failure + /// (`isDeterministicLocalFailure`), so the read engine propagates it on the first attempt + /// instead of retrying it to the lease budget like a transport fault. void failReadsContaining(String needle) { std::lock_guard lock(mutex); @@ -161,7 +163,7 @@ class CountingObjectStorage : public DB::LocalObjectStorage { std::lock_guard lock(mutex); if (!fail_reads_containing.empty() && key.find(fail_reads_containing) != String::npos) - throw DB::Exception(DB::ErrorCodes::CANNOT_READ_ALL_DATA, "CountingObjectStorage: injected read failure on '{}'", key); + throw DB::Exception(DB::ErrorCodes::CORRUPTED_DATA, "CountingObjectStorage: injected read failure on '{}'", key); } size_t count(Kind kind, std::string_view prefix) const @@ -189,14 +191,19 @@ const std::string kNonAtomicTbl = "data/db/tbl"; class CountingStoragePool { public: - CountingStoragePool(std::shared_ptr storage_, - std::string root_, std::string scratch_) - : storage(std::move(storage_)), root(std::move(root_)), scratch(std::move(scratch_)) + /// Owns the two directories from construction, before a metadata storage exists: a throwing + /// `startup()` still unwinds through this object's destructor and removes them. + CountingStoragePool(std::string root_, std::string scratch_) + : root(std::move(root_)), scratch(std::move(scratch_)) { } CountingStoragePool(const CountingStoragePool &) = delete; CountingStoragePool & operator=(const CountingStoragePool &) = delete; + /// Needed for the by-value return from `openCountingStorage`: NRVO is not guaranteed for a + /// named local, and the deleted copy constructor above suppresses the implicit move + /// constructor the language would otherwise synthesize. + CountingStoragePool(CountingStoragePool &&) = default; ~CountingStoragePool() { @@ -206,6 +213,8 @@ class CountingStoragePool std::filesystem::remove_all(scratch, ec); } + void attach(std::shared_ptr storage_) { storage = std::move(storage_); } + DB::ContentAddressedMetadataStorage * operator->() const { return storage.get(); } DB::ContentAddressedMetadataStorage & operator*() const { return *storage; } @@ -222,9 +231,16 @@ CountingStoragePool openCountingStorage( const String unique = std::to_string(::getpid()) + "_" + std::to_string(counter.fetch_add(1)); const auto root = (std::filesystem::temp_directory_path() / ("cas_dir_probes_" + unique)).string(); const auto scratch = (std::filesystem::temp_directory_path() / ("cas_dir_probes_scratch_" + unique)).string(); + + /// Take directory ownership BEFORE creating anything under `root`, so a throwing `startup()` + /// below still leaves cleanup to this object's destructor. + CountingStoragePool pool(root, scratch); + std::error_code ec; std::filesystem::remove_all(root, ec); std::filesystem::create_directories(root, ec); + if (ec) + throw std::runtime_error("openCountingStorage: create_directories(" + root + ") failed: " + ec.message()); out_object_storage = std::make_shared( DB::LocalObjectStorageSettings("test", root, /*read_only_=*/false)); @@ -240,7 +256,8 @@ CountingStoragePool openCountingStorage( auto storage = std::make_shared( out_object_storage, "pool", "srv1", "", nullptr, settings); storage->startup(); - return CountingStoragePool(std::move(storage), root, scratch); + pool.attach(std::move(storage)); + return pool; } /// Publishes one part through the real transaction path: every (relative file, bytes) pair is written @@ -326,10 +343,18 @@ TEST_P(CASDirectoryProbes, PartFileAnswersFromTheViewWithoutAList) if (!disable_caches) EXPECT_EQ(os->getCount(""), 0u) << "warm view: no GET at all"; else - EXPECT_GT(os->getCount(emu_root + storage->store()->layout().casManifestsPrefix()), 0u) << "cold caches: the manifest is read, never listed"; + { + /// Nine of the ten probes above reach `getView` (all but `isDirectoryEmpty(p.proj)`, which + /// short-circuits on the `ProjectionDir` prefix without touching the view). With both caches + /// disabled, every `getView` call issues exactly one manifest `GET` and nothing else, so both + /// counts must be exactly nine, not merely positive. + const std::string manifests_prefix = emu_root + storage->store()->layout().casManifestsPrefix(); + EXPECT_EQ(os->getCount(manifests_prefix), 9u) << "cold caches: one manifest GET per probe reaching the view"; + EXPECT_EQ(os->getCount(""), 9u) << "cold caches: nothing else is GET"; + } } -INSTANTIATE_TEST_SUITE_P(Caches, CASDirectoryProbes, ::testing::Bool(), +INSTANTIATE_TEST_SUITE_P(CASCaches, CASDirectoryProbes, ::testing::Bool(), [](const ::testing::TestParamInfo & param_info) { return param_info.param ? "Disabled" : "Default"; }); /// Detached and non-Atomic parts route through the same shape and the same view. @@ -428,10 +453,23 @@ TEST(CASDirectoryProbes, FailedManifestReadPropagatesAndDoesNotList) publishPart(*storage, part, {{"columns.txt", "cols"}}); const std::string emu_root = os->getCommonKeyPrefix() + "/"; const std::string files_prefix = emu_root + filesPrefixOf(*storage, kTbl); + const std::string manifests_prefix = emu_root + storage->store()->layout().casManifestsPrefix(); - os->failReadsContaining(emu_root + storage->store()->layout().casManifestsPrefix()); + os->failReadsContaining(manifests_prefix); os->reset(); - EXPECT_THROW(storage->existsDirectory(part + "/columns.txt"), DB::Exception); + try + { + storage->existsDirectory(part + "/columns.txt"); + FAIL() << "expected the injected manifest-read failure to propagate"; + } + catch (const DB::Exception & e) + { + EXPECT_EQ(e.code(), DB::ErrorCodes::CORRUPTED_DATA); + EXPECT_NE(e.message().find(manifests_prefix), std::string::npos) << e.message(); + } + /// One attempted manifest GET, no reissue: `CORRUPTED_DATA` is a deterministic local failure, so + /// the read engine never retries it to the lease budget. + EXPECT_EQ(os->getCount(manifests_prefix), 1u); EXPECT_EQ(os->listCount(files_prefix), 0u); EXPECT_EQ(os->listCount(""), 0u); } diff --git a/tests/queries/0_stateless/05053_cas_part_file_probes_no_list.reference b/tests/queries/0_stateless/05053_cas_part_file_probes_no_list.reference index cccdd12dc23c..0ae053d4fef0 100644 --- a/tests/queries/0_stateless/05053_cas_part_file_probes_no_list.reference +++ b/tests/queries/0_stateless/05053_cas_part_file_probes_no_list.reference @@ -2,5 +2,5 @@ parts_20 20 rows_after_attach_20 20 parts_200 200 rows_after_attach_200 200 -lists_equal 1 lists_below_parts 1 +lists_equal 1 lists_below_parts 1 lists_positive 1 dropped_ok diff --git a/tests/queries/0_stateless/05053_cas_part_file_probes_no_list.sh b/tests/queries/0_stateless/05053_cas_part_file_probes_no_list.sh index 414e98f6400f..7716fdae725d 100755 --- a/tests/queries/0_stateless/05053_cas_part_file_probes_no_list.sh +++ b/tests/queries/0_stateless/05053_cas_part_file_probes_no_list.sh @@ -41,7 +41,7 @@ WITH (SELECT ProfileEvents['CASRootList'] FROM system.query_log WHERE current_database = currentDatabase() AND type = 'QueryFinish' AND query_id = '${CLICKHOUSE_DATABASE}_attach_200' ORDER BY event_time DESC LIMIT 1) AS l200 -SELECT 'lists_equal', l200 = l20, 'lists_below_parts', l200 < 20" +SELECT 'lists_equal', l200 = l20, 'lists_below_parts', l200 < 20, 'lists_positive', l20 > 0" ${CLICKHOUSE_CLIENT} -q "DROP TABLE t_20" ${CLICKHOUSE_CLIENT} -q "DROP TABLE t_200" From 58f8458c707adee7d59e1aa190e15f44ffe8f856 Mon Sep 17 00:00:00 2001 From: Mikhail Filimonov Date: Sun, 27 Sep 2026 18:20:04 +0200 Subject: [PATCH 8/8] =?UTF-8?q?docs(cas):=20read-path=20fix=20wave=20?= =?UTF-8?q?=E2=80=94=20non-Atomic=20fallback=20branch,=20LIST=20in=20inlin?= =?UTF-8?q?e=20code?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The unresolved-ref fallback sentence named only the Atomic-table branch (the table-level file listing). A non-Atomic table's unresolved probe falls back to the mirrored live-tree listing instead; say both. Also wrap `LIST` in inline code, matching every other S3 verb on this page. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01V8mZSGiD8iJumpJMiQnrmC Signed-off-by: Mikhail Filimonov --- docs/en/antalya/cas/architecture/read-path.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/en/antalya/cas/architecture/read-path.md b/docs/en/antalya/cas/architecture/read-path.md index 93e38d36a042..18e19aecacbe 100644 --- a/docs/en/antalya/cas/architecture/read-path.md +++ b/docs/en/antalya/cas/architecture/read-path.md @@ -15,8 +15,9 @@ access is served, the full chain for the common case, the two caches that sit on how a part still open inside a write transaction serves its own reads. A directory probe on a path inside a part (`
//`, which `MergeTree` issues for every checksum entry at load) is answered from the part's retained folder manifest: a plain file is not a directory, a nested -directory is and lists its children. No object-store LIST is involved; only a probe whose part does -not resolve falls back to the table-level file listing. +directory is and lists its children. No object-store `LIST` is involved; only a probe whose part +does not resolve falls back to a listing: the table-level file listing on an `Atomic` table, the +mirrored live-tree listing on a non-`Atomic` table. ## How a file access is served {#access-kinds}