UNIQUE KEY: read and synchronous delete (4/N) by murphy-4o · Pull Request #108348 · ClickHouse/ClickHouse

murphy-4o · 2026-06-24T01:32:38Z

Adds the read + delete half of experimental UNIQUE KEY MergeTree, on the per-partition transaction layer.

Snapshot read — a query pins a per-partition CSN; per part it applies the delete bitmap with max csn ≤ pinned and sees only parts with creation_csn ≤ pinned, so a write committed mid-query is invisible. Granule-skip + row-filter drop dead rows; legacy / non-UK reads are zero-cost.
count() answers from the same per-partition snapshot path (dead rows subtracted), so it agrees with SELECT.
Synchronous DELETE — a per-partition critical section stages a 0-row marker part, installs the cumulative delete bitmap (prev ∪ matched) per touched part, and commits atomically (manifest fsync'd before sidecars; rename(tmp → active) is the visibility point), with crash-safe recovery + fail-closed on an unreconcilable rollback. Re-DELETE is idempotent.

Scope / non-goals: Local (non-replicated) only — replicated/shared is gated off at DDL; per-partition (not cross-partition) atomicity.

Changelog category (leave one):

Not for changelog (changelog entry is not required)

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

Experimental, non-replicated UNIQUE KEY MergeTree: read filtering, count(), and synchronous DELETE over the per-partition transaction layer (stack slice; gated, not yet reachable end-to-end).

Documentation entry for user-facing changes

Not applicable — experimental and gated; user docs land with the end-to-end feature.

Per-partition transaction layer for non-replicated UNIQUE KEY MergeTree. A PartitionTxnController composes three strategy interfaces — ICommitCoordinator (CSN linearization via a per-partition commit_lock), IBitmapStore (versioned per-part delete bitmaps), IPinRegistry (snapshot pins / GC floor) — behind which a future shared/replicated backend can slot in. Crash-safe commit (manifest fsync'd before sidecars; rename(tmp->active) is the atomic visibility point), fail-closed recovery + poison latch, and a monotonic per-partition CSN as the snapshot/visibility clock. Includes the MergeTreeData/StorageMergeTree/ IMergeTreeDataPart wiring (controller registry, recovery at table load, part creation_csn in the manifest) and MergeTreeBitmapStore. See Txn/README.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Scan-time delete-bitmap application for UNIQUE KEY reads. Per touched partition, takeQuerySnapshot pins a CSN C; each part is filtered to creation_csn <= C (intrinsic-manifest snapshot visibility — a part newer than the snapshot is excluded, never gets C-era bitmaps), and its bitmap (max csn <= C) drops dead rows via granule-skip + row-filter. count() answers from the same per-partition snapshot path so it agrees with SELECT. Snapshot pins ride the pipeline resources so GC can't reclaim a referenced bitmap mid-scan. Legacy/non-UK reads are zero-cost. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Synchronous DELETE for UNIQUE KEY MergeTree. A per-partition critical section stages a 0-row marker part, installs the cumulative delete bitmap (prev ∪ matched rows) per target before the rename, and commits through the transaction layer. The internal row finder rides the read filter, so already-dead rows are excluded (idempotent re-DELETE). The per-partition marker+bitmap commit is the atomic unit (not cross-partition); concurrent merges are not yet reconciled (best-effort skip + TODO(unique-key), full reconciliation deferred to merge support). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

clickhouse-gh · 2026-06-24T01:35:04Z

Workflow [PR], commit [992388c]

Summary: ❌

Performance Comparison: Performance dashboard

job_name	test_name	status	info
Integration tests (amd_asan_ubsan, db disk, old analyzer, 4/6)		FAIL
	test_s3_plain_rewritable/test.py::test[s3_plain_rewritable-data/]	FAIL	cidb
AST fuzzer (amd_debug)		FAIL
	Logical error: Block structure mismatch in A stream: different columns: (STID: 0993-27f0)	FAIL	cidb, issue

AI Review

Summary

The current PR fixes most earlier UNIQUE KEY read/delete review findings, including partitioned DELETE rejection, distributed-plan serialization, projection reads, transaction recovery, pinned snapshots, and _part_offset handling. I still see two reachable correctness holes that can resurrect deleted rows, plus one user-visible privilege regression and one missing optimized-count test.

Findings

❌ Blockers

src/Planner/findParallelReplicasQuery.cpp:50 [dismissed by author -- https://github.com/UNIQUE KEY: read and synchronous delete (4/N) #108348#discussion_r3466788429] A UNIQUE KEY table on the non-driving side of a JOIN can still be read by remote parallel replicas under parallel_replicas_prefer_local_join. The delete-bitmap filter depends on the local pinned partition snapshot, so this path can return logically deleted rows. The direct-read guards are not enough; the query needs a tree-wide UNIQUE KEY parallel-replica guard or a forced GLOBAL join.
src/Storages/MergeTree/registerStorageMergeTree.cpp:844 / src/Storages/MergeTree/MergeTreeData.cpp:4646 [dismissed by author -- https://github.com/UNIQUE KEY: read and synchronous delete (4/N) #108348#discussion_r3471725017] The storage-policy guard only rejects non-local disks, so a local policy with multiple disks or volumes is still accepted. A background move can clone and later swap an active part while a delete_bitmap_<csn>.rbm sidecar was written to the original location after the clone, leaving the swapped part without that bitmap and making deleted rows visible again. Either reject multi-disk/multi-volume policies for UNIQUE KEY for now, or make part moves preserve/reconcile bitmap sidecars transactionally.

⚠️ Majors

src/Storages/MergeTree/UniqueKey/UniqueKeyDeleteRowFinder.cpp:141 [dismissed by author -- https://github.com/UNIQUE KEY: read and synchronous delete (4/N) #108348#discussion_r3465978241] DELETE on a UNIQUE KEY table still runs the row finder as a normal internal SELECT, so the analyzer requires SELECT privilege in addition to ALTER DELETE. That changes the historical DELETE privilege contract for MergeTree: users allowed to delete but not select cannot use this path. The internal read needs to run with full access, or the limitation needs an explicit guard and user-visible decision.

💡 Nits

None.

Tests

tests/queries/0_stateless/04155_unique_key_delete_partitions_predicates.sql:58 [dismissed by author -- https://github.com/UNIQUE KEY: read and synchronous delete (4/N) #108348#discussion_r3471713685] The partition-predicate trivial-count assertion still does not exercise StorageMergeTree::totalRowsByPartitionPredicate under the analyzer, because PlannerJoinTree::applyTrivialCountIfPossible returns false for any WHERE at src/Planner/PlannerJoinTree.cpp:363. The whole-table count covers StorageMergeTree::totalRows, but the partition predicate path needs allow_experimental_analyzer = 0, an EXPLAIN, or a profile-event assertion to prove the optimized path is used.
CI report fetching for the PR latest commit showed no failed tests. I did not run local tests in this review pass.

Final Verdict

Status: ❌ Block

Minimum before merge: close the remaining parallel-replica JOIN path for UNIQUE KEY, reject or make safe local multi-disk/multi-volume movement, and decide the DELETE privilege contract before exposing this path.

Review-round fixes for the read + count + DELETE slice. Reject `DELETE ... IN PARTITION` on UNIQUE KEY tables with NOT_IMPLEMENTED until partition-scoped delete lands, and cover the optimized count() path (optimize_trivial_count_query = 1, including a partition predicate) after DELETE so it agrees with SELECT. Propagate the per-part delete bitmap through the lazy-materialization read split (LazyReadFromMergeTreeSource::splitRanges), matching the ReadFromMergeTree / PartsSplitter rebuilds, so lazy reads do not resurrect deleted rows. Consolidate the transaction test fakes and the StorageMergeTree test harness, fold the row-filter builder into DeleteBitmap::buildKeepFilter (reusing containsBulk), unify the DeleteBitmapPtr / ConstDeleteBitmapPtr aliases into UniqueKeyTxnTypes.h, and make selectLiveMarkRanges file-local. clang-tidy -Werror fixes, plus dead-code removal (unused overloads / fields) and comment trims. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Remove LocalCommitCoordinator::currentCsn, LocalPinRegistry::totalPinCount, and UniqueKeyPartitionMutex::size — no production callers (the latter two had no callers at all). The one test using currentCsn already asserts the same csn via withinSnapshotRegion; pin-RAII is observable through clusterFloor. The GC-floor seam (clusterFloor) is kept. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Resolve a PartName via the single indexed getPartIfExists(name, {Active, Outdated}) lookup (data_parts_by_info.find, O(log P)) instead of a redundant two-step: a linear scan of the partition's active parts followed by that same getPartIfExists. The scan only ever found Active parts, which getPartIfExists already covers, so it was pure overhead — the source of the O(P^2)-per-read cost. Behavior is unchanged (Active/Outdated/absent resolve identically); part names are globally unique and encode the partition, so the global index needs no partition scoping. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Remove resolution_partition_id (write-only after the resolvePart simplification) and its ctor parameter; remove the pinned_bitmap_csn field plus its read-path plumbing (it was written and copied through the splitters but never consumed — the delete_bitmap propagation beside it stays); inline the one-caller recordUniqueKeyMutexHold ProfileEvent increment into its caller and drop it from the public header. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Gate the unsafe UNIQUE KEY x feature combinations surfaced in review: - count(): disable projection-based reading including the implicit _minmax_count_projection, so count() uses the bitmap-aware path instead of physical row counts (was overcounting after DELETE under default settings). - Reject non-key column-rewrite mutations (MATERIALIZE COLUMN / CLEAR COLUMN) — they publish replacement parts without delete-bitmap sidecars, resurrecting deleted rows. - Reject DELETE inside a Replicated database (the marker/bitmap layer is local-only; replicated-DDL replay would diverge). - Reject DELETE inside an MVCC transaction (publish runs with txn = nullptr, so a ROLLBACK can't undo it). - Disable lazy-FINAL for UNIQUE KEY (ReplacingMergeTree + UNIQUE KEY) so a FINAL read uses one snapshot instead of several. - Reject the distributed query plan for UNIQUE KEY (the pinned per-partition snapshot is not serialized to workers). - Recovery now scans read-only disks and fails closed on a broken disk that may hold UNIQUE KEY tmp state, instead of silently skipping it. - Audit-log successful delete-bitmap sidecar and tmp-dir removal. Records (TODO) the read/count snapshot-window limitation — a DELETE committed between upstream part-selection and the per-partition pin is applied to an already-started SELECT/count; the boundary-pin fix is the next batch. Tests: 04162 (count() with default implicit projections; column-mutation and in-transaction rejection; lazy-FINAL correctness on ReplacingMergeTree + UK). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…EATE guard Address two clickhouse-gh review majors on the read/DELETE PR: - The DELETE row finder runs an internal `SELECT _part, _part_offset` and turns the offsets into local delete bitmaps, so it must read the local single-replica path. The cloned context inherited the user's read-routing settings, so under parallel-replica / distributed-plan routing it could collect part offsets from another replica and delete the wrong rows. Force enable_parallel_replicas=0 and make_distributed_plan=false on the cloned context (mirrors MutationsInterpreter). - runUniqueKeyTxnRecovery unlinks delete-bitmap sidecars and removes tmp_ marker dirs (not read-only), but ran before the stale-data guard. A plain CREATE over an existing data dir mutated old state, then threw INCORRECT_DATA. Move recovery after the guard so the rejection fires first; it still runs after loadDataParts so the csn-seed shared-parts lock doesn't self-deadlock. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…MATERIALIZE guard The UNIQUE KEY mutation guard now rejects MATERIALIZE COLUMN and CLEAR COLUMN on any column (they rewrite the part via the full mutation path and drop the delete-bitmap sidecars, resurrecting deleted rows). 03313 predated DELETE bitmaps: it expected ALTER_OF_COLUMN_IS_FORBIDDEN for CLEAR COLUMN on a key column and allowed MATERIALIZE/CLEAR on the non-key column d. Both now return SUPPORT_IS_DISABLED. Reference output is unchanged (the rejected ALTERs produce no rows; count stays 2). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Close the read/count snapshot-isolation window. A UNIQUE KEY SELECT/count fixed its part list at query start but pinned its per-partition CSN later (in the read filter / getDeadRowsForUniqueKey), so a DELETE committing in that gap had a csn <= the pinned C and its bitmap was applied to the already-captured (older) part list, hiding rows that were live at query start. Pin every active partition's snapshot BEFORE capturing the part list, so each pinned csn is a lower bound consistent with the parts the read then sees: - captureUniqueKeyPartitionSnapshots() enumerates all active partitions (getAllPartitionIds), instantiates + CSN-seeds each controller from on-disk bitmap state, and pins it. A partition absent from the map was created after the pin (no committed pre-snapshot DELETE) -> its parts are fully live. - SELECT: createStorageSnapshot pins before getPossiblySharedVisibleDataPartsRanges and stores the map in SnapshotData; applyUniqueKeyDeleteBitmaps reads csn/bitmap from it, sharing the pins into the pipeline-resource holder for GC-safe lifetime. - count: totalRows / totalRowsByPartitionPredicate pin before capturing parts and pass the map to getDeadRowsForUniqueKey. Non-UK paths skip the capture entirely. TODO(unique-key) kept: this holds GC pins table-wide for the query because the snapshot is taken before partition pruning; narrow once pruning can inform it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…set through PREWHERE Address two clickhouse-gh review majors on the read path: - M4: a non-replicated UNIQUE KEY read could execute on another replica that has no local marker/bitmap state (deleted rows reappear / counts overcount). Disable parallel-replica reads for UNIQUE KEY tables at every eligibility gate: StorageMergeTree::read (the three PR branches), parallelReplicasEnabledForStorage, and findParallelReplicasQuery (isTableNodeEligibleForParallelReplicas + the View-over-MergeTree branch). A UK table on the non-driving side of a JOIN is a known remaining gap (TODO). - M5: the post-PREWHERE delete-bitmap filter is keyed on _part_offset, but PREWHERE can consume and remove that column, silently skipping the filter and returning deleted rows. Retain _part_offset through PREWHERE for UNIQUE KEY reads with a bitmap (getReadTaskColumns, UK-gated), and fail closed in MergeTreeSelectProcessor: throw if a non-empty bitmap is attached but _part_offset is missing or malformed, instead of silently bypassing. Test: 04163 (PREWHERE referencing _part_offset on a UK table after DELETE keeps the deleted rows filtered; discriminates the fix). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

WithinSnapshotRegionIsAtomicWithCommit relied on the OS scheduling the reader thread during the writer's commit loop. Under slow instrumentation (msan / coverage) the reader could be starved until after the writer drained, so EXPECT_GT(reads, 0) flaked — the lock-coupling invariant itself never failed (tsan / asan_ubsan stay green). Have the writer wait until the reader has taken at least one observation before committing, so the commits provably overlap a live reader and reads > 0 holds without depending on thread scheduling; reads is now atomic. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Relocate the per-partition writer mutex from the free-standing UniqueKeyPartitionMutex class (+ its MergeTreeData registry) into LocalCommitCoordinator, exposed via ICommitCoordinator::lockForWrite(). The controller forwards it; UniqueKeyDelete holds the guard across the whole write statement. No behavior change — the lock is relocated, not removed. The writer mutex (OUTER) serializes whole DELETE statements per partition so two writers can't both read prev bitmaps before either publishes (lost update); commit_lock (INNER) still linearizes publish + CSN bump and the reader snapshot. Readers take only commit_lock. This puts the Local-only serialization primitive in the Local strategy and gives a future Shared coordinator (CAS retry) a seam that returns an empty guard. Removes UniqueKeyPartitionMutex.{h,cpp} and MergeTreeData::getOrCreatePartitionMutex. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

The row finder narrowed _part_offset to UInt32 and threw LOGICAL_ERROR for a part with more than 2^32 rows. DeleteBitmap already upgrades its roaring representation to 64-bit, so carry the row id as UInt64 end to end and drop the guard. Leaves a TODO noting that the internal row-finder SELECT runs under the caller's access, so a DELETE currently also requires SELECT (the analyzer checks it unconditionally); preserving the historical ALTER DELETE-without-SELECT contract needs the internal read to run with full access, as background mutation reads do. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

createReadTasksForTextIndex rebuilt the StorageSnapshot with the 2-arg ctor, dropping its data (the per-partition snapshot pins). A text-search read on a UNIQUE KEY table then applied no delete bitmaps and returned rows a prior DELETE had marked dead. Move the existing data into the rebuilt snapshot. Test 04165 pins the text-index direct-read path and discriminates the regression. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Ordinary INSERT parts are written via the plain MergeTreeSink and carry no creation_csn, so they reach the visibility predicate as always-visible. Document that a post-pin INSERT is therefore visible (standard MergeTree snapshot-at-query-start) and that full INSERT isolation lands when INSERT publishes through the txn layer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…d of moving it createReadTasksForTextIndex rebuilt the StorageSnapshot by moving `data` out of it, but that StorageSnapshot is shared (query_info and the select executor still hold it) and its `data` is dereferenced elsewhere on the read path (e.g. MergeTreeDataSelectExecutor) — emptying it segfaulted the server on any text-index read. Clone the SnapshotData instead: copy the shared-ptr fields and .share() the per-partition pins, leaving the shared original intact while the rebuilt snapshot still carries the pins. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…n latch On a DELETE commit rollback double-fault (a rollback removeBitmap itself throws, stranding an orphaned delete_bitmap_<csn>.rbm on an active target part), drop the in-memory partition_state_torn latch in favor of quarantining the broken part the way DETACH/DROP PART do: PartitionTxnController records the orphan-bearing targets (takeOrphanedTargets), and the DELETE executor retires each via removePartsFromWorkingSet (-> Outdated, deferred refcount-gated removal; never renames live files) plus a best-effort clone into detached/. Once Outdated the part leaves the active set, so csn-seed can't surface the orphan -- closing the reload hole the in-memory latch left open. Residual durability-tail (crash between Outdated and physical removal) noted as a TODO. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

clickhouse-gh · 2026-06-25T13:07:08Z

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	85.40%	85.40%	+0.00%
Functions	92.60%	92.60%	+0.00%
Branches	77.60%	77.60%	+0.00%

Changed lines: Changed C/C++ lines covered by tests: 1768/1907 (92.71%) | Lost baseline coverage (was covered on master, now uncovered in this PR): 73 line(s) · Uncovered code

Full report · Diff report

alesapin

TBH I'm really afraid to have alternative transactions implementation specifically for UniqueKey. I would try to pursue any possibility to reuse existing experimental transactions infrastructure which we have now: #24258.

cc @tuanpach @tavplubix

alesapin · 2026-07-08T13:02:07Z

+        /// (`MergeTreeData::SnapshotData::uk_partition_snapshots`). Null on
+        /// estimation / without-data paths ⇒ every partition reads as fully
+        /// live. See `applyUniqueKeyDeleteBitmaps`.
+        const std::unordered_map<String, UniqueKeyTxn::QuerySnapshot> * uk_partition_snapshots = nullptr);


Let's use alias for the map type.

alesapin · 2026-07-08T13:02:35Z

    void addResources(const QueryPlanResourceHolder & resources_) { resources.append(resources_); }
    void setQueryIdHolder(std::shared_ptr<QueryIdHolder> query_id_holder) { resources.query_id_holders.emplace_back(std::move(query_id_holder)); }
    void addContext(ContextPtr context) { resources.interpreter_context.emplace_back(std::move(context)); }
+    void addCustomResource(std::shared_ptr<ICustomResourceHolder> resource) { resources.custom_resources.emplace_back(std::move(resource)); }


Too generic name, what is resource?

alesapin · 2026-07-08T13:43:56Z

+void MergeTreeData::runUniqueKeyTxnRecovery()
+{
+    /// SMT delegates to Keeper; this Local-only path enumerates `tmp_<op>_*`
+    /// dirs at the table's data root, groups by partition_id, and drives


too sloppy comments

alesapin · 2026-07-08T13:46:50Z

 }

+MergeTreeData::DataPartsVectorPtr
+MergeTreeData::getActivePartsInPartitionShared(const String & partition_id) const


Shared with whom?

alesapin · 2026-07-08T13:48:41Z

+    return std::make_shared<const DataPartsVector>(std::move(parts));
+}
+
+UniqueKeyTxn::PartitionTxnController & MergeTreeData::getOrCreateTxnController(const String & partition_id) const


It's very confusing. We already have transactions support for MergeTree, also we have class MergeTreeTransaction which is responsible for parts rename on disk. And now it's third type of transactions. Need to change naming/make it more hidden for MergeTreeData code.

alesapin · 2026-07-08T15:44:07Z

+    /// Callers staging for the publish lock pass `creation_csn = INVALID_CSN`;
+    /// the commit driver rewrites the manifest with the real csn at the
+    /// linearization point. Durability ordering is owned by `UniqueKeyManifest::write`.
+    UniqueKeyManifest::write(new_data_part->getDataPartStorage(), meta);


The part is not temporary anymore. Is it safe to write new file into this part? do we do it with write ->temp-> rename pattern?

alesapin · 2026-07-08T15:46:26Z

+///
+/// `bitmaps_created` are the `(target, csn)` bitmaps OWNED by this commit;
+/// recovery unlinks them on abort (one entry per touched old part).
+/// `forwarded` are `(target, csn)` pairs REFERENCED but not owned — schema-only


I don't understand what forwarded means from this description :(

alesapin · 2026-07-08T15:50:07Z

+
+/// Part name (e.g. `all_1_1_0`). Stable identifier of a MergeTree part within
+/// its partition; the manifest's bitmap targets are part names.
+using PartName = String;


But why?) And if we need it, why not in MergeTreePartInfo?

alesapin · 2026-07-08T15:57:58Z

+        for (const auto & entry : parts_in_partition)
+        {
+            auto part = storage.getActiveContainingPart(entry.part_name, parts_lock);
+            if (!part)


How it can be? The only case is DROP PART/PARTITION. Do we handle it here?

alesapin · 2026-07-08T15:59:41Z

+            /// post-resolution merge (merges don't take the writer mutex) is a
+            /// silent under-delete until merge-side late-kill lands; re-running
+            /// the DELETE clears the survivors.
+            if (part->name != entry.part_name)


Sorry, but it doesn't look like it works at all. Any concurrent merge will make DELETE for affected rows noop. There is no retry or locking.

clickhouse-gh · 2026-08-10T13:56:40Z

Dear @alesapin, you haven't been active on this PR for 30 days. You will be unassigned. Will you continue working on it? If so, please feel free to reassign yourself.

murphy-4o and others added 3 commits June 23, 2026 23:25

clickhouse-gh Bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Jun 24, 2026