Skip to content

fix: a column rename can NULL out rows still in flight - #1395

Merged
minguyen9988 merged 5 commits into
2.10.0from
omniwatcher/fix-ddl-rename-inflight-null
Aug 24, 2026
Merged

fix: a column rename can NULL out rows still in flight#1395
minguyen9988 merged 5 commits into
2.10.0from
omniwatcher/fix-ddl-rename-inflight-null

Conversation

@minguyen9988

@minguyen9988 minguyen9988 commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

Reviewed by a 3-model cross-provider adversarial judge panel (Gemini 3.1 Pro / Grok 4.2 Reasoning / GPT-5.5): 3/3 APPROVE, round 1, zero blocking findings.

Full detail follows, as committed:

fix: a column rename can NULL out rows still in flight

ALTER TABLE ... CHANGE COLUMN old new renames the column in ClickHouse
while records read under the PRE-rename schema are still buffered. Those
records carry the OLD field name; the table now has the NEW column. The
writer finds no value for it, binds NULL, and overwrites the real one.

The row count is unaffected, so count-based checksum jobs report the table
clean. The corruption is invisible until someone reads the column.

Measured on MySQL 8.0.36 (ROW/FULL/GTID) -> ClickHouse 24.8.14, 80 inserts
issued across a rename with no pause:

row count        mysql=80   ch=80    <- matches, so checksums pass
NULL in renamed  mysql=0    ch=3     <- ids 38,39,40 silently blanked

and an UPDATE straddling the rename was lost outright:

mysql : 1=upd1, 2=upd2, 3=orig3
ch    : 1=<NULL>, 2=upd2, 3=orig3

The connector logged Column index missing for column b_renamed five
times during the run, which is the writer reporting exactly this and
carrying on.

Root cause is the DDL path's flush, which does not flush. On receiving a
DDL it logs "Flush all existing records" and calls executor.pause(), but
pause() only stops NEW batches from starting: it does not drain the queue
and does not wait for a batch already inside a task body. The DDL was then
applied immediately, while a batch of pre-rename records was still being
written -- against the post-rename table.

The fix makes the drain real, in three ordered steps before the DDL is
applied: let the pool consume what is already queued (the pool is still
running at this point), then pause() so no new batch starts, then wait out
the batches still executing. Only then is every record read under the
pre-ALTER schema actually in ClickHouse.

Waiting out the running batches needs a signal the executor did not have,
so ClickHouseBatchExecutor now counts batches inside a task body
(incremented in beforeExecute, decremented in afterExecute so a batch that
throws cannot leak the counter) and exposes awaitQuiescent(timeout).

The drain is bounded at 60s. On timeout the DDL proceeds and a warning
names the condition: blocking replication indefinitely would be worse than
the corruption being avoided, and a warning is better than doing this
silently, which is how the bug behaved.

After the fix, same scenarios:

scenario                                  mysql   ch    result
quiescent rename, no in-flight writes       0     0     PASS
rename with 80 writes in flight             0     0     PASS   (was 3)
UPDATE straddling the rename              upd1   upd1   PASS   (was NULL)
"Column index missing" log lines             -     0    PASS   (was 5)

Note this is the second half of the pre-ALTER record problem. The ADD
COLUMN half was fixed earlier by omitting from the INSERT column list any
column absent from the record's schema, so ClickHouse applies the column
DEFAULT instead of NULL. That approach cannot address a rename: the value
is present in the record, just under the previous name, so the correct
outcome is to write it before the rename takes effect rather than to skip
the column.

Tests: sink-connector 190 run / 0 failures / 20 errors against an untouched
baseline of 186 / 0 / 20 on the same base commit; the 20 are pre-existing
testcontainers "Could not find a valid Docker environment" errors and the
delta is exactly the 4 new tests. Failing-first verified: with
awaitQuiescent stubbed to return immediately (the old no-drain behaviour)
testAwaitQuiescentWaitsForRunningBatch fails on the corruption window,
while the idle-path, failed-batch and pause-still-blocks guards keep
passing.

Arnaud Adant and others added 5 commits August 8, 2026 23:41
…ptions

add options to take non locking snapshots
`ALTER TABLE ... CHANGE COLUMN old new` renames the column in ClickHouse
while records read under the PRE-rename schema are still buffered. Those
records carry the OLD field name; the table now has the NEW column. The
writer finds no value for it, binds NULL, and overwrites the real one.

The row count is unaffected, so count-based checksum jobs report the table
clean. The corruption is invisible until someone reads the column.

Measured on MySQL 8.0.36 (ROW/FULL/GTID) -> ClickHouse 24.8.14, 80 inserts
issued across a rename with no pause:

    row count        mysql=80   ch=80    <- matches, so checksums pass
    NULL in renamed  mysql=0    ch=3     <- ids 38,39,40 silently blanked

and an UPDATE straddling the rename was lost outright:

    mysql : 1=upd1, 2=upd2, 3=orig3
    ch    : 1=<NULL>, 2=upd2, 3=orig3

The connector logged `Column index missing for column b_renamed` five
times during the run, which is the writer reporting exactly this and
carrying on.

Root cause is the DDL path's flush, which does not flush. On receiving a
DDL it logs "Flush all existing records" and calls `executor.pause()`, but
pause() only stops NEW batches from starting: it does not drain the queue
and does not wait for a batch already inside a task body. The DDL was then
applied immediately, while a batch of pre-rename records was still being
written -- against the post-rename table.

The fix makes the drain real, in three ordered steps before the DDL is
applied: let the pool consume what is already queued (the pool is still
running at this point), then pause() so no new batch starts, then wait out
the batches still executing. Only then is every record read under the
pre-ALTER schema actually in ClickHouse.

Waiting out the running batches needs a signal the executor did not have,
so ClickHouseBatchExecutor now counts batches inside a task body
(incremented in beforeExecute, decremented in afterExecute so a batch that
throws cannot leak the counter) and exposes awaitQuiescent(timeout).

The drain is bounded at 60s. On timeout the DDL proceeds and a warning
names the condition: blocking replication indefinitely would be worse than
the corruption being avoided, and a warning is better than doing this
silently, which is how the bug behaved.

After the fix, same scenarios:

    scenario                                  mysql   ch    result
    quiescent rename, no in-flight writes       0     0     PASS
    rename with 80 writes in flight             0     0     PASS   (was 3)
    UPDATE straddling the rename              upd1   upd1   PASS   (was NULL)
    "Column index missing" log lines             -     0    PASS   (was 5)

Note this is the second half of the pre-ALTER record problem. The ADD
COLUMN half was fixed earlier by omitting from the INSERT column list any
column absent from the record's schema, so ClickHouse applies the column
DEFAULT instead of NULL. That approach cannot address a rename: the value
is present in the record, just under the previous name, so the correct
outcome is to write it before the rename takes effect rather than to skip
the column.

Tests: sink-connector 190 run / 0 failures / 20 errors against an untouched
baseline of 186 / 0 / 20 on the same base commit; the 20 are pre-existing
testcontainers "Could not find a valid Docker environment" errors and the
delta is exactly the 4 new tests. Failing-first verified: with
awaitQuiescent stubbed to return immediately (the old no-drain behaviour)
testAwaitQuiescentWaitsForRunningBatch fails on the corruption window,
while the idle-path, failed-batch and pause-still-blocks guards keep
passing.
@minguyen9988 minguyen9988 changed the title WIP: DO NOT MERGE - fix: a column rename can NULL out rows still in flight fix: a column rename can NULL out rows still in flight Aug 24, 2026
@minguyen9988
minguyen9988 merged commit 33ff707 into 2.10.0 Aug 24, 2026
8 of 10 checks passed
@minguyen9988
minguyen9988 deleted the omniwatcher/fix-ddl-rename-inflight-null branch August 24, 2026 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants