Conversation
…alysis A pending ALTER MODIFY COLUMN that changes a column type leaves old parts with on-disk skip-index data serialized using the old type. Index analysis (MergeTreeDataSelectExecutor::canUseIndex) skips such an index for a part by checking the column against AlterConversions::getAllUpdatedColumns(), which is populated from the read snapshot's READ_COLUMN (alter) mutations. createStorageSnapshot built the read snapshot with need_alter_mutations = apply_mutations_on_fly || apply_patch_parts so when both settings were off the pending READ_COLUMN mutation was dropped from the snapshot, getAllUpdatedColumns() returned empty, canUseIndex did not exclude the type-incompatible index, and filterMarksUsingIndex deserialized the old index granule with the new type. For a String to Nullable(UInt64) change this aborted the server with a LOGICAL_ERROR: "Sizes of nested column and null map of Nullable column are not equal after deserialization". The implicit count() projection path (optimizeUseAggregateProjections) reaches the index read during planning, so even an EXPLAIN or a plain SELECT count() crashes. apply_mutations_on_fly (UPDATE/DELETE on the fly) and apply_patch_parts (lightweight updates) are unrelated to column type changes. Visibility of pending column type changes is required for correct skip-index analysis regardless of those flags, so the read snapshot now always requests alter mutations. Column data reads were already correct: IMergeTreeReader converts to the requested type from the part's own stored type, independent of the snapshot. The change only widens what the default path already did (apply_patch_parts defaults to true, which already set need_alter_mutations), so it has no effect on the common case; it only fixes the both-off case that serverfuzz exercised. This is the same crash family addressed by the data-read-phase guard in supportsSkipIndexesOnDataRead; that guard does not cover the primary-key analysis phase, which relies on canUseIndex. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Pre-PR validation gate
Session id: cron:clickhouse-worker-slot-6:20260610-141800 |
|
cc @CurtizJ @alesapin — could you review this? The query read snapshot only requested alter (READ_COLUMN) mutations when |
PedroTadim
added
the
can be tested
|
Workflow [PR], commit [137b450] Summary: ❌
AI ReviewSummaryThis PR fixes the missing pending- Final Verdict✅ No new findings. LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 31/33 (93.94%) · Uncovered code |
clickhouse-gh
Bot
added
the
pr-bugfix
…index analysis Request alter mutations in the read snapshot only when a pending READ_COLUMN mutation actually exists (num_alter > 0), instead of unconditionally. Reads without a pending ALTER MODIFY COLUMN keep hitting the empty-snapshot fast path in getMutationsSnapshot rather than taking the mutation lock and scanning current_mutations_by_version. In the replicated queue, initialize seen_all_data_mutations from need_data_mutations only: alter and metadata mutations carry alter_version != -1 and are handled by the metadata branch, so a snapshot that needs only alter visibility no longer walks data-mutation entries. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Pre-PR validation gate (updated for the fast-path fix, HEAD
|
| # | Question | Answer |
|---|---|---|
| a | Deterministic repro? | Yes. Built the base commit fc73f46 (pre-fix) and ran the new test against it: the server aborts every time with Logical error: 'Sizes of nested column and null map of Nullable column are not equal after deserialization (null map size = 1, nested column size = 0)' via SerializationNullable::deserializeBinaryBulkWithMultipleStreams -> MergeTreeIndexSet::deserializeBinary -> filterMarksUsingIndex. Repro is the test SQL: ALTER ... MODIFY COLUMN value Nullable(UInt64) with alter_sync=0, mutations_sync=0, then SELECT count() ... WHERE value=300 SETTINGS apply_mutations_on_fly=0, apply_patch_parts=0. |
| b | Root cause explained? | Yes. createStorageSnapshot set need_alter_mutations = apply_mutations_on_fly || apply_patch_parts. Both off => the pending READ_COLUMN alter mutation is dropped from the read snapshot => AlterConversions::getAllUpdatedColumns() is empty => canUseIndex does not exclude the type-incompatible skip index => filterMarksUsingIndex deserializes the old String-serialized SET-index granule as Nullable(UInt64) (null map size 1, nested size 0) => LOGICAL_ERROR. |
| c | Fix matches root cause? | Yes, and it now preserves the no-alter fast path per the AI Major. need_alter_mutations is requested when a pending READ_COLUMN mutation actually exists, gated on getMutationCounters().num_alter > 0 (a cheap 3-int read), instead of unconditionally true. Reads with no pending column-type change keep need_data_mutations=false && need_alter_mutations=false, so getMutationsSnapshot still hits the empty-snapshot fast path (no mutation lock, no current_mutations_by_version scan). The replicated queue initializes seen_all_data_mutations from need_data_mutations only, so an alter-only snapshot no longer walks data-mutation entries (alter/metadata mutations carry alter_version != -1 and use the metadata branch). |
| d | Test intent preserved / new tests added? | Yes. New regression test 04151_skip_index_after_alter_modify_column_nullable.sql (unchanged from prior HEAD) pins apply_mutations_on_fly=0, apply_patch_parts=0 for a String -> Nullable(UInt64) change with a pending mutation, and checks the result with both flags off, with defaults, and after materialization. |
| e | Both directions demonstrated? | Yes. Base binary (pre-fix): server aborts with the LOGICAL_ERROR above. Fixed binary (HEAD 6e4dd00): the test prints 1/1/1 matching .reference and the server stays alive. Additionally verified the fast path is preserved: a read with apply_patch_parts=0 and a pending UPDATE (data mutation, num_alter=0) returns the correct result with the server alive. Sibling tests 03702_alter_column_modify_secondary_index_general, 03702_alter_column_update_and_delete_secondary_index_general, 03702_alter_codec_index pass and match references. |
| f | Fix is general, not a narrow patch? | Yes. The fix is at snapshot construction, not a guard at the crash site, so it covers every skip-index type read with an incompatible on-disk type after any pending ALTER MODIFY COLUMN. The gating uses the existing num_alter counter (already maintained for all alter mutations), and the symmetric replicated read path is handled too. The write/merge snapshots (MergeTask uses !patch_parts.empty(), MutateTask uses true) were reviewed and are unaffected; column data reads use the part's own stored type in IMergeTreeReader, independent of the snapshot. |
Session id: cron:clickhouse-worker-slot-14:20260610-200400
|
Thanks for the review. Addressed both points in Major (preserve the no-alter fast path). For the replicated queue, Metadata. Reworded the changelog entry to "Fix a Verified locally: the new test crashes the pre-fix binary with the documented LOGICAL_ERROR and passes on the fixed binary; a read with |
|
CI finished on HEAD
Fast test, Style check, and Bugfix validation are green on this HEAD. |
CI summary, covered HEAD
|
| Check | Failure | Classification | Disposition |
|---|---|---|---|
| Stateless tests (arm_asan_ubsan, azure, sequential) | Segmentation fault (STID 0883-65ab), then Server died |
Inherited trunk corruption (multistage distributed-queries thread-teardown UAF, STID 0883-* family) |
Not PR-caused. This HEAD predates the master revert #107122 (merged 01:37 UTC 06-11), so it inherited the now-fixed corruption. The arm_asan_ubsan, azure, sequential check is clean on master across 55 commits since 03:00 UTC 06-11 (0 failures); the same pre-revert window shows this family on 16+ unrelated PRs as singletons. A rebase past 01:37 UTC 06-11 clears it. |
| Integration tests (amd_tsan, 1/6) | test_refreshable_mat_view_replicated::test_circular_dependencies_survive_restart |
Chronic flaky (38 hits / 33 distinct PRs / 3 master in 30d) | Not PR-caused (this PR only touches MergeTreeData::createStorageSnapshot). Tracked under the chronic refreshable-MV task (issue #106651). |
No PR-caused failures. The fix (skip-index over a column with a pending ALTER MODIFY COLUMN type change; regression test 04151) is unaffected.
|
Fresh independent P0 reproduction of this bug surfaced on an unrelated PR's
I verified this PR's change fixes it on current master HEAD ( CREATE TABLE t (id UInt64, value String, INDEX idx (value) TYPE set(0) GRANULARITY 1)
ENGINE = MergeTree ORDER BY id SETTINGS index_granularity = 8, min_bytes_for_wide_part = 0;
INSERT INTO t SELECT number, toString(number * 1000000000) FROM numbers(128);
SYSTEM STOP MERGES t;
ALTER TABLE t MODIFY COLUMN value Nullable(UInt64); -- alter_sync = 0, mutations_sync = 0
SELECT count() FROM t WHERE value = 300
SETTINGS force_data_skipping_indices = 'idx', use_skip_indexes_on_data_read = 1,
max_rows_to_read = 0, apply_mutations_on_fly = 0, apply_patch_parts = 0;
One note for a rebase: master's |
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
The fix gates `need_alter_mutations` on a pending ALTER MODIFY COLUMN, and that snapshot flag has two consumers: the planning-time exact-ranges path (`filterMarksUsingIndex`) and `ReadFromMergeTree::supportsSkipIndexesOnDataRead`, which decides whether skip indexes are applied during the data read. The test only covered the first, so a future change could reintroduce the abort in `MergeTreeSkipIndexReader::read` with the test still green. Verified both directions on a debug build. Reverting only the `has_alter_mutations` term from the gate and rebuilding (Build ID ba9b530f -> dbbf2576) makes the new query abort on its own with "Sizes of nested column and null map of Nullable column are not equal after deserialization", through MergeTreeIndexBulkGranulesSet::deserializeBinary <- MergeTreeSkipIndexReader::read; the pre-existing planning-time query was removed for that run so the new row is proven load-bearing rather than carried by an earlier assertion. With the fix restored (Build ID back to ba9b530f) the query returns the correct row. `max_rows_to_read = 0` is required because the data-read phase disables itself when clickhouse-test injects `read_overflow_mode = throw` together with a row limit, the same reason 04143 sets it. Every setting the row depends on is pinned at statement level, so runner randomization of `use_skip_indexes_on_data_read` cannot flip it.
`force_data_skipping_indices` does not throw when skip indexes are disabled globally: measured on a debug build, the new direct-read row still returns the correct row with `use_skip_indexes = 0`, so the value assertion alone does not prove the index was consulted. Pin the setting so the row cannot pass while exercising nothing. The abort-based proof is unaffected (reverting the gate makes the row abort on its own), and `use_skip_indexes` is not runner-randomized today, so this closes a latent hole rather than an active failure.
The shortcut skipped the data-mutation walk whenever data mutations were not
requested, on the assumption that every alter mutation carries
`alter_version != -1` and is therefore served by the metadata branch. That
assumption does not hold for entries serialized before `alter version` was
written: `ReplicatedMergeTreeMutationEntry::readText` reads the field only
`if (checkString("\nalter version: ", in))`, so such an entry keeps the `-1`
default and is routed through the data branch, where
`needIncludeMutationToSnapshot` would have accepted its `READ_COLUMN` command.
Skipping that walk could drop a pending alter for an old znode, which is the
exact condition this PR exists to prevent.
The shortcut was only an optimization for alter-only snapshots, not part of the
fix, so revert it: this file is now identical to master and the PR is reduced to
the single `createStorageSnapshot` gate plus its test. The empty-snapshot fast
path that motivated the earlier review round is preserved by the `num_alter`
gate in MergeTreeData.cpp, which is untouched.
Re-verified on a debug build after the revert: 04151 matches its reference, the
direct-read row still returns the correct row, and 04143 still passes.
LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 11/11 (100.00%) · Uncovered code |
groeneai
added
the
groeneai-origin-review-human
Ergus
left a comment
Ergus
left a comment
There was a problem hiding this comment.
The regression tests is not failing over master without the fix, so the issue was already fixed or the reproduction is nor determninistic?
|
You are right. It stopped reproducing, because this PR was superseded by my own #112484.
It is not a vacuous pass on master: the mutation is still pending on 3 of 3 parts, The fix is Same for the other shape, Two consequences. With I would close this and move those two queries into |
abashkeev
added
the
comp-mergetree
5 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fix a
LOGICAL_ERRORexception ("Sizes of nested column and null map ... are not equal after deserialization") when querying a table with a skip index over a column that has a pendingALTER MODIFY COLUMNtype change whileapply_mutations_on_flyandapply_patch_partsare both disabled.Description
A pending
ALTER TABLE ... MODIFY COLUMNleaves old parts with skip-index data serialized using the old column type.MergeTreeDataSelectExecutor::canUseIndexskips such an index for a part by checking the column againstAlterConversions::getAllUpdatedColumns(), populated from the read snapshot'sREAD_COLUMNmutations.createStorageSnapshotbuilt that snapshot withneed_alter_mutations = apply_mutations_on_fly || apply_patch_parts. With both off the pending mutation is dropped, the incompatible index is not excluded, andfilterMarksUsingIndexdeserializes an old granule with the new type. ForStringtoNullable(UInt64)that raisesSizes of nested column and null map of Nullable column are not equal after deserialization(an exception in release builds; debug and sanitizer builds abort). The implicitcount()projection reaches the index read during planning, so a plainSELECT count()is enough.Neither setting relates to column type changes, so the snapshot now requests alter mutations whenever one is pending. The request is
need_alter_mutations_if_pending, resolved againstnum_alterinsidegetMutationsSnapshot, which already holds the lock guarding the counters and already reads them for its fast-path condition. A read with no pending alter still returns the empty snapshot without scanningcurrent_mutations_by_version, and adds no lock acquisition.Column data reads were already correct:
IMergeTreeReaderconverts from the part's own stored type. ThesupportsSkipIndexesOnDataReadguard covers the data-read phase but not this planning-time path; both are pinned by the tests. Found by serverfuzz running03702_alter_column_modify_secondary_index_rebuild.sh.Repro:
Related: #112213 (this fixes the pending-mutation half; the killed-mutation half is #112484).