Fix index granularity collapsing to 1 when mutating an adaptive part on a table with index_granularity_bytes = 0 by groeneai · Pull Request #111694 · ClickHouse/ClickHouse

groeneai · 2026-07-23T22:30:27Z

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):

Fixes index granularity collapsing to one mark per row (large mark and primary-index bloat) when a row-changing mutation rewrites an adaptive data part that lives on a table with index_granularity_bytes = 0.

Description

A part written with byte-based granularity can land on a table where that sizing is disabled (index_granularity_bytes = 0), for example via RESTORE ... AS ... SETTINGS allow_different_table_def = 1. A row-changing mutation (ALTER ... DELETE, TTL, any full-column rewrite) then rewrites it and inherits the source's granularity info, so the writer sizes granules by bytes while the byte budget is zero.

computeIndexGranularity computed 0 / size_of_row = 0, which the floor below turned into 1: one mark per row, a thousands-fold mark and primary-index bloat. Data stayed correct, so correct-result tests did not catch it.

Fix: when index_granularity_bytes = 0, use the fixed row granularity instead of dividing a zero byte budget. The index_granularity_bytes > 0 and non-adaptive paths are untouched, verified byte-for-byte against a pre-fix build across small and large byte budgets, both part types, and index_granularity = 1.

The guard sits on the single function every part writer reaches, so it covers every route into that state, not only the reproducer's: also a restored Compact part, unblocked by #111626, and a replicated merge, which honours the assigning replica's part format.

The test covers both part types, and both arms fail against a pre-fix build: for the 10000-row part below, marks go from 10000 to 3 (Wide) and 10000 to 2 (Compact).

CREATE TABLE src (k UInt64, s String) ORDER BY k
    SETTINGS min_bytes_for_wide_part = 0, index_granularity = 8192;
INSERT INTO src SELECT number, toString(number) FROM numbers(10000);
BACKUP TABLE src TO Disk('backups', 'b');

CREATE TABLE dst (k UInt64, s String) ORDER BY k SETTINGS index_granularity_bytes = 0;
RESTORE TABLE src AS dst FROM Disk('backups', 'b') SETTINGS allow_different_table_def = 1;
ALTER TABLE dst DELETE WHERE k = 0 SETTINGS mutations_sync = 2;

SELECT part_type, rows, marks FROM system.parts WHERE table = 'dst' AND active;

Workflow [PR]
Sync PR [sync-upstream/pr/111694]

…on a table with index_granularity_bytes = 0 An adaptive part (a Wide part with .mrk2 marks, or a Compact part) can be placed on a table whose index_granularity_bytes = 0 (non-adaptive) via RESTORE ... AS ... with allow_different_table_def = 1. A row-changing mutation (ALTER DELETE, TTL, or any mutation that rewrites all columns) then rewrites the part, inheriting the source part's adaptive index_granularity_info. The writer therefore runs with can_use_adaptive = true while the table's index_granularity_bytes = 0. In computeIndexGranularity the adaptive branch then fell through to index_granularity_bytes / size_of_row = 0 / N = 0, clamped to 1, producing one mark per row (thousands-fold mark and primary-index bloat). Data stayed correct, so correct-result tests could not catch it. Fix: in the adaptive branch, when index_granularity_bytes = 0 (byte-based sizing disabled) use the fixed row granularity instead of dividing by a zero byte budget. This enforces the invariant the function already documented. The normal adaptive path (index_granularity_bytes > 0) and the non-adaptive path are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

groeneai · 2026-07-23T22:30:59Z

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes. `clickhouse-local`/test 04372: adaptive part restored onto an `index_granularity_bytes = 0` table, then `ALTER DELETE` -> 10000 marks for 9999 rows, every time.
b	Root cause explained?	Mutation inherits the source part's adaptive `index_granularity_info` (`MutateTask.cpp:3581`), so the writer runs `can_use_adaptive = true` while `index_granularity_bytes = 0`; `computeIndexGranularity` then hits `index_granularity_bytes / size_of_row = 0/N = 0`, clamped to 1 -> one mark per row.
c	Fix matches root cause?	Yes. In the adaptive branch, `index_granularity_bytes == 0` now uses `fixed_index_granularity_rows` instead of dividing by a zero byte budget, enforcing the invariant the function already documented.
d	Test intent preserved / new tests added?	New stateless test `04372` asserts exact `part_type`/`rows`/`marks` (3, not 10000) after the mutation. No existing test weakened.
e	Both directions demonstrated?	Yes, on the identical master base: pristine (Build ID 3DDAE2E7) -> 10000 marks; with fix (31C56DC0) -> 3 marks, data intact.
f	Fix is general across code paths?	Yes. `computeIndexGranularity` is the single funnel all write paths use (insert/merge/mutation writers); no sibling copy of the `0/row_size` logic exists. Verified for both `Wide` and `Compact` adaptive parts.
g	Fix generalizes across inputs?	Yes. The branch is entered for adaptive-marks + `igb == 0` regardless of column datatypes. Boundary paths unchanged: `igb > 0` and non-adaptive (`can_use_adaptive = false`) are byte-for-byte identical.
h	Backward compatible?	Yes. No setting-default, on-disk, wire, or format change (marks layout is per-part self-describing). Pure in-memory granularity computation; no `SettingsChangesHistory` entry needed.
i	Invariants and contracts preserved?	Yes. The `min(fixed_gr, ...)` and `== 0 -> 1` clamps still hold (input is already `fixed_gr`); the other two adaptive sub-branches are unchanged. No caller contract altered.

Session id: cron:clickhouse-author-slot-6:20260723-202100

groeneai · 2026-07-23T22:31:19Z

Internal second-model review — adjudication log (click to expand)

Pre-publication review by an independent model (engine: codex; 5 findings across 1 full pass + 1 fix round + 1 delta recheck; all AGREE and fixed before publishing).

#	Sev	Finding	Verdict	Evidence / action
1	⚠️	Test left `index_granularity` / `index_granularity_bytes` exposed to the settings randomizer, so mark counts were non-deterministic	AGREE	Pinned `index_granularity_bytes = 10485760` on src and `index_granularity = 8192` on dst. Confirmed `clickhouse-test` randomizes both (`tests/clickhouse-test:1680-1682`).
2	⚠️	`marks <= 3` oracle too weak — would pass on an oversized granularity or lost adaptive metadata	AGREE	Changed to exact assertions: `Wide / 9999 / 3` post-mutation (and exact src/restore rows+marks).
3	⚠️	`min_bytes_for_full_part_storage` randomized -> src could become `Packed`, which the non-adaptive dst rejects at RESTORE before the fix is exercised	AGREE	Pinned `min_bytes_for_full_part_storage = 0` on src. Reproduced the Packed-RESTORE failure and the pinned-Full success locally.
4	💡	`Related: #111626` link left inside the HTML comment, invisible in the rendered body	AGREE	Moved it to a visible `Related:` line.
5	💡	Changelog entry used past tense (`Fixed`) contrary to the present-tense guideline	AGREE	Changed opening to `Fixes`.

Severity: ❌ blocker / ⚠️ major / 💡 nit. All findings were AGREEd and addressed; the fix to the source (computeIndexGranularity) was unchanged across review rounds — findings 1-3 hardened the regression test, 4-5 the PR text.

Session id: cron:clickhouse-author-slot-6:20260723-202100

groeneai · 2026-07-23T22:32:14Z

cc @alesapin @CurtizJ - could you review this? A row-changing mutation of an adaptive part (Wide .mrk2 or Compact) that sits on an index_granularity_bytes = 0 table (reachable via RESTORE ... allow_different_table_def = 1) inherits the part's adaptive granularity, so computeIndexGranularity divides a zero byte budget and collapses to one mark per row. The fix falls back to the fixed row granularity when index_granularity_bytes = 0.

clickhouse-gh · 2026-07-23T22:55:15Z

Workflow [PR], commit [72a5c13]

Summary: ❌

Performance Comparison: Performance dashboard

job_name	test_name	status	info
Fast test (arm_darwin)		FAIL
	03312_squashing_with_low_card_mem_usage	FAIL	cidb
Stateless tests (amd_llvm_coverage, ParallelReplicas, s3 storage, parallel)		FAIL
	04510_not_has_function_explain	FAIL	cidb
	02402_merge_engine_with_view	NOT_FAILED	cidb
	02156_storage_merge_prewhere	NOT_FAILED	cidb

AI Review

Summary

This PR fixes computeIndexGranularity for the case where an adaptive part is rewritten on a table with index_granularity_bytes = 0, so the writer falls back to the fixed row granularity instead of collapsing to one mark per row. I traced the mutation, merge, and writer call paths that reach this helper and reviewed the new Wide and Compact regression coverage; I did not find any blockers or majors in the current patch.

Final Verdict

✅ No blockers or majors found.

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	87.00%	87.00%	+0.00%
Functions	91.90%	91.90%	+0.00%
Branches	79.40%	79.40%	+0.00%

Changed lines: Changed C/C++ lines covered: 6/6 (100.00%) · Uncovered code

Full report · Diff report

clickhouse-gh · 2026-07-24T04:01:49Z

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	80.60%	86.20%	+5.60%
Functions	91.00%	92.00%	+1.00%
Branches	72.50%	78.30%	+5.80%

Changed lines: Changed C/C++ lines covered: 6/6 (100.00%) · Uncovered code

Full report · Diff report

groeneai · 2026-07-24T06:55:52Z

CI finish ledger — `ac3f0e5`

CI fully finished (Config + Finish Workflow SUCCESS, 2h30m buffer). Both Fast tests passed, so no PR-caused failure. Every failed check has an owner; only CH Inc sync is exempt.

Check / test	Reason	Owner / fixing PR
Integration tests (arm_binary, distributed plan, 2/4) / test_storage_s3_queue/test_system_stop.py::test_stopped_table_releases_hash_ring_slot	crash — hash-ring OOB, master regression from #107476 (11 PRs / 2 master in 14d; unrelated to this granularity PR)	#108977 (ours, open)
Integration tests (arm_binary, distributed plan, 2/4) / test_storage_s3_queue/test_system_stop.py::test_refresh_survives_unready_dependencies	collateral from the same-node #108977 crash; standalone rabbitmq variant tracked separately	#108977 (ours, open) / #111480 (ours, open)
Stateless tests (amd_asan_ubsan, distributed plan, parallel) / 04509_hash_table_sizes_stats_table_functions	flaky (5 unrelated PRs + master in 30d; hash-table-sizes-stats measurement timing-sensitive)	a fix task is created (investigating at full effort; fixing-PR link to follow here)
Upgrade check (amd_release) / Changed + New settings not reflected in settings changes history	flaky/infra (165 PRs / 0 master in 7d; upgrade-check settings-history baseline)	#104860 (merged) — join_runtime_filter_min_probe_rows SettingsChangesHistory entry
Upgrade check (amd_release) / Error message in clickhouse-server.log	infra — DiskLocalCheckThread clickhouse_disk_checker teardown-race (0 master)	#110773 (ours, open)
Sync	-	CH Inc sync (private, not actionable)

Session id: cron:our-pr-ci-monitor:20260724-063000

groeneai · 2026-07-27T04:42:47Z

Fixing PR for the 04509_hash_table_sizes_stats_table_functions line of the CI finish ledger above: #111502 (external, @ alexey-milovidov, open).

I also need to correct the reason I gave in that ledger row. I wrote "measurement timing-sensitive", which is wrong -- the failure is deterministic, not timing-dependent. All 5 occurrences of this signature over the last 45 days carry a nonzero randomized max_bytes_before_external_group_by (15366675, 7260142, 47485666, 59730821, 42439855). Under that setting the aggregation spills to disk, and in AggregatingTransform::finalizeAggregation the merge branch is gated on if (!aggregator_has_temporary_data()), which guards the only two Aggregator::prepareVariantsToMerge calls -- and that is the only call site of updateStatistics. So a spilled query never records its hash-table sizes, the next run's getSizeHint finds no entry, and AggregationPreallocatedElementsInHashTables reads 0 instead of 650000. The runner's own minimizer confirmed determinism on one of these runs: 32/32 failed replaying the same randomized settings, 60/60 passed with randomization off.

#111502 pins max_bytes_before_external_group_by=0 and max_bytes_ratio_before_external_group_by=0 in both the settings and dist_settings arrays, which covers both branches of the runner's randomize_external_sort_group_by draw. No fix is needed from me here.

…r arm)

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.

groeneai · 2026-08-21T21:33:20Z

Pushed a master merge plus a second test arm. What changed and why:

Master merged (68d548f). Required for the new arm: before Fix LOGICAL_ERROR when loading an existing Compact part under a non-adaptive table policy #111626 (now merged) the Compact restore aborted before the mutation could run, so that carrier was untestable. This also picks up the fixes for the three checks that were red on the previous head, all trunk flakes at the time.
Compact carrier arm added to the existing test, not a second file. Marks after ALTER ... DELETE: 10000 to 2 for Compact, 10000 to 3 for Wide. The asymmetry is real, not a typo: the Compact writer folds the trailing partial granule into the last mark, the Wide writer does not.
Comment trim in the test and one line in src/. The remaining src/ comment states the invariant; the removed one narrated the defect.
The src/ fix itself is unchanged from the previously reviewed head.

Internal second-model review (Gate B, cold re-review): 0 findings

Gate B re-reviewed the full scope cold against the frozen PR body: 0 findings (engine codex, $10.50; PR total $49.58 across 2 Gate A and 4 Gate B rounds).

My own independent cold review raised 2 nits, both DISAGREE with evidence, 0 blockers, 0 majors:

#	Finding	Verdict	Evidence
⚠️ 1	Exact-mark oracle vs unpinned randomized merge-tree settings (`use_const_adaptive_granularity`, `enable_index_granularity_compression`, `compact_parts_max_*_to_buffer`, `merge_max_block_size`)	DISAGREE	None is an input to the mark stride: after the fix the granule size is `index_granularity`, and `marks` is `num_marks_without_final + has_final_mark`. Those keys change the granularity class, the on-disk encoding and flush cadence. Confirmed empirically: 50/50 runs pass under live randomization, and a 4-config A/B matrix is byte-for-byte identical to a pre-fix build. Pinning them off would reduce coverage for no gain.
⚠️ 2	Only `no-fasttest`; should `no-parallel` / `no-replicated-database` be added?	DISAGREE	Not owed. Backup names derive from `CLICKHOUSE_TEST_UNIQUE_NAME` and every table is in `currentDatabase()`, so concurrent copies cannot collide. `--replicated-database` changes only the database engine, not table engines, so part type and mark counts are unaffected.

Validation on this head: both directions on one base with two build IDs (identity asserted against each running server); the test file reddens on the pre-fix binary with both reference lines moving independently, so the new arm is not riding on the existing one; 50/50 randomized runs pass; the four non-defective configurations are unchanged.

Verified during review that the line this diff replaces is from an earlier commit of mine, not a maintainer's, and that the change map is exactly three files with no deletions against the merged base.

clickhouse-gh · 2026-08-21T23:12:59Z

Build profile diff (arm_release)

Comparing 72a5c135c with master 34cba4476 (stripped binary size, per-symbol sizes and ThinLTO time; compile times per translation unit against the most recent warmup build that recompiled it).

✅ No significant changes.

Binary sizes

Binary	Master	PR	Δ
`programs/clickhouse-stripped`	705.14 MiB	702.11 MiB	-3.03 MiB (-0.43%)

Only the stripped binary is compared: the official master build keeps debug symbols while PR builds strip them, so the other binaries differ by construction.

Compile time of recompiled translation units

7 translation units recompiled, 6 s compile time in total, 7 of them have a recent master baseline.

Job report

groeneai · 2026-08-22T02:51:55Z

CI finish ledger - `72a5c13`

Every failure below has an owner: a fixing PR (mine 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
Stateless tests (amd_llvm_coverage, ParallelReplicas, s3 storage, parallel) / 04510_not_has_function_explain	trunk bug, not caused by this PR: `Code: 53 ... CAST AS Array can only be performed between same-dimensional array types` on `_CAST([[('k', '3')]], 'Array(Map(String, String))')`. Thrown from `FunctionsConversion.cpp:897`; this PR only touches `MergeTreeIndexGranularity.cpp`. The same failure fires on PR 112985, which shares no file with this one. The failing query is a master-side test line that postdates this branch point (the test is 80 lines at this head, 403 on master).	#115883 (external, open)
Fast test (arm_darwin) / 03312_squashing_with_low_card_mem_usage	macOS runner disk exhaustion: `Code: 243 ... Cannot reserve 317.82 MiB, not enough space` while inserting 5e6 rows. A runner capacity condition, not a granularity change; the test is not in this PR's diff.	#113605 (mine, open)
CH Inc sync	still running	CH Inc sync (private, not actionable by me)

Mergeable Check and PR are roll-ups of the two leaves above, not separate failures.

Session id: cron:our-pr-ci-monitor:20260822-023000

alexey-milovidov added the can be tested Allows running workflows for external contributors label Jul 23, 2026

clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Jul 23, 2026

This was referenced Jul 24, 2026

Materialize column statistics on INSERT for small tables by default #109454

Merged

Add UUID2 data type with correct sorting #110084

Open

groeneai added the groeneai-origin-unknown PR origin: unclassified (legacy task) label Aug 19, 2026

groeneai added 2 commits August 21, 2026 18:18

Merge master to pick up ClickHouse#111626 (enables the Compact carrie…

72dfbcd

…r arm)

groeneai added the groeneai-origin-follow-up PR origin: groeneai's own out-of-scope finding during its PR work label Aug 21, 2026

alexey-milovidov mentioned this pull request Aug 27, 2026

Fix buffer overflows on data from external servers #115706

Open

clickhouse-gh Bot added the comp-mergetree MergeTree* family: parts, merges, primary index, column statistics, background data transformation. label Sep 4, 2026

Navigation Menu

Uh oh!

Fix index granularity collapsing to 1 when mutating an adaptive part on a table with index_granularity_bytes = 0 - #111694

Fix index granularity collapsing to 1 when mutating an adaptive part on a table with index_granularity_bytes = 0#111694
groeneai wants to merge 3 commits into
ClickHouse:masterfrom
groeneai:groeneai-fix-mutate-adaptive-part-granularity-igb0

groeneai commented Jul 23, 2026 •

edited by clickhouse-gh Bot

Loading

Uh oh!

groeneai commented Jul 23, 2026

Uh oh!

groeneai commented Jul 23, 2026

Uh oh!

groeneai commented Jul 23, 2026

Uh oh!

clickhouse-gh Bot commented Jul 23, 2026 •

edited

Loading

Uh oh!

clickhouse-gh Bot commented Jul 24, 2026

Uh oh!

groeneai commented Jul 24, 2026 •

edited

Loading

Uh oh!

groeneai commented Jul 27, 2026

Uh oh!

groeneai commented Aug 21, 2026

Uh oh!

clickhouse-gh Bot commented Aug 21, 2026

Uh oh!

groeneai commented Aug 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

groeneai commented Jul 23, 2026 • edited by clickhouse-gh Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

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

Description

Uh oh!

groeneai commented Jul 23, 2026

Uh oh!

groeneai commented Jul 23, 2026

Uh oh!

groeneai commented Jul 23, 2026

Uh oh!

clickhouse-gh Bot commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI Review

Summary

Final Verdict

LLVM Coverage Report

Uh oh!

clickhouse-gh Bot commented Jul 24, 2026

LLVM Coverage Report

Uh oh!

groeneai commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI finish ledger — ac3f0e5

Uh oh!

groeneai commented Jul 27, 2026

Uh oh!

groeneai commented Aug 21, 2026

Uh oh!

clickhouse-gh Bot commented Aug 21, 2026

Build profile diff (arm_release)

Uh oh!

groeneai commented Aug 22, 2026

CI finish ledger - 72a5c13

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

groeneai commented Jul 23, 2026 •

edited by clickhouse-gh Bot

Loading

clickhouse-gh Bot commented Jul 23, 2026 •

edited

Loading

groeneai commented Jul 24, 2026 •

edited

Loading

CI finish ledger — `ac3f0e5`

CI finish ledger - `72a5c13`