Skip to content

CAS: answer directory probes inside a part from the part manifest, no LIST per part file - #2440

Merged
filimonov merged 8 commits into
antalya-26.6from
fix/antalya-26.6/cas-part-file-probes-no-list
Sep 30, 2026
Merged

filimonov merged 8 commits into
antalya-26.6from
fix/antalya-26.6/cas-part-file-probes-no-list

Conversation

@filimonov

Copy link
Copy Markdown
Member

On a cas disk, a directory probe on a path inside a part (<table>/<part>/<file>) was routed to the table-subdirectory branch, which runs an S3 LIST of the table's _files/ prefix. MergeTree makes this probe for every checksum entry of every part while it loads (MergeTreeDataPartChecksum::checkSize), so one restart of a 1,672-part server issued 77k LISTs in three minutes, got 537 503 Slow Down and failed 139 uploads.

This PR gives such a path its own shape, PartFile, and answers it from the part's folder manifest: a plain file is not a directory, a nested directory is. No LIST is issued when the part resolves. When it does not resolve (a table-level subdirectory that happens to look like a part), the old branch runs unchanged.

Measured on ATTACH TABLE: CASRootList was 105 for 20 parts and 1005 for 200 parts; it is now 5 for both.

One visible change: a nested non-projection directory inside a part now reports present, its children and non-empty. Before, it reported absent and empty. Projection directories keep their answers.

Tests: routing cases in gtest_ca_wiring, a new gtest_cas_directory_probes (zero LIST for resolved parts, exact one-LIST oracle for unresolved paths, a failed manifest read throws and never lists, a 50-part load profile) and the stateless test 05053 (DETACH/ATTACH of 20 and 200 parts, equal LIST counts read from the ATTACH query's own ProfileEvents).

Closes: #2439

Changelog category (leave one):

  • Performance Improvement

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

cas disk: loading a table no longer issues one object-store LIST per part file. Directory probes inside a part are answered from the part manifest, so a restart costs a fixed number of LISTs per table instead of one per file.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

filimonov and others added 8 commits September 27, 2026 21:46
`classifyDirectory` gains `DirShape::PartFile` for `<table>/<part>/<file>`
(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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V8mZSGiD8iJumpJMiQnrmC
Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
…der view

`existsDirectory`/`listDirectory` on `<table>/<part>/<file>` 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V8mZSGiD8iJumpJMiQnrmC
Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
…rt-file probes

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V8mZSGiD8iJumpJMiQnrmC
Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V8mZSGiD8iJumpJMiQnrmC
Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V8mZSGiD8iJumpJMiQnrmC
Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
…ifest

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V8mZSGiD8iJumpJMiQnrmC
Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
…n three test oracles, fix the gtest gate filter

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: #2439

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V8mZSGiD8iJumpJMiQnrmC
Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
…nline code

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V8mZSGiD8iJumpJMiQnrmC
Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
@filimonov filimonov added the CAS label Sep 27, 2026
@github-actions

Copy link
Copy Markdown

Workflow [PR], commit [58f8458]

filimonov added a commit to filimonov/ClickHouse that referenced this pull request Sep 27, 2026
Related: Altinity#2440

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V8mZSGiD8iJumpJMiQnrmC
@filimonov filimonov mentioned this pull request Sep 27, 2026
68 tasks
@alsugiliazova

Copy link
Copy Markdown
Member

PR #2440 Distributed-Systems Audit

AI audit note: This review was generated by AI (GPT-5.6 Sol).

Confirmed defects

Timing / ordering

Medium: A post-repoint probe can use the previous manifest

Impact. existsDirectory() or listDirectory() can return stale results after a ref has been repointed. A removed nested directory may still appear, or a newly added directory may appear absent.

Anchor.

  • src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/ContentAddressedMetadataStorage.cpp
    • ContentAddressedMetadataStorage::existsDirectory, PartFile branch at line 1752
    • ContentAddressedMetadataStorage::listDirectory, PartFile branch at line 1943
  • src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Parts/PartFolderAccess.cpp
    • CachedPartFolderAccess::getView, lines 154–216
    • CachedPartFolderAccess::buildView, lines 235–274

Minimal trigger.

  1. Reader A resolves ref R to manifest M1, becomes the cold-build leader, and stalls while reading M1.
  2. A writer repoints R to M2 and clears the retained view.
  3. Reader B starts after the repoint and resolves R to M2.
  4. Reader B joins Reader A's in-flight future because the single-flight map is keyed only by PartRefKey::cacheKey().
  5. Reader A completes. Reader B returns and retains the M1 view without comparing it with its resolved M2 ID.

Why this is a defect. Retained-cache hits compare cached->manifestId() with the caller's resolved manifest ID. The single-flight follower path returns future.get() without the equivalent validation. Cache invalidation during the repoint does not cancel or generation-fence an older in-flight build.

Fix direction. Key in-flight builds by (PartRefKey, ManifestId), or compare the completed view with resolved->manifest_id and rebuild on mismatch.

Regression test direction. Block the M1 manifest GET, repoint the ref to M2, start a second directory probe, release M1, and assert that the second probe observes only M2.

Limits

This was a static review of PR head 58f8458c707. No runtime reproduction was performed, and no review comment was posted to GitHub.

@filimonov

filimonov commented Sep 30, 2026 •

Copy link
Copy Markdown
Member Author

PR #2440 Distributed-Systems Audit

The mechanics are as described: a follower of a cold CachedForLoad build in CachedPartFolderAccess::buildView returns the leader's view without comparing its manifestId with the id the follower resolved. The warm path in getView does compare.

It is not introduced by this PR, so I am not changing it here:

  • The single-flight path came with the Parts layer (9d08bc0). This PR does not touch Parts/; it adds two getView(..., CachedForLoad) call sites next to about fifteen existing ones.
  • CachedForLoad is documented as stale-tolerant. Reads that need freshness use ForceFresh, which does not coalesce.
  • The exposure is the calls that join one in-flight manifest GET. The next access sees the id mismatch and rebuilds.
  • For the two probes this PR adds (existsDirectory / listDirectory inside a part) the answer differs only if an in-place repoint changes the directory set of a committed part.

The follower check is cheap (compare and rebuild on mismatch) and is tracked separately, together with the question whether a writer can read its own in-place change through a coalesced build.

@filimonov filimonov closed this Sep 30, 2026
@filimonov filimonov reopened this Sep 30, 2026
@filimonov
filimonov merged commit e2dd0f5 into antalya-26.6 Sep 30, 2026
822 of 863 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CAS: existsDirectory on a part file and listDirectory on a table dir issue an S3 LIST; a restart makes 77k LISTs and gets 503 Slow Down

4 participants