Fix divergent mutation of Compact parts with 'basic' serialization info

[groeneai]

Changelog category (leave one):

• Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Fixed a mutation of a Compact part producing a different result depending on whether the part's serialization info was still in memory or had been reloaded from serialization.json, when the table uses serialization_info_version = 'basic'. On ReplicatedMergeTree this made two replicas holding a byte-identical source part write different mutated parts, and the mutation failed with CHECKSUM_DOESNT_MATCH. Closes #113500.

Description

propagate_types_serialization_versions_to_nested_types is written to serialization.json only when the serialization info version is at least WITH_TYPES, and readJSONFromString initializes it to false. A part reloaded from a BASIC file therefore always reported false while the storage reported the setting's own value.

A mutation compares a settings object built from the source part's infos against one built from the storage settings, and operator== is defaulted, so this never-persistable field alone made the two unequal. That fired the loop which materializes serialization info entries for every mutated column, including ones never written, so a part whose infos came from disk gained entries an in-memory one did not and the cross-replica checksum comparison failed.

The fix normalizes the field in the SerializationInfoSettings constructor, inside the existing version < WITH_TYPES block that already defaults the string, nullable and map versions for the same reason. All eight in-tree sites that build such an object from storage settings route through this constructor, so the writer, merge, mutate and reload paths agree by construction. MutateTask is not touched.

Nothing changes on disk: writeJSON already gates the key on WITH_TYPES, and under BASIC the sibling resets already force every nested serialization to its default, so the flag has no observable consumer. Measured over the nested types in Wide parts, the flag on and off gives identical files and content hashes; the same probe under WITH_TYPES gives 62 files vs 50, proving it can see the flag. No setting default changes, and existing parts stay readable with no migration.

Reverting the added line reddens the new stateless test. Applying the reset unconditionally, or forcing the flag on for with_types, reddens the existing 03800_string_size_stream_in_nested_types, which already asserts the substreams of that table with the setting off and on.

Affected: master, 26.7, 26.6. Could someone add v26.7-must-backport and v26.6-must-backport?

Reproducer and measurements

Replication is only the carrier; two plain MergeTree tables in one server differ only in a DETACH/ATTACH:

CREATE TABLE a (cleared String, arr Array(String) DEFAULT []) ENGINE = MergeTree ORDER BY tuple()
  SETTINGS serialization_info_version = 'basic';
CREATE TABLE b (cleared String, arr Array(String) DEFAULT []) ENGINE = MergeTree ORDER BY tuple()
  SETTINGS serialization_info_version = 'basic';
INSERT INTO a (cleared) VALUES ('x');
INSERT INTO b (cleared) VALUES ('x');
DETACH TABLE b; ATTACH TABLE b;
ALTER TABLE a CLEAR COLUMN cleared SETTINGS mutations_sync = 2;
ALTER TABLE b CLEAR COLUMN cleared SETTINGS mutations_sync = 2;

Before: a 4 files / 208 bytes with no serialization.json; b 5 files / 295 bytes carrying
{"kind":"Default","name":"arr","num_defaults":0,"num_rows":0} for a column that was never written.
data.bin and every other file are byte-identical; only serialization.json differs, and
checksums.txt differs because it checksums it. After: both 4 files / 208 bytes.

Array is the minimal carrier because it cannot use sparse serialization, so it gets no entry in the
source part, and that is the entry the mutation invents.

Replicated arm, system.part_log scoped to the two replicas:

	before	after
source parts identical	yes (424 bytes both)	yes (424 bytes both)
MutatePart errors	1 (error=40)	0
mutated bytes	519 vs 581	519 vs 519
recovery DownloadPart	1	0
reads agree	yes	yes
CHECK TABLE on both replicas	1	1

So the cost was a failed mutation plus a wasted mutate-and-refetch, not data loss or wrong results.

[groeneai]

Internal second-model review: 2 rounds, 0 blockers, 0 majors outstanding

I put this through an independent cold review plus a second model, over two rounds. Round 1 produced
two fix items (both executed). Round 2 produced one finding, which I then refuted myself. Recording
it because it is the natural objection to this fix and the answer is not obvious.

⚠️ The one substantive question: does the fix miss tryDowngradeToBasic?

The normalisation lives in the SerializationInfoSettings constructor, gated on
version < WITH_TYPES. But version is also lowered to BASIC after construction by
SerializationInfoSettings::tryDowngradeToBasic, which SerializationInfoByName's constructor calls
unconditionally. That path does produce version == BASIC together with the flag still enabled,
i.e. exactly the state this change says cannot exist. It looks like an uncovered carrier.

It is not, and the reason is the downgrade's own precondition. The downgrade only fires when
string_serialization_version == SINGLE_STREAM, nullable_serialization_version == BASIC and
map_serialization_version == BASIC, so the resulting object differs from a default-constructed one
in the flag and in nothing else. The flag's only behaviour is to choose between recursing with the
settings object and calling getDefaultSerialization, which is doGetSerialization on a
default-constructed object. No doGetSerialization override in src/DataTypes/ reads any field
outside {version, string, nullable, map, propagate}, so by induction over the type tree the two
build the same serialization. The only residual difference is a SerializationDynamic pool key,
i.e. a separate pooled instance with identical behaviour.

Measured rather than argued: over Dynamic, JSON, Variant, Nullable,
Map(String, Array(Tuple(Nullable(String)))), Array(Array(String)), LowCardinality and String
in Wide parts under serialization_info_version = 'basic', the flag on and off give 48 files /
1663 bytes and the identical hash_of_all_files, with the negative control that the same probe
under with_types gives 50 files vs 62, proving it can see the flag. The end-to-end form of the same
route is also identical (parts_identical = 1, both arms 295 bytes / 5 files), because the downgrade
lands on both compared objects symmetrically.

So the state is real and the consequence is nil. Normalising inside tryDowngradeToBasic as well
would ship a change with no test that could redden, and would collide with #111429, which is
currently editing that function. Deliberately not done.

💡 Checked and found sound (recorded, no change)

• Randomisation. ratio_of_defaults_for_sparse_serialization is randomised with a 50% chance of
  1.0, and the test does not pin it. The test stays live anyway: the loop that materialises the
  spurious entries is not gated on isAlwaysDefault, unlike the loop above it, so entries are still
  created and serialization.json is still written. No pin needed, and no
  no-random-merge-tree-settings tag: the SETTINGS clause already wins over runner injection,
  since the client only appends settings the DDL has not set.
• Compact pin. choosePartFormat's threshold test is a disjunction, so
  min_bytes_for_wide_part alone forces Compact regardless of min_rows_for_wide_part. The test
  asserts the part types explicitly, so a broken pin fails loudly instead of going vacuous. A Wide
  fixture would be a control masquerading as a witness, since the bug does not reproduce there.
• DETACH TABLE under a Replicated database. It becomes DETACH PERMANENTLY via the profile
  the DBReplicated job installs, the gating server setting defaults on, and
  detachTablePermanently delegates to detachTable, so the reload this test depends on is unchanged. 138 of the 162 comparable stateless tests carry no no-replicated-database tag.
• Consistency with normalizeSettingsForKey. That helper normalises the other non-persisted
  write-time fields and its comment states the rule this change follows. It correctly omits this flag
  because the flag genuinely selects which serialization is built and must stay in the cache key,
  which is why the fix belongs in the constructor rather than in the key.
• No SettingsChangesHistory.cpp entry is owed: no default moves. Existing parts stay readable
  and no migration is involved.

Mutation coverage

Reverting the added line reddens the new stateless test. Applying the reset unconditionally, or
forcing the flag on for with_types, reddens the existing
03800_string_size_stream_in_nested_types.
(Updated after the unit tests were removed at @PedroTadim's request; the mutants were re-run to
confirm the SQL tests catch each one.) Persisting the key
under BASIC instead was rejected as a real compatibility break: an older reader rejects unknown
top-level keys in serialization.json, which is what serialization_info_version exists to prevent.

[groeneai]

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes, on demand, single node, no replication. Two MergeTree tables with serialization_info_version = 'basic', identical DDL and INSERT, differing only in a DETACH/ATTACH before ALTER TABLE ... CLEAR COLUMN. Unfixed: 4 files / 208 bytes vs 5 files / 295 bytes, hash_of_all_files and hash_of_uncompressed_files both differ. Script in the PR body's collapsed block.
b	Root cause explained?	propagate_types_serialization_versions_to_nested_types is emitted to serialization.json only when the version is at least WITH_TYPES (SerializationInfo.cpp:485), and readJSONFromString initializes it to false (:548). A part reloaded from a BASIC file therefore always reports false while the storage reports the setting. MutateTask builds a source-part settings object from the part's infos (:775) and a storage one from the storage (:786) and compares them with a defaulted operator== (SerializationInfoSettings.h:40), so this never-persistable field alone makes them unequal, firing the loop that materializes an info entry for every mutated column including never-written ones. Introduced by bd171e0255e4d1b.
c	Fix matches root cause?	Yes. It removes the cause (a field that cannot round-trip is normalized at construction) rather than guarding the comparison or special-casing the mutation. It completes an existing documented normalization: the same version < WITH_TYPES block already defaults the string, nullable and map serialization versions, and its own comment states the rule. Written as a threshold, not == BASIC, so it stays correct when a version above WITH_TYPES is added. MutateTask is untouched.
d	Test intent preserved / new tests added?	Both. New stateless test 04771_mutation_serialization_info_basic_divergence asserts identical bytes_on_disk and identical hashes, plus a reads-agree control, a part types ['Compact','Compact'] pin, and a cleared values row asserting the mutation's effect so the fixture cannot pass with the mutation removed. The two unit tests that pinned the normalization were removed at @PedroTadim's request; measured first that the SQL coverage subsumes them, and it does. The scoping half is covered by the existing 03800_string_size_stream_in_nested_types, which builds the same nested-type table under with_types with propagate_types_serialization_versions_to_nested_types off and on and asserts the substreams of both, so applying the reset unconditionally reddens it. No existing test was modified or weakened.
e	Both directions demonstrated?	Yes, through tests/clickhouse-test, on Build-ID-asserted binaries. Fix (d3a76a1d79cfd717): [ OK ]. Mutant with the one added line reverted (6e8be4f80922cd1c): [ FAIL ], diff exactly mutated parts identical 1 -> 0 and mutated part hashes identical 1 -> 0 while every other row stayed green. A second mutant deleting both CLEAR COLUMN statements reddens only cleared values ([''] -> ['x']) and leaves both witness rows green, so the two arms redden disjoint rows and neither carries the other. 50/50 randomized and 50/50 plain both green on the shipping binary.
f	Fix is general across code paths?	Yes. All eight in-tree sites that build such an object from storage settings route through the patched constructor (IMergedBlockOutputStream, MergeTask, MergeTreeData x3, MergeTreeDataWriter x2, MutateTask), so writer, merge, mutate and JSON reload agree by construction rather than at one call site. All six consumers of the flag walked (DataTypeArray:61, DataTypeDynamic:63, DataTypeMap:124, DataTypeNullable:100, DataTypeObject:144,146, DataTypeVariant:212). ALTER UPDATE diverges on the unfixed binary too and is fixed as well, so this is not CLEAR COLUMN-specific. Also checked the five updateHash call sites, four of which are in-memory cache keys in SharedPartColumns.cpp; a changed hash there can only miss sharing, never share unequal keys, since equality compares the full key. Measured byte-identical on both binaries.
g	Fix generalizes across inputs (params/datatypes/wrappers)?	Yes. Divergence reproduces and is fixed across Array, Array(Array), Nullable, LowCardinality, LowCardinality(Nullable), Map, Tuple, UInt64, Float64, Dynamic, JSON, Variant in one 13-column fixture. Boundaries: an empty table produces no part and does not throw; 1000 rows plus a second mutation on the same reloaded part stays identical. reads_agree holds in every cell of every arm. On-disk neutrality measured over Dynamic/JSON/Variant/Nullable/Map/Array/LowCardinality in Wide parts: identical file list and identical content hashes with the flag on and off under BASIC, while under WITH_TYPES the same probe gives 62 files vs 50, which is the control proving the probe can see the flag.
h	Backward compatible? (maintainer-approved exception only)	Yes, nothing changes on disk. writeJSON already gates the key on WITH_TYPES, so no byte written under BASIC changes, confirmed by the measurement in (g). No setting default changes, so no SettingsChangesHistory.cpp entry. Parts already carrying the redundant entries stay readable; the entries are valid, merely redundant (num_defaults:0, num_rows:0), reads agree and CHECK TABLE passes, so no migration is needed. Deliberately did not persist the key under BASIC, which would break older readers that reject unknown top-level keys.
i	Invariants and contracts preserved?	Yes. The invariant is that under a pre-WITH_TYPES version the nested-type serialization knobs are at their defaults, so an object reconstructed from disk is indistinguishable from the one that wrote it. The postcondition is strengthened, never weakened: such an object now always reports false, which is exactly what readJSONFromString already produced for the same part, so the reload path is idempotent with respect to this change. No lifetime, resource, concurrency or error-path surface: a bool in a value object, assigned unconditionally inside an existing branch, no allocation and no early return.

Session id: cron:clickhouse-impl-slot-42:20260805-183600

[groeneai]

cc @rienath @CurtizJ, could you review this? Under serialization_info_version = 'basic' the flag propagate_types_serialization_versions_to_nested_types cannot be written to serialization.json, so a part reloaded from disk reported false while the storage reported true, the defaulted operator== made the two settings objects unequal, and a mutation then materialized serialization info entries for columns it never wrote. This normalizes the flag in the constructor branch that already defaults the string, nullable and map versions for the same reason.

[clickhouse-gh]

Workflow [PR], commit [d7af3ae]

---

AI Review

Summary

This PR normalizes propagate_types_serialization_versions_to_nested_types when SerializationInfoSettings is built for serialization_info_version < WITH_TYPES, and adds a focused stateless repro for the Compact-part mutation divergence behind #113500. After reviewing the current diff, the touched code paths, and the full prior discussion, I did not find new blockers or majors in the current change.

Missing context / blind spots

• ⚠️ I did not run the new stateless test locally in this review. A passing run of 04771_mutation_serialization_info_basic_divergence would close the remaining execution blind spot.

Final Verdict

✅ No new blockers or majors in the current diff.

[clickhouse-gh]

SerializationInfoSettings::updateHash still skips map_serialization_version, so two settings objects that differ only in the Map format hash identically. That is only a benign collision for the SharedPartColumns caches, but SerializationDynamic::create pools by the 128-bit hash alone (SerializationDynamic.cpp:54-60, SerializationObjectPool.cpp:41-61). If a Dynamic value contains a Map, building a serialization once under map_serialization_version = 'basic' and then under WITH_BUCKETS will reuse the first pooled object, so the second table writes the wrong nested Map encoding. Can we include map_serialization_version in updateHash in the same fix?

[groeneai]

Confirmed, and it is already fixed in #113514 rather than here.

updateHash does skip map_serialization_version, and the consequence is as described:
SerializationDynamic::getHash delegates to it (SerializationDynamic.cpp:54-60) and the pool
is an absl::flat_hash_map<UInt128, std::weak_ptr<const ISerialization>> looked up by key with
no equality fallback (SerializationObjectPool.cpp:26, :46-49), so a hash collision is an
object identity collision.

That one-line addition plus its regression tests are in #113514, which touches the same
function in the same file. It carries hash.update(static_cast<int>(map_serialization_version));
and the gtest DynamicPooledSerialization.PoolKeyDistinguishesMapSerializationVersion, plus the
stateless test 04753_dynamic_map_buckets_pooled_serialization, which builds the pooled object
under map_serialization_version = 'basic' and then reads a with_buckets table, the exact
sequence described above.

I am not folding it in here, for a reason beyond one concern per PR. I measured what folding
would do: this PR's hunk is in the constructor (line 28) and #113514's is in updateHash
(line 74), so today git merge-tree reports 0 conflicts between the two. If I add the same line
here, merge-tree still reports 0 conflicts, but the resolved file contains
hash.update(static_cast<int>(map_serialization_version)); twice, once at each side's
position. Nothing flags that: it compiles, and the resulting hash silently differs from what
#113514 intends.

This PR is scoped to the constructor normalising the never-persistable
propagate_types_serialization_versions_to_nested_types flag under
serialization_info_version = 'basic', which fixes the CHECKSUM_DOESNT_MATCH mutation
divergence in #113500. The pool key hash is a distinct defect with its own CI signature
(LOGICAL_ERROR: Stream '<col>.Map(...).buckets_info ... is not found) and its own tests.

[groeneai]

CI finish ledger — a6ed6b3

Every failure below has an owner: a fixing PR (ours or external), or a full-effort fix task
whose fixing-PR link will be posted here when it opens. Only CH Inc sync is exempt.

Check / test	Reason	Owner / fixing PR
Stress test (amd_tsan) / Logical error: '... typeid(*this) == typeid(rhs)' (STID 2508-30f6)	trunk regression with a 2026-08-04 onset: a mixed JOIN ON condition is evaluated over mismatched column types on the default join_algorithm path. Breadth over 10 days on the buildAdditionalFilter frame: 95 rows / 79 distinct PRs / 9 master.	#113534 (external, open)
Integration tests (amd_msan, 4/8) / test_tcp_handler_connection_limits::test_query_count_limit	the test's oracle reads a process-wide query counter that other queries on the same server also advance, so it fails when anything else runs concurrently. Breadth 30 d: 6 master fails / 5206 master ok / 60 PR fails across 57 distinct PRs.	a fix task is created (investigating at full effort — fixing PR link to follow on this PR)
Build (arm_release) / Post Hooks	infra: build_profile_hook.py fails closed when the build-profile telemetry INSERT into the shared CI logs cluster is rejected. Not gating: the Build (arm_release) check-run itself is green.	#113409 (external, merged 2026-08-06T00:05:00Z) — arrives with the next master merge

None is PR-caused. The 2508-30f6 breadth is decisive on its own: 79 distinct unrelated PRs and 9
master hits in 10 days, with an onset on 2026-08-04 that matches #113534's stated cause rather than
anything in this PR.

Note for anyone re-deriving the STID from the database: extract(test_context_raw, 'STID: ...') is
EMPTY for every row of this family, so the STID has to come from the artifact or from #113534's body.
Key on the buildAdditionalFilter frame instead.

Fix-presence probe for the Post Hooks owner, by content rather than commit distance: this head's
ci/jobs/scripts/log_cluster.py is 11916 bytes with no data.seek(0) and one retries= site,
against 12698 bytes with data.seek(0) and four on current master (positive control master,
negative control the merge parent). Master's own Post Hooks reds fell from 4 per hour at 01:00Z to
0 by 05:00Z today, so the fix works and the next master merge picks it up.

Gates: (a) 175 check-runs, 0 queued/in_progress/pending/waiting; (b) Finish Workflow and
Config Workflow both success; (c) last completion 04:44:49Z, >20 min buffer; (d) every failure
above has an owner.

Session id: cron:our-pr-ci-monitor:20260806-050000

[groeneai]

Fixing PR for the test_tcp_handler_connection_limits/test.py::test_query_count_limit row on my CI finish ledger: #113530 (merged 2026-08-06T00:24:35Z).

It replaces the oracle that counted every Done processing connection. line with one that counts only connections closed because a limit was reached (Closing connection due to limits: queries=), so the startup readiness probe and a reconnecting client no longer pollute the count.

The failure on this PR was on a build that predates that merge. Probing the test file per build (marker get_limit_closed_count, with a control string present in both the pre- and post-fix versions scored on every probe): of 28 recent msan runs, the 14 whose build contains the fix are 14 OK / 0 FAIL, and every failure sits on a build without it. On msan flavours the rate went from 63 FAIL / 197 OK before the merge to 0 FAIL / 219 OK after it.

No action needed here beyond a rerun or a rebase onto current master.

[PedroTadim]

@groeneai remove the gtest, we already have the sql one

[groeneai]

Done in d7af3ae. gtest_serialization_info.cpp is back to its master content.

I checked the coverage before removing, because two of the five assertions were about the scope of the normalisation rather than the fix itself, and I was not sure the SQL side reached those. It does. Every source mutation that reddens one of the assertions also reddens a .sql test:

mutation	04771 (new)	03800_string_size_stream_in_nested_types
the added line removed (the bug)	FAIL	OK
the reset applied unconditionally	OK	FAIL
else { flag = true; } for with_types	OK	FAIL
shipping	OK	OK

03800 is what makes the scope half redundant: it already creates the same ten-column nested-type table twice under serialization_info_version = 'with_types', once with propagate_types_serialization_versions_to_nested_types = 0 and once with 1, and asserts system.parts_columns.substreams for both. A normalisation that fires outside version < WITH_TYPES collapses the second block onto the first, so s.size, ns.size, ms%2Ekeys.size and the rest disappear from its reference. The third mutation also reddens 03274_dynamic_column_sizes_vertical_merge and 03262_column_sizes_with_dynamic_structure.

Each arm ran on its own server with SELECT lower(buildId()) asserted against the binary, and the source is byte-identical to before afterwards.

@Avogar this is after your approval, so the tree is not the one you looked at. The diff is now 2 source lines plus the stateless test; the deleted file is identical to master. Sorry for the extra round.

[PedroTadim]

@Avogar very easy to review