Record the storage column type when a mutation rewrites the whole part - #112501
Conversation
`MutationHelpers::getColumnsForNewDataPart` took the type of every column absent from the mutation's `updated_header` from the source part, with the comment "It may differ from column type in storage". That is right for `MutateSomePartColumnsTask`, which hardlinks those columns byte for byte, but wrong for `MutateAllPartColumnsTask`: it re-reads every column through the interpreter pipeline, which produces it at the current type in storage. So the new part recorded the stale type while its data had the new one. The part writer takes the serialization from the column list and the block from the pipeline, so the very first write handed a `ColumnNullable` to `SerializationString` and threw `Bad cast from type DB::ColumnNullable to DB::ColumnString` in `initColumnsSubstreamsIfNeeded` - an exception in the release build, an abort in debug and sanitizer builds. A part carries a type older than the metadata whenever an `ALTER TABLE ... MODIFY COLUMN` was not applied to its data, which happens both when the part is detached across the `ALTER` and when the type change is left to be applied on read. A later mutation that rewrites the whole part then aborts. The reported run hit it through `ALTER TABLE ... MATERIALIZE PROJECTION`: the projection's required columns are only read, so they stay out of `updated_header`, while the `_block_number` materialization that `enable_block_number_column` adds and the automatic statistics of `auto_statistics_types` together make the mutation rewrite all columns. Decide "rewrites every column" once, in `MutationHelpers::rewritesAllPartColumns`, and use it both for the column list and for the task selection, so the two cannot drift apart. CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?REF=master&sha=09f2aea4ab4c9f6c0b85369f9fb3feec15f9bd4a&name_0=MasterCI&name_1=Stateless%20tests%20%28amd_msan%2C%20WasmEdge%2C%20parallel%2C%202%2F2%29 Closes: #112459 Related: #112417 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Workflow [PR], commit [a378342] Summary: ✅
AI ReviewSummaryThis PR makes full-part mutation rewrites use the same Final Verdict✅ Approve. LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 48/51 (94.12%) · Uncovered code |
clickhouse-gh
Bot
added
the
pr-bugfix
…list-type-mismatch
|
The only CI failure, |
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
…hen a mutation rewrites the whole part The column list of the new part now records the storage type for an untouched column when the mutation rewrites every column, but its `SerializationInfo` was still carried over from the source part, where it describes the stale type. `IMergeTreeDataPart::setColumns` passes that info to `getSerialization` of the type from the column list, and `DataTypeTuple` `assert_cast`s it to `SerializationInfoTuple` and indexes one element info per tuple element. A part detached across `ALTER TABLE ... MODIFY COLUMN t Tuple(x String) -> Tuple(x String, y String)` therefore read past the end of that element list on the next full-part rewrite: an out-of-bounds access in the release build, a libc++ hardening assertion in debug and sanitizer builds. The info is now rebuilt at the storage type in that case, the same way it is for the columns the mutation writes, and the new test covers the tuple scenario.
…list-type-mismatch
…alizationInfoTuple::createWithType` `SerializationInfoTuple::createWithType` builds the element infos from the new type but carried the element names of the old one over. `structureEquals` ignores names, so a same-arity rename of a tuple element - `Tuple(String)` to `Tuple(y String)`, for example - reached that path and produced an info whose elements are keyed by the names the part no longer has. Everything that merges tuple subinfos matches them by name: `SerializationInfoTuple::replaceData`, which the mutation uses to fold in the data actually written, and `SerializationInfoTuple::add`, which both the per-table serialization hints and a merge use to aggregate the source parts. A renamed element therefore looked missing and contributed all-default rows, so a merge after a full-part rewrite picked the sparse serialization for a column without a single default value. Both callers of `createWithType` produce an info for the new type, so taking the names from it is the only consistent choice. Added a fourth case to `04653_mutation_rewrite_stale_part_column_type`: without the fix the merged part stores `t.y` as `Sparse` although all 105 of its rows are non-default.
…list-type-mismatch
|
🕵 The CI reds on
@groeneai, please investigate the two stateless failures above and provide a fix in a separate PR; if a fix is already in progress, link it here. |
|
Both investigated. One already has a merged fix (yours); the other I could not attribute to a product defect, and I found a separate real harness defect while looking.
The failing shape was
From the expect debuglog: the client answered Separate finding, real and mechanical: Every one of them sets Truncation is visibly happening in this very run: expect's match buffer starts at the sent command, then later restarts mid-table around row 8. It is not the cause of the failure above, because expect discards the front of the buffer and the prompt arrives last, which I confirmed by replaying a 9.4KB burst ending in the prompt 25 times at each limit (25/25 matched either way). So this is a latent defect that weakens the harness rather than an explanation for the red. Happy to send that one-word-per-file |
LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 40/41 (97.56%) · Uncovered code |
…list-type-mismatch
… too `getColumnsForNewDataPart` has three carriers of the source part's column type for a column that is not in `updated_header`: the column the mutation renames into this name, the stale name a renamed column still has in the source part, and the same-named column. Only the last one was guarded by `rewrites_all_columns`, so a full-part rewrite of a stale part whose column was renamed after the type change still recorded the source part's type. Reproduced: detach a part, `MODIFY COLUMN b Nullable(String)`, attach it, `RENAME COLUMN b TO d` - the part then carries `d` as `String` while the table says `Nullable(String)`. A following full-part rewrite reads `d` through the rename branch and the new part claimed `String` for a `ColumnNullable`, so the writer threw `Bad cast from type DB::ColumnNullable to DB::ColumnString`. Added a fifth case to `04653_mutation_rewrite_stale_part_column_type`.
diegomestre2
left a comment
diegomestre2
left a comment
There was a problem hiding this comment.
The implementation fixes the Bad cast from DB::ColumnNullable to DB::ColumnString do to an issue between the metadata and the data parts after an alter table. I would consider just a reduction on the amount of comments but this can be done later, for now the main issue is addressed. Not sure how this would apply when two replicas have to do this change in parallel but maybe the tests are already covered.
robot-ch-test-poll
added
the
pr-synced-to-cloud
| continue; | ||
| } | ||
|
|
||
| new_serialization_infos.emplace(new_name, old_info); |
There was a problem hiding this comment.
emplace(new_name, old_info) hands the new part the same SerializationInfo object the source part owns, and the full-rewrite finalizer then mutates it in place.
SHould we clone old_info instead to prevent this? Because in some cases both parts point at the same object and the type guard in updateSerializationHintsForPart passes for both, so the two operations cancel exactly: the new part's numbers are never added and the source part's original numbers are never removed.
There was a problem hiding this comment.
Yes, clone it. The aliasing you describe is real and I confirmed the exact intersection by pointer identity, not by reading.
I instrumented three sites on master (0eb5eba20b9) and logged the SerializationInfo addresses: the emplace(new_name, old_info) here, the serialization_infos.replaceData(new_serialization_infos) in MergedBlockOutputStream::fillChecksums, and the add/remove in updateSerializationHintsForPart. Case 5 of 04653_mutation_rewrite_stale_part_column_type reaches all three on one object:
ALIAS_TAKEN part=all_3_3_0_4 col=a ptr=0x781b6ab07018 rewrites_all=true
MBOS_REPLACEDATA (pre) part=all_3_3_0_5 col=a ptr=0x781b6ab07018 nrows=0 ndef=0
MBOS_REPLACEDATA (post)part=all_3_3_0_5 col=a ptr=0x781b6ab07018 nrows=1 ndef=0
HINT_ADD part=all_3_3_0_5 col=a ptr=0x781b6ab07018 nrows=1 ndef=0
HINT_REMOVE part=all_3_3_0_4 col=a ptr=0x781b6ab07018 nrows=1 ndef=0
So the source part and the new part do hold the same object, the full-rewrite finalizer does mutate it in place, and both hint operations then read the post-mutation state. The add for the new part and the remove for the source part cancel exactly, and the source part's original contribution is never subtracted, which is what you said.
What I could not do is turn that into a wrong serialization_hint. In every shape I got to the alias line with rewrites_all_columns, the two counts were equal anyway, so the cancellation was a no-op. I tried 23 shapes (partial and full rewrites; wide, compact and packed storage; row TTL and column TTL; RENAME/CLEAR/DROP/MODIFY COLUMN; MATERIALIZE COLUMN/INDEX/PROJECTION/TTL; APPLY PATCHES; lightweight and ALTER DELETE), and the ones that change a column's row count or defaults inside a full rewrite all pull the column into updated_header, so they take the branch above instead of this one. My oracle was the in-memory hint against the same hint rebuilt from the on-disk parts by DETACH/ATTACH; it agreed in all 23.
So I have no user-visible repro, but the invariant is broken and the in-place write is real, so the arithmetic is only correct by accident of the counts matching. old_info->clone() is one allocation per untouched column on a path that is already rewriting the whole part, and it makes the two parts independent regardless of what a future path does to the counts. I would clone.
5 participants
MutationHelpers::getColumnsForNewDataParttook the type of every column absent from the mutation'supdated_headerfrom the source part, with the comment "It may differ from column type in storage". That is right forMutateSomePartColumnsTask, which hardlinks those columns byte for byte, but wrong forMutateAllPartColumnsTask: it re-reads every column through the interpreter pipeline, which produces it at the current type in storage.So the new part recorded the stale type while its data had the new one. The part writer takes the serialization from the column list and the block from the pipeline, so the very first write handed a
ColumnNullabletoSerializationStringand threwBad cast from type DB::ColumnNullable to DB::ColumnStringinMergeTreeDataPartWriterOnDisk::initColumnsSubstreamsIfNeeded- an exception in the release build, an abort in debug and sanitizer builds.A part carries a type older than the metadata whenever an
ALTER TABLE ... MODIFY COLUMNwas not applied to its data, which happens both when the part is detached across theALTERand when the type change is left to be applied on read. A later mutation that rewrites the whole part then aborts. The reported run hit it throughALTER TABLE ... MATERIALIZE PROJECTIONin03522_alter_modify_column_and_materialize_projection: the projection's required columns are only read, so they stay out ofupdated_header, while the_block_numbermaterialization thatenable_block_number_columnadds and the automatic statistics ofauto_statistics_typestogether make the mutation rewrite all columns."Rewrites every column" is now decided once, in
MutationHelpers::rewritesAllPartColumns, and used both for the column list and for the task selection, so the two cannot drift apart.Verified locally against a release build of
master: the new test and03522_alter_modify_column_and_materialize_projectionabort with the exact stack trace from the report before the change and pass after it; 319 tests matchingmutat,projection,alter_modifyandmaterialize_columnpass, and every remaining failure in that selection is a missing local prerequisite (no Keeper, no s3, transactions disabled, no macros).CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?REF=master&sha=09f2aea4ab4c9f6c0b85369f9fb3feec15f9bd4a&name_0=MasterCI&name_1=Stateless%20tests%20%28amd_msan%2C%20WasmEdge%2C%20parallel%2C%202%2F2%29
Closes: #112459
Related: #112417
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fix
Bad cast from type DB::ColumnNullable to DB::ColumnStringwhen a mutation that rewrites the whole part runs on a part whose column type is older than the one in the table metadata, for example afterALTER TABLE ... MODIFY COLUMNfollowed byALTER TABLE ... MATERIALIZE PROJECTION.