Fix LOGICAL_ERROR when loading an existing Compact part under a non-adaptive table policy - #111626
Conversation
…daptive policy
canUsePolymorphicParts() is equivalent to canUseAdaptiveGranularity(). When a table's
current settings make it false (non-adaptive granularity), the part LOAD path rejected an
already-existing Compact part in two places, both deriving the part's validity from the
table's current create-time policy rather than the part's physical on-disk reality:
1. MergeTreeDataPartBuilder::build() threw "... does not support polymorphic parts".
2. Even without (1), the IMergeTreeDataPart constructor initializes index_granularity_info
from the current policy, building MarkType(adaptive=false, Compact), which throws
"Non-Wide data part type with non-adaptive granularity".
But a Compact part is always adaptive by invariant (see the MarkType constructor), and an
existing on-disk part's type is a fact, not a choice. The current policy only governs the
format chosen for NEW parts, which MergeTreeData::choosePartFormat() already enforces
(it returns {Wide, Full} when polymorphic parts are disabled). In debug and sanitizer
builds the exception aborts the server.
Reproduced by, for example, a read-only table that shares an on-disk path with another
table and reloads its parts via SYSTEM RESTART DISK or on server startup. Found by the
Stress test (amd_msan) check, recurring across several unrelated PRs and check types.
Fix:
- MergeTreeIndexGranularityInfo(storage, settings, type): a Compact part type now always
yields an adaptive mark type, regardless of the table's current policy; Wide keeps its
policy-derived adaptivity. This covers the load path and mutations that inherit a
Compact source part's format.
- MergeTreeDataPartBuilder::build(): removed the redundant polymorphic-parts guard.
New-part format selection is enforced by choosePartFormat; the type reaching build()
otherwise belongs to an existing part where it is a physical fact.
New-part creation behavior is unchanged. Added regression test
04371_load_compact_part_non_adaptive_reader.sh.
Since a corrupt part that looks Compact is now caught while its columns are read (and
detached as broken) instead of aborting at construction, the debug/sanitizer skip in the
existing test_merge_tree_load_parts_filesystem_error is no longer needed and was removed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Internal second-model review — adjudication log (click to expand)Pre-publication review by an independent model (engine: codex; 2 findings across 1 full pass + 1 delta re-review; bounded).
Severity: ❌ blocker / Session id: cron:clickhouse-author-slot-1:20260723-131900 |
Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-author-slot-1:20260723-131900 |
|
cc @hanfei1991 @CheSema — could you review this? It fixes a |
PedroTadim
added
the
can be tested
|
Workflow [PR], commit [77456c9] Summary: ❌
AI ReviewSummaryThis PR fixes the Findings
Final VerdictChanges requested: the readonly shared-disk load fix is reasonable, but the same relaxation must not make |
clickhouse-gh
Bot
added
the
pr-bugfix
…r check The new regression test 04371 configures the reader table with index_granularity_bytes = 0 (so canUsePolymorphicParts() is false and the Compact-part load path is exercised) but left min_bytes_for_wide_part at its default 10485760. On a non-adaptive table a nonzero wide-part threshold makes MergeTreeData log a "settings will be ignored" <Warning> at CREATE. The Fast test job runs .sh tests with --send_logs_level=warning, so that warning reached the client stderr and tripped the empty-stderr check (Reason: having stderror), even though the query results were correct and the fix under test worked. Set min_bytes_for_wide_part = min_rows_for_wide_part = 0 on the reader table. These thresholds are meaningless on a non-adaptive table (that is exactly what the warning says), and zeroing them keeps canUsePolymorphicParts() false via index_granularity_bytes = 0, so the load-path scenario the PR fixes is still reproduced. No source change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CI finish ledger — 416ca73CI fully finished (Config + Finish Workflow SUCCESS, >5h buffer). The earlier PR-caused
Session id: cron:our-pr-ci-monitor:20260724-063000 |
The regression test only drove the part-load path through SYSTEM RESTART DISK.
The same LOGICAL_ERROR is reachable from the background refresh task, and that
form is what CI actually hits: in the Stress reds the abort comes from a
BackgroundSchedulePool thread via
MergeTreeDataPartBuilder::build (MergeTreeDataPartBuilder.cpp:79)
<- MergeTreeData::loadDataPart (MergeTreeData.cpp:2487)
<- MergeTreeData::loadDataPartWithRetries (MergeTreeData.cpp:2594)
<- MergeTreeData::refreshDataPartsOnce (MergeTreeData.cpp:3191)
<- MergeTreeData::refreshDataParts (MergeTreeData.cpp:3112)
rather than from a client query, so it aborts the server instead of failing a
statement. Add a second reader on the same plain_rewritable path that carries
refresh_parts_interval and no explicit restart. It is created before the insert
so the part arrives after the initial load and the refresh task is what loads
it. The reader disk stays readonly, both because refreshDataPartsOnce requires
it and because the task is only started when all disks are readonly.
The new reader keeps min_bytes_for_wide_part = 0 and min_rows_for_wide_part = 0
next to index_granularity_bytes = 0 for the same reason as the existing one: on
a non-adaptive table a nonzero threshold logs a "settings will be ignored"
warning at CREATE, which trips the Fast test empty-stderr check.
Verified both directions on a debug build. Against current master, the refresh
shape alone aborts the server with the signature above; with the fix the part
loads and the server stays up. 50 consecutive runs across 5 parallel copies pass.
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
| /// No polymorphic-parts policy check here: it governs only the format of NEW parts and is | ||
| /// enforced by MergeTreeData::choosePartFormat. The type reaching build() otherwise belongs to | ||
| /// an existing part (loaded from disk, or inherited by a mutation), where it is a physical fact. |
There was a problem hiding this comment.
This assumption is too broad for the restore path. loadPartRestoredFromBackup also comes through withPartFormatFromDisk(), and StorageMergeTree::attachRestoredParts never runs canReplacePartition(), so RESTORE ... SETTINGS allow_different_table_def = 1 can now attach a backup Compact part into a writable index_granularity_bytes = 0 table.
Once that happens, follow-up DELETE / REWRITE_PARTS mutations keep part->getFormat() (StorageMergeTree.cpp:1972) but recompute granularity from the target table settings (MutateTask.cpp:2345-2350, MergeTreeDataPartWriterOnDisk.cpp:106-112). In computeIndexGranularity(), index_granularity_bytes = 0 on the adaptive path normalizes every mark to 1 row (MergeTreeIndexGranularity.cpp:142-157), so the restored part mutates into a pathological one-row-granule Compact part.
So this change does not only relax the readonly refresh/restart scenario from the PR description; it also opens a writable state that regresses post-restore mutations badly. We need to keep rejecting this combination on restore/attach (or harden the mutation path) before removing the build-time policy check here.
There was a problem hiding this comment.
The mechanism is right and I reproduced it, but the conclusion that this change opens the regression is not: the collapse pre-exists this PR, and the fix already exists at the funnel you name in #111694.
I measured four things on a debug build, server buildId() matched against the ELF on every run.
1. Your scenario on this PR's head (Build ID 1cbae537) behaves exactly as you describe: RESTORE ... allow_different_table_def = 1 lands a Compact part on the writable index_granularity_bytes = 0 table, then ALTER TABLE ... DELETE rewrites it to 9999 rows with 10000 marks (data correct, 9999 / 49995000).
2. The same scenario on unmodified master (snapshot whose MergeTreeDataPartBuilder.cpp, MergeTreeIndexGranularityInfo.cpp and MergeTreeIndexGranularity.cpp are md5-identical to origin/master) does not reach the mutation at all. It aborts inside the restore with the signature this PR fixes:
Logical error: 'Cannot create part with type Compact and storage type Full because table does not support polymorphic parts'
MergeTreeDataPartBuilder.cpp:79 build()
MergeTreeData.cpp:8358 loadPartRestoredFromBackup(...)
MergeTreeData.cpp:8331 restorePartFromBackup(...)
MergeTreeData.cpp:8267 restorePartsFromBackup(...)
In a release build that exception is caught by loadPartRestoredFromBackup's own catch (...) and routed to mark_broken, so the restored part is detached as broken-from-backup and the data is silently dropped. That is the current behaviour of the state you want to keep rejecting.
3. The collapse is pre-existing and is not caused by the Compact route. On that same unmodified master snapshot I ran the identical restore with a Wide adaptive part (min_bytes_for_wide_part = 0, .mrk2 marks): RESTORED, then the mutation gives 9999 rows with 10000 marks, no abort, no <Fatal>. So an adaptive part already reaches a writable index_granularity_bytes = 0 table through RESTORE today and already mutates into one-row granules. canReplacePartition never runs on the restore path on master either. My change adds one more part type to a funnel that is already wrong; it does not create the defect, and the guard I remove is not what was protecting it.
4. #111694's guard alone covers both routes, which is why no extra restore/attach check is needed. I applied only its src/ hunk on top of this head and rebuilt (Build ID moved 1cbae537 to c08a5c57):
| restored part | without the guard | with #111694's guard |
|---|---|---|
Compact (your scenario) |
9999 rows / 10000 marks | 9999 rows / 2 marks |
Wide (pre-existing on master) |
9999 rows / 10000 marks | 9999 rows / 3 marks |
Reverting the hunk and rebuilding restores 10000 marks, so the guard is causal. It suffices for every entry point because MergeTreeDataPartWriterCompact.cpp:266 and MergeTreeDataPartWriterWide.cpp:339 both go through MergeTreeDataPartWriterOnDisk::computeIndexGranularity at :106 into DB::computeIndexGranularity, where that guard sits; the only other callers of it are unit tests. However the part arrived, whether by load, restore, attach, mutation inherit or a replicated merge taking the format from the log entry, the granularity is computed at that one place.
So I am keeping this PR to the load-path logical error and leaving the granularity computation to #111694, which is deliberately based on master because the defect is reachable there without this PR. I have noted the Compact-via-RESTORE route on that PR so its test matrix covers it alongside the Wide one it currently uses.
LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 10/10 (100.00%) · Uncovered code |
CI finish ledger - 77456c9Every failure below has an owner: a fixing PR (mine or external), or a full-effort fix task
Both failures are unrelated to this diff, which only touches part-loading granularity handling and its stateless test. Session id: cron:our-pr-ci-monitor:20260728-033000 |
|
Fixing PR for The ledger line above attributed this to a loss of
The actual mechanism is session loss, not database loss. #111990 removes the ping before every query, so a slow server no longer costs the client its Nothing here is caused by this pull request. |
4b34059
robot-ch-test-poll4
added
the
pr-synced-to-cloud
The test only exercised the Wide route. The Compact route reaches the same granularity computation and was raised twice by an automated reviewer on the sibling PR ClickHouse#111626, which is now merged and no longer aborts the restore of a Compact part onto a table with a non-adaptive granularity policy, so the route became testable. Measured on this branch, both routes reproduce the defect without the fix and are correct with it: after RESTORE and a row-changing mutation of a 10000-row part, marks go 10000 -> 3 (Wide) and 10000 -> 2 (Compact). Also drop the test's references to C++ internals and to the fix itself, and one source comment line that narrated the defect rather than stating the invariant.
|
This fix is missing on the active release branches, and 26.7 aborts on it in CI. Requesting the What happens:
The port is clean and complete. Asking only for 26.7, which is where the abort was observed. |
|
Hi @alexey-milovidov, gentle ping on the request above: could you apply |
4 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fixed a
LOGICAL_ERROR(Cannot create part with type Compact and storage type Full because table does not support polymorphic parts) raised when aMergeTreetable loads a pre-existingCompactpart while its current settings disable polymorphic parts (non-adaptive granularity). This could happen, for example, when a read-only table shares an on-disk path with another table and reloads its parts viaSYSTEM RESTART DISKor on server startup. In debug and sanitizer builds the exception aborts the server.Description
Found by the
Stress test (amd_msan)check on an unrelated PR (STID 3286-0cd6), with the same error recurring across several unrelated PRs and check types (Stress msan/tsan/asan, Integration msan/tsan) over the last two months. No tracking issue existed.Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=109360&sha=3791a7836dd7b2f089113ba6666a14b6f9c26492&name_0=PR&name_1=Stress%20test%20%28amd_msan%29
Root cause.
MergeTreeData::canUsePolymorphicParts()is equivalent tocanUseAdaptiveGranularity(). When a table's current settings make itfalse(non-adaptive granularity), the part LOAD path rejected an already-existingCompactpart in two places, both deriving the part's validity from the table's current create-time policy rather than the part's physical on-disk reality:MergeTreeDataPartBuilder::build()threw the... does not support polymorphic partsLOGICAL_ERRORdirectly.IMergeTreeDataPartconstructor initializesindex_granularity_infofrom the current policy, buildingMarkType(adaptive=false, Compact), which throwsNon-Wide data part type with non-adaptive granularity.But a
Compactpart is always adaptive by invariant (see theMarkTypeconstructor), and an existing on-disk part's type is a fact, not a choice. The current policy only governs the format chosen for NEW parts, whichMergeTreeData::choosePartFormat()already enforces (it returns{Wide, Full}when polymorphic parts are disabled).Fix.
MergeTreeIndexGranularityInfo(storage, settings, type): aCompactpart type now always yields an adaptive mark type, regardless of the table's current policy.Widekeeps its policy-derived adaptivity (aWidepart may legitimately be non-adaptive). This covers both the load path and mutations that inherit aCompactsource part's format.MergeTreeDataPartBuilder::build(): removed the redundant polymorphic-parts guard. New-part format selection is enforced bychoosePartFormat; the type reachingbuild()otherwise belongs to an existing part (read from disk, or inherited by a mutation), where it is a physical fact.New-part creation behavior is unchanged. The change only makes ClickHouse load pre-existing
Compactparts it currently rejects.Added regression test
04371_load_compact_part_non_adaptive_reader.sh(a non-adaptive reader shares aplain_rewritablepath with an adaptive writer and loads the writer'sCompactpart viaSYSTEM RESTART DISK). It aborts the server without the fix and passes with it.