Workflow [PR], commit [37da0b8]

Summary: ✅

• Performance Comparison: Performance dashboard

AI Review

Summary

This PR adds a borrow_from_cache object storage backed by filesystem cache segments, plus a memory metadata storage intended for ephemeral table data. The current PR head addresses the previously unresolved truncateFile(path, 0) behavior, and I did not find a new correctness issue that needs a fresh inline comment. One existing policy concern remains dismissed by the author: the new storage backend is still exposed without an experimental gate.

Missing context / blind spots

• ⚠️ The local checkout could not be updated to 37da0b847b2e79e1b74a0ce5c427fe175c14b15e because .git/FETCH_HEAD is read-only; this review used the GitHub PR diff and raw PR-head files downloaded under tmp/pr_100371_head.
• ⚠️ I did not run the new stateless or integration tests locally; CI results would close the runtime coverage gap for the cache lifecycle and restart scenarios.

Findings

❌ Blockers

• [src/Disks/DiskObjectStorage/ObjectStorages/ObjectStorageFactory.cpp:285] [dismissed by author -- https://github.com/Add borrow_from_cache object storage and memory metadata types #100371#discussion_r2991373474] The PR introduces a new production-visible object storage backend and an in-memory metadata backend without an allow_experimental_* gate. Per the ClickHouse review rule for genuinely new storage/data-loss-sensitive features, this should fail closed behind an experimental setting until the backend has enough production mileage; otherwise users can enable a new data placement path with no rollout guard. The current code still registers borrow_from_cache unconditionally, so the original concern still holds despite the author's dismissal.

Final Verdict

❌ Not ready as-is while the dismissed experimental-gate concern remains unresolved.

Pushed 4f931588c42:

• Merged master (the branch was ~1134 commits behind). The merge was conflict-free (git merge-tree reported no conflicts) and also reconciled the contrib/avro submodule pointer that master had advanced to 7b7f6a5.
• Fixed the Style check failure (test_numbers_check: Gap (4355, 5017) > 100). The regression test 05017_borrow_from_cache_part_modification_time had an anomalously high number, while the highest stateless test number on master is 04356. check_gaps_in_tests_numbers (ci/jobs/check_style.py) flags gaps of 100 or more between consecutive test numbers. Renumbered it to 04357 (the next available number) and updated the in-test cache_name/disk/path identifiers to match. The gap check now passes locally (no gaps >= 100).

Verified that the 1134-commit merge did not break the new code: BorrowFromCacheObjectStorage.cpp, MetadataStorageInMemory.cpp, MetadataStorageFactory.cpp, ObjectStorageFactory.cpp, DiskType.cpp, and gtest_metadata_in_memory.cpp all pass -fsyntax-only clean against current-master headers.

The only remaining unresolved review item (an allow_experimental_* gate for borrow_from_cache) reflects the deliberate design decision already stated on this PR — it is intended to be production, not experimental — and is left as-is. The remaining blocker is the human review (@kssenii).

Pushed bea54b1e8fd:

• Addressed the AI Review "Request changes" (the sole remaining unresolved thread, docs/en/operations/storing-data.md:49): the overview stated unconditionally that metadata_type defaults to local, which contradicts the new borrow_from_cache backend. MetadataStorageFactory::getCompatibilityMetadataTypeHint returns memory for ObjectStorageType::BorrowFromCache (and local for the durable object storages, web for web), and 04141_borrow_from_cache_metadata_check relies on omitting metadata_type defaulting to memory.

  Qualified the wording so the default is stated per object storage type: local for durable object storages (s3, azure_blob_storage, local_blob_storage, hdfs), web for the web object storage, and memory for borrow_from_cache. The following sentence already documents that borrow_from_cache requires metadata_type='memory' and that any other combination is rejected with INVALID_CONFIG_PARAMETER.

CI for the previous commit 4dfb2e8e is green (150 passed, 0 failed). This is a docs-only wording change; the behavior is unchanged and already covered by 04141_borrow_from_cache_metadata_check.

Pushed 5afd10a60b4:

• Addressed the AI Review "Request changes" (the remaining unresolved thread on BorrowFromCacheObjectStorage.cpp). HoldingReadBuffer kept the borrowed FileSegmentsHolder alive but, as a bare ReadBufferFromFileDecorator, forwarded only getFileOffsetOfBufferEnd. The borrowed cache file is plugged into ReadBufferFromRemoteFSGather (and, on the async path, AsynchronousBoundedReadBuffer) exactly like the local_blob_storage read wrapper ReadBufferFromFileWithLogging. Made the decorator transparent by forwarding the same set of methods — setReadUntilPosition, setReadUntilEnd, supportsRightBoundedReads, supportsReadAt, readBigAt, isSeekCheap, isContentCached — so the gather's right bound (set when a requested range ends inside an object) is honored and positional readBigAt reads of the borrowed object are enabled (the inner pread buffer reports supportsReadAt() == true), while still holding the segment alive.

• Added 04489_borrow_from_cache_bounded_read: a bounded sub-range read of a borrowed object with the synchronous read path forced (remote_filesystem_read_method = 'read') so the gather buffer is used directly and an over-read cannot be trimmed by AsynchronousBoundedReadBuffer.

The changed TU compiles cleanly with -Werror; the test reference was validated with clickhouse-local (the borrowed-disk backend does not change SELECT results). The disk DDL mirrors 04054_borrow_from_cache_object_storage.

Pushed 271d32a965d (two commits):

• Fail-close: disable zero-copy replication for memory metadata storage (2431d73da95). DiskObjectStorage::supportZeroCopyReplication previously returned true for every metadata type except Keeper, so it advertised zero-copy support for the new memory metadata. That metadata is in-memory and ephemeral — MetadataStorageInMemory inherits IMetadataStorage::getSerializedMetadata (throws NOT_IMPLEMENTED), and for borrow_from_cache the bytes live in node-local cache segments other replicas cannot read. Now it also excludes MetadataStorageType::Memory. All zero-copy call sites (getZookeeperZeroCopyLockPaths, getZeroCopyPartPath, tryCreateZeroCopyExclusiveLock, …) gate on supportZeroCopyReplication and simply skip disks that do not support it, so this falls back to ordinary replication rather than erroring late.

• Test 04490_borrow_from_cache_replicated_zero_copy (271d32a965d). A ReplicatedMergeTree on a borrow_from_cache/memory disk with allow_remote_fs_zero_copy_replication = 1 must work end-to-end via ordinary replication. Validated against a local server with an embedded Keeper: insert + OPTIMIZE … FINAL + read succeed, the single active part lands on the borrowed disk, and no zero_copy ZooKeeper root is created. The existing 04054/04141/04357/04489 tests still pass with the change.

This addresses the zero-copy item of the AI Review "Request changes". The other item — gating borrow_from_cache behind an allow_experimental_* setting — is intentionally not done: this backend is meant to ship as production (e.g. temporary tables with custom engines in the Cloud without S3), not as an experimental feature, as already noted on the corresponding thread. The feature already fails closed on misconfiguration (borrow_from_cache requires metadata_type = 'memory', and memory metadata is rejected for durable object storages, both with INVALID_CONFIG_PARAMETER).

Root cause of the Stress test (arm_msan) failure (report, commit 271d32a9).

The sole CI failure (Cannot start clickhouse-server) is a real, PR-introduced restart-robustness gap — not a flaky-unrelated failure. It hit only 1 of 10 stress configs (arm_msan; the other nine are green), which points to a load-order race, but the underlying defect is genuine.

What happens. The stress harness hard-restarts the server while stateless tests are mid-flight. If the server is killed after 04141_borrow_from_cache_metadata_check runs CREATE TABLE tmp_borrowed ... disk = disk(type = object_storage, object_storage_type = 'borrow_from_cache', cache_name = '04141_cache_creator', ...) but before its DROP, the table's .sql metadata persists. On the next startup the server re-attaches it, which reconstructs the borrow_from_cache disk, which calls FileCacheFactory::get('04141_cache_creator') and throws There is no cache by name ... (code 36). The metadata-load job fails fatally, so the server cannot start.

The named cache is registered by a separate custom disk (tmp_cache_creator, type = cache), and custom DDL disks are materialized lazily per table — so the borrow disk can be reconstructed before the cache-creating disk has registered the cache. That ordering nondeterminism is why only one shard tripped.

This is inherent to a persistent table on a borrow_from_cache disk: its data is ephemeral by design (the in-memory entries map and memory metadata are empty after restart), so the reloaded table is necessarily empty — but today the disk reconstruction aborts the whole server instead of coming up empty.

no-stress is not an escape hatch: tests/clickhouse-test rejects that tag ("stress tests must be able to run every test"), so the server itself must tolerate this on restart.

Three points that require the cache on reattach (all currently throw when it is absent):

1. object-storage construction — registerBorrowFromCacheObjectStorage → FileCacheFactory::get (the throw seen in the log);
2. the disk access check — IDisk::checkAccess writes a probe blob → BorrowFromCacheObjectStorage::writeObject → needs the cache;
3. MergeTreeData's format_version.txt write on attach: with memory metadata the file is always absent after restart, so !read_format_version is true and it writes it — again writeObject → needs the cache.

Suggested fix. DiskFromAST/DiskFactory already thread an attach flag down into the object_storage disk creator in RegisterDiskObjectStorage.cpp (currently received as the ignored first trailing bool). Use it:

• Make BorrowFromCacheObjectStorage resolve the cache lazily — it is only ever used in writeObject — so construction no longer requires the cache and point (1) stops throwing on reattach. A fresh CREATE with a bad cache_name still fails fast, just at the access check instead of at construction.
• When attach == true and the named cache is absent (!FileCacheFactory::instance().getAll().contains(cache_name)), bring the disk up degraded so the empty table loads and the server starts: skip the access check and skip the format_version.txt write. Both already key off isReadOnly, so a read-only disk is the simplest lever — but ReadOnlyDiskWrapper blocks removals, so a leftover table could not then be DROPped; a write-once/broken-style flag that still permits removal is preferable. That is the one design call here.

I did not push a patch: this worker's checkout is in an unrelated dirty state and a full rebuild is not feasible in a single session, so the read-only-vs-degraded choice and a local restart check (create a borrow table, kill -9, restart) are best done where it can be built and tested.

Everything else is green and the AI review is ✅ Approve with no unresolved threads, so the remaining blockers are this restart fix and @kssenii's review.

Pushed 47b883e8915 + d41b102f2a8, implementing the fix for the restart-robustness gap analyzed above (the Stress test (arm_msan) → Cannot start clickhouse-server).

Fix. The borrow_from_cache object storage no longer resolves its cache eagerly at construction (which called FileCacheFactory::get and threw There is no cache by name ..., aborting metadata loading). It now resolves the cache lazily, by name, on first use, and reports isReadOnly while the cache is not registered. Read-only disks already skip every attach-time write that would otherwise need the cache — the access check in IDisk::startup, directory creation and the format_version.txt write in MergeTreeData, and cleanup in MergeTreeData::dropAllData are all gated on isReadOnly — so the (necessarily empty) reattached table loads cleanly, the server starts, and the leftover table can be queried and dropped. The disk becomes writable again once the cache is registered.

A fresh CREATE still fails loudly when cache_name does not exist: the attach flag (already threaded through DiskFactory) is now forwarded to ObjectStorageFactory::create and the object-storage creators, and the missing cache is rejected with BAD_ARGUMENTS only when attach is false. Added a non-throwing FileCacheFactory::tryGet.

Verified locally against a freshly-built server with a real stop/restart cycle:

• With the referenced cache genuinely absent on restart (table .sql rewritten to a never-created cache_name, and separately via a removed config cache disk): the server starts, system.disks.is_read_only = 1 for the borrowed disk, SELECT count() returns 0 (empty), INSERT is rejected with TABLE_IS_PERMANENTLY_READ_ONLY, and DROP TABLE succeeds.
• A fresh CREATE with a missing cache_name is rejected with Filesystem cache '...' not found for borrow_from_cache.
• Existing stateless tests 04054, 04357, 04489 still pass, and the 04141 rejections (metadata_type='local', durable + memory) still fire with INVALID_CONFIG_PARAMETER.

Regression test. Added tests/integration/test_borrow_from_cache_restart, which reproduces the scenario deterministically (config-defined cache disk removed before restart_clickhouse) and asserts the server starts, the table is empty and droppable, the disk is read-only, writes are rejected, and a fresh CREATE against a missing cache fails.

Merged master and pushed 7b0dff7c177.

The branch was ~1070 commits behind and had gone CONFLICTING, which is why the restart-fix head d41b102f2a8 never got a CI run — GitHub could not build the merge ref. The push flips the PR back to MERGEABLE and a fresh PR workflow is now running on the merged head, so the arm_msan → Cannot start clickhouse-server restart gap fixed by 47b883e8915/d41b102f2a8 finally gets exercised in CI.

Conflict (one file, MetadataStorageFactory.cpp): master added registerMetadataStorageFromIndexPages (metadata type web_index) while this PR added registerMetadataStorageInMemory (metadata type memory) in the same two spots — the function definitions and the registerMetadataStorages body. Resolved by keeping both function definitions and both registration calls; the includes for both MetadataStorageInMemory.h and MetadataStorageFromIndexPages.h were already reconciled by the textual merge.

Verified locally: the 9 translation units touched by the merge — BorrowFromCacheObjectStorage.cpp, MetadataStorageInMemory.cpp, MetadataStorageFactory.cpp, ObjectStorageFactory.cpp, RegisterDiskObjectStorage.cpp, DiskType.cpp, FileCacheFactory.cpp, DistributedPlanExecutor.cpp, and gtest_metadata_in_memory.cpp — all compile clean with zero errors and zero warnings under -Weverything against current-master headers (which reorganized the metadata storages into Web//Memory/ subdirectories). std::make_shared<MetadataStorageInMemory> compiling confirms the class still fully implements the current IMetadataStorage interface after the merge.

AI Review is ✅ Approve with no unresolved threads; the remaining blocker is the human review (@kssenii).

Pushed 37da0b847b2:

Addressed the AI Review "Request changes" — the sole remaining unresolved thread, MetadataStorageInMemory.cpp:901 (truncateFile missing-file contract).

MetadataStorageInMemoryTransaction::truncateFile(path, 0) threw FILE_DOESNT_EXIST before checking target_size, so the public IDisk::truncateFile operation failed only for metadata_type='memory' / borrow_from_cache. The disk-backed MetadataStorageFromDisk::TruncateMetadataFileOperation treats zero-truncation of a missing file as a no-op and errors only for a non-zero target (MetadataStorageFromDiskTransactionOperations.cpp:568-574), and the public DiskObjectStorageTest.TruncateFileToZero (gtest_disk_object_storage.cpp:715) relies on truncateFile(missing, 0) succeeding.

Fix mirrors the disk-backed path: return when the file is missing and target_size == 0; throw only when a missing file is truncated to a non-zero size.

Added the focused regression test the review asked for — MetadataInMemoryTest.TestTruncateMissingFileToZero (gtest_metadata_in_memory.cpp) — covering the missing-file zero-truncation no-op, the missing-file non-zero-truncation error, and zero-truncation of an existing file (file kept, blobs dropped).

Both changed translation units compile clean with -fsyntax-only under -Weverything against current-master headers; the gtest is exercised end-to-end by the Unit tests job.

The other AI Review item — gating borrow_from_cache behind an allow_experimental_* setting — remains intentionally not done (recorded as author-dismissed on the corresponding thread); this backend is meant to ship as production and already fails closed on misconfiguration.

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered: 633/878 (72.10%) · Uncovered code

Full report · Diff report

@Michicosun, temporary tables in the cloud where we don't have local storage.