Fix logical error 'current_writer != nullptr'#102215
Conversation
clickhouse-gh
Bot
added
the
pr-not-for-changelog
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
vitlibar
force-pushed
the
fix-logical-error-current-writer-null
branch
from
6def6f6 to
40d854c
Compare
There was a problem hiding this comment.
Pull request overview
Fixes an assertion-triggering logical error in MergeTreeDeduplicationLog by ensuring the current log writer is initialized before writing, addressing the crash reported in #86189 for certain object-storage disk configurations.
Changes:
- Introduce
initCurrentWriter()to lazily open the deduplication log writer when needed. - Replace
chassert(current_writer != nullptr)inaddPart()/dropPart()with writer initialization. - Refactor writer finalization/cancellation handling in
rotate()and reuse the new initializer fromload()/rotate().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/Storages/MergeTree/MergeTreeDeduplicationLog.h |
Declares initCurrentWriter() to centralize writer initialization. |
src/Storages/MergeTree/MergeTreeDeduplicationLog.cpp |
Implements initCurrentWriter() and uses it to prevent null-writer assertions during INSERT/dedup log updates. |
|
The The bug was reverted by PR #102247 (merged at ~14:48 UTC). All nessie/lakekeeper test failures between these two merges are unrelated to this PR. |
…CAL_ERROR message Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…urrent-writer-null
…licy Reproduces the storage policy shape from issue ClickHouse#86189: the deduplication log lives on the first disk of the policy (`metadata_type=plain`, which does not support writing with append), while data parts are routed to a second `local`-metadata disk. Verifies that `INSERT` with deduplication works. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Picked up this PR and pushed updates (
While validating the test I confirmed (by building Built on ARM and ran the new integration test — it passes. |
…urrent-writer-null
|
Merged Verified No unresolved review threads. This refreshes CI to clear the stale advisory status. |
…urrent-writer-null
|
Merged |
…urrent-writer-null Bring the branch up to date with master to clear unrelated red CI: the 'Fast test (arm_darwin)' failures in 01951_distributed_push_down_limit and 01952_optimize_distributed_group_by_sharding_key were a distributed query-plan regression on the old merge-base, fixed on master by the remote-plan rework in ClusterProxy/executeQuery.cpp. Master did not touch the PR's files. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…urrent-writer-null
|
Reworked the PR description to resolve the AI Review's The description now tells one consistent story matching the evidence: this is defensive hardening, not a stable-release regression fix. The Also merged |
|
Merged All four failures on the previous run were unrelated, widespread flakes (each reproduces on
No unresolved review threads; the AI Review |
|
Merged The merge is conflict-free and does not touch the PR's own files; |
LLVM Coverage ReportChanged lines: Changed C/C++ lines covered by tests: 25/33 (75.76%) | Lost baseline coverage: none · Uncovered code |
Replace the over-strict
chassert(current_writer != nullptr)inMergeTreeDeduplicationLogwith lazy initialization of the log writer (initCurrentWriter), so the writer is opened on demand fromload,addPart, anddropPart. The write mode (append or rewrite) is chosen from whether the disk's metadata storage supports appending, which keeps the deduplication log working on storage policies that mix disks with different metadata types (e.g. aplainand alocalobject-storage metadata disk, as in the linked issue).This is defensive hardening rather than a stable-release regression fix: the
current_writer != nullptrlogical error from the linked issue is no longer reproducible on currentmaster(the existingMetadataStorageType::Plainhandling inloadalready opens the writer), so there is no user-visible misbehavior in a stable release to announce — hence theNot for changelogcategory. The change removes the remaining code path through which the assertion could be reached and adds a regression test (test_merge_tree_deduplication_mixed_metadata) covering the storage-policy shape from the issue.Closes: #86189
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Not for changelog (defensive hardening: the logical error is already unreachable on current
master).