Skip to content

Conversation

@filimonov
Copy link
Member

@filimonov filimonov commented Jan 28, 2026

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in an official stable release)

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

Make zero‑copy cleanup check the correct directory before it is moved, so shared blobs aren’t deleted while other replicas still need them ( regression after ClickHouse#94262 ).

Documentation entry for user-facing changes

(I hope) fixes #1338

What went wrong

A data part is removed in two steps:

  1. The part directory is renamed to delete_tmp_*.
  2. Then the system decides whether it can delete the shared blobs (for zero‑copy) and clears the files.

In PR ClickHouse#94262, we stopped updating the internal path (part_dir) after step 1 to fix a data race with system.parts.

That was correct for the race, but it had a side effect:

  • The zero‑copy cleanup logic checks a small file (references) inside the part directory to decide whether it’s safe to delete shared blobs.
  • After the rename, the directory has moved, but part_dir still points to the old location, so the check fails.
  • When the check fails, the code assumes “this is a temporary part” and deletes blobs without asking ZooKeeper.

Result: a replica can delete blobs that other replicas still need, causing fetch failures and permanent missing parts.

What the fix does

Before we rename the directory, we now evaluate the zero‑copy delete decision and keep the result.

That way:

Why 94262 introduced the regression

94262 removed part_dir updates during removal to fix a data race with system.parts.
That means any logic that depends on part_dir after the directory is renamed will now read a stale path.

Zero‑copy cleanup is exactly such logic.

Why only zero‑copy is affected

Non‑zero‑copy disks don’t use refcount files or ZooKeeper locks.
The cleanup simply deletes files and never checks shared blob ownership.

So only zero‑copy paths care about that missing references file.

CI/CD Options

Exclude tests:

  • Fast test
  • Integration Tests
  • Stateless tests
  • Stateful tests
  • Performance tests
  • All with ASAN
  • All with TSAN
  • All with MSAN
  • All with UBSAN
  • All with Coverage
  • All with Aarch64
  • All Regression
  • Disable CI Cache

Regression jobs to run:

  • Fast suites (mostly <1h)
  • Aggregate Functions (2h)
  • Alter (1.5h)
  • Benchmark (30m)
  • ClickHouse Keeper (1h)
  • Iceberg (2h)
  • LDAP (1h)
  • Parquet (1.5h)
  • RBAC (1.5h)
  • SSL Server (1h)
  • S3 (2h)
  • Tiered Storage (2h)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants