Skip uncommitted parts in non-transactional TRUNCATE#109598
Conversation
A non-transactional TRUNCATE covers all active parts, including a part whose creating transaction has not committed yet (for a non-transactional observer `isVisible` only checks `removal_tid.isEmpty()`, not `creation_csn`). Covering such a part set its `removal_csn` to `NonTransactionalCSN` while `creation_csn` was still unknown, discarding a write that may still commit and tripping a `LOGICAL_ERROR` in `VersionMetadata::validateInfo`. Filter the covered parts to only those already committed at `Tx::MaxCommittedCSN`, leaving in-flight parts for their own transaction to commit or roll back.
|
Workflow [PR], commit [2fddd21] Summary: ✅
AI ReviewSummaryThis PR makes non-transactional Findings❌ Blockers
Final Verdict
|
clickhouse-gh
Bot
added
the
pr-bugfix
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
|
|
||
| /// Do not remove parts whose creating transaction has not committed yet: | ||
| /// removing such a part would discard a write that may still commit. | ||
| std::erase_if(parts, [](const auto & part) { return !part->version->isVisible(Tx::MaxCommittedCSN, Tx::EmptyTID); }); |
There was a problem hiding this comment.
This only hardens TRUNCATE, but the same invalid state is still reachable from the other non-transactional removal paths. dropPart (src/Storages/StorageMergeTree.cpp:2686-2707), dropPartition (src/Storages/StorageMergeTree.cpp:2796-2827), movePartitionToTable source cleanup (src/Storages/StorageMergeTree.cpp:3265-3286), and the non-transactional REPLACE PARTITION cleanup (src/Storages/StorageMergeTree.cpp:3118-3119 via src/Storages/MergeTree/MergeTreeData.cpp:5812-5910) still feed unfiltered Active parts into the same removal machinery. That machinery writes removal_csn = NonTransactionalCSN immediately for txn == nullptr (src/Interpreters/MergeTreeTransaction.cpp:137-142), so an in-flight insert can still end up as creation_csn == 0 && removal_csn == 1 and trip VersionMetadata::validateInfo.
I think this needs to be fixed at a shared "committed parts only for non-transactional destructive DDL" layer rather than patching only this TRUNCATE call site.
There was a problem hiding this comment.
The finding is correct — dropPart, dropPartition, the REPLACE PARTITION cleanup via removePartsInRangeFromWorkingSet, and the MOVE PARTITION TO TABLE source cleanup can reach the same creation_csn == 0 && removal_csn == 1 state.
The narrow scope here is intentional: this PR is an alternative to #109537. #109537 relaxes validateInfo to accept that state, fixing all non-transactional destructive-DDL paths at the shared layer but weakening the invariant. This PR takes the opposite trade-off — keep the invariant and stop feeding uncommitted parts into non-transactional destructive DDL, starting with the TRUNCATE reproducer. Whether to extend the call-site filter to the sibling paths or adopt the shared-layer approach of #109537 is a design choice between the two PRs, so mixing both into one change is out of scope here.
When transactions are not in use, every part is already committed and the filter is a no-op that still locks each part's version mutex. Skip it using the per-table `transactions_enabled` flag, mirroring the existing gate for beginning a transaction on merges.
…moved
Address review feedback on the regression test: the previous version only
exercised `TRUNCATE` on an otherwise empty table, so it would also pass if the
new `transactions_enabled` branch accidentally turned `TRUNCATE` into a no-op.
Each case now seeds an already-committed row before `TRUNCATE` and asserts that
it disappears while the in-flight (uncommitted) part is skipped. This proves the
advertised contract ("skip uncommitted parts") without weakening ordinary
`TRUNCATE` semantics: if `TRUNCATE` removed nothing, the committed row would
survive and the assertions would fail.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The only CI failure on the latest run, Stateless tests (amd_tsan, flaky check) → "Start ClickHouse Server", is unrelated to this PR. The server aborted at startup on a pre-existing TSan data race in |
LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 5/5 (100.00%) · Uncovered code |
robot-ch-test-poll3
added
the
pr-synced-to-cloud
4 participants
A non-transactional
TRUNCATEcovers all active parts, including a part whose creating transaction has not committed yet (for a non-transactional observerisVisibleonly checksremoval_tid.isEmpty(), notcreation_csn). Covering such a part set itsremoval_csntoNonTransactionalCSNwhilecreation_csnwas still unknown, which discarded a write that may still commit and tripped aLOGICAL_ERRORinVersionMetadata::validateInfo:This changes the non-transactional
TRUNCATEto only remove parts whose creation is already committed (visible atTx::MaxCommittedCSN), leaving in-flight parts for their own transaction to commit or roll back. This is an alternative to #109537, which instead relaxesvalidateInfoto accept the state.Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fixed a
LOGICAL_ERROR(creation_csn is not set while removal_csn is set to 1) that could be thrown when a non-transactionalTRUNCATEran concurrently with an uncommitted transaction that had inserted into the same table. Such aTRUNCATEno longer removes parts created by not-yet-committed transactions.Documentation entry for user-facing changes