Repository navigation
fix: make generated schema-change DDL safe to replay after a restart - #1402
Conversation
A connector restart re-delivers DDL it already applied: Debezium flushes
offsets periodically, so every DDL event committed since the last flush
arrives again. The generated statements carried no existence guard, so
the replay failed -- and because DDL is retried indefinitely, ONE failed
replay stalls the ENTIRE replication stream, not merely the offending
table. The ClickHouse copy then silently falls behind its source.
Observed end to end during the 2.10.0 image verification: a connector
restarted after a DROP COLUMN retried the replayed statement 20 times and
stopped applying row events altogether; MySQL had 3 rows, ClickHouse 2.
Reproduced directly on ClickHouse 24.8.14, which also showed the problem
is wider than the DROP that surfaced it:
replayed ADD COLUMN Code: 15 DUPLICATE_COLUMN
(column with this name already exists)
replayed DROP COLUMN Code: 10 NOT_FOUND_COLUMN_IN_BLOCK
(cannot find the column to drop)
replayed RENAME COLUMN Code: 10 NOT_FOUND_COLUMN_IN_BLOCK
(cannot find the column to rename)
replayed MODIFY COLUMN succeeds
So ADD/DROP/RENAME are now guarded and MODIFY deliberately is not:
re-applying the same type is already a no-op, and adding IF EXISTS there
would swallow a modification of a column that genuinely does not exist,
turning a real schema divergence into silence. That asymmetry is pinned
by a test so it is not tidied up later.
RENAME needed more than the constant. The rename is also emitted from two
paths that bypass it: postProcessModifyColumn built the statement from a
hardcoded literal, and parseRenameColumn echoes the source tokens
verbatim, so the guard is injected as the COLUMN keyword goes past. Only
fixing the constant would have left both unguarded -- the
grep-and-fix-one-site trap.
This is a pre-existing defect, not a regression: DROP_COLUMN has been
unguarded since e56f041 (2025-04-04) and the recent keyless work never
touched it. The same file already carries the commit "Added if exists to
drop constraint" (7d186e6), so this completes a fix that was started for
one operation and never applied to its siblings.
Also corrects the persist.raw.bytes documentation. BLOB/BINARY/BIT values
are stored HEX-ENCODED AS TEXT by default, so the MySQL x-literal
0102030405 becomes the 10-character string 0102030405 and hex(col) in
ClickHouse returns 30313032303330343035 rather than the original bytes --
a byte-for-byte comparison with the source does not match. The old
one-line comment described only the true branch and never said what the
default actually does, which is how the encoding reads as data corruption
when it is a configured default. The default is unchanged, since flipping
it would require rewriting existing tables.
Tests: 145 green. DdlReplayIdempotencyTest (new, 5) pins a guard on each
generated statement, every clause of a multi-clause statement, and the
deliberate MODIFY exception. Failing-first verified: with the guards
reverted, 4 of 5 fail, each naming the exact production error code.
Pinned expectations in MySqlDDLParserListenerImplTest updated for the
guarded output; the MySQL INPUT statements in those tests are unchanged.
Destructive-Op-C
8000
heck: sites=8; result=pass
How each bug is fixedPosting a per-bug walkthrough of the change, since the two fixes are very different in kind: the first changes behaviour, the second only changes a comment. Bug 1 — a restart after a schema change stalls the whole streamWhat actually goes wrongDebezium flushes offsets periodically, not per event. So when the connector restarts, every DDL event committed since the last flush is delivered again — including events already applied to ClickHouse. Replaying an already-applied schema change fails, and because DDL is retried indefinitely, that one failure never drains: it blocks the queue behind it and the entire replication stream stops, not just the offending table. That last part is what makes it severe. Row events for every other table stop being applied too, and nothing errors loudly at the pipeline level — ClickHouse simply falls further behind its source. Observed end to end during the The scope was wider than the reported
|
| operation | before | after |
|---|---|---|
ADD COLUMN |
ADD COLUMN %s %s |
ADD COLUMN IF NOT EXISTS %s %s |
DROP COLUMN |
DROP COLUMN %s |
DROP COLUMN IF EXISTS %s |
RENAME COLUMN |
RENAME COLUMN |
RENAME COLUMN IF EXISTS |
MODIFY COLUMN |
MODIFY COLUMN %s %s |
unchanged — deliberately |
The guard costs nothing in the normal path: dropping a column that is already gone reaches exactly the intended end state. The difference is that the replay becomes a no-op instead of a permanent stall.
Why MODIFY COLUMN is deliberately left alone
Re-applying the same type is already a no-op, so it needs no guard — verified above. Adding IF EXISTS there would do something actively harmful: it would also swallow a modification of a column that genuinely does not exist, converting a real schema divergence into silence. That is the opposite failure of the one being fixed here, and the worse of the two, because a stall is at least visible.
testModifyColumnIsDeliberatelyNotGuarded pins that asymmetry so a later cleanup pass doesn't "make it consistent".
RENAME needed three fixes, not one
Patching the constant is the obvious move and it is not sufficient — the rename is emitted from two other paths that never read it:
Constants.RENAME_COLUMN— the constant.postProcessModifyColumn— built the statement from a hardcoded string literal, bypassing the constant entirely.parseRenameColumn— echoes the source parse-tree tokens verbatim (RENAME,COLUMN, the two names), so there is no template to change; the guard is injected as theCOLUMNkeyword goes past.
Fixing only (1) would have left (2) and (3) unguarded, and the unit tests would still have gone green. This is why the change touches the parser and not just the constants file.
Not a regression
DROP_COLUMN has been unguarded since e56f0418 (2025-04-04). The recent keyless/GIPK work never touched it — I checked the file history rather than assuming. The same file already carries the commit "Added if exists to drop constraint" (7d186e66), so this completes a fix that was started for one operation and never applied to its siblings.
Bug 2 — persist.raw.bytes: a documentation fix, not a code fix
What is actually happening
BLOB/BINARY/BIT values are stored hex-encoded as text. The MySQL literal x'0102030405' becomes the 10-character string 0102030405, so hex(col) in ClickHouse returns 30313032303330343035 — the hex of that text, not the original bytes — and a byte-for-byte comparison against the source does not match.
Why no behaviour changed
This is a configured default, not corruption. Two checks established that before anything was touched:
- A keyed control table (real primary key, same BLOB payload) renders identically — so it is independent of row identity and unrelated to the keyless work.
- The code path is an explicit
if (persist.raw.bytes) … else hex-encode, i.e. a documented switch withfalseas its default.
So the defect is in the documentation, not the code. The old comment read:
persist.raw.bytes: If set to true, the connector will persist raw bytes as received in a String column.
It describes only the true branch and never says what the default actually does — which is exactly why the hex output reads as data corruption to anyone comparing against MySQL. The comment now documents both modes, the precise consequence (hex(col) returning the hex of the text), and when to flip it — e.g. when db_compare checksum jobs compare binary values.
The default is unchanged. Flipping it would change the stored representation and require rewriting every existing table, which is not a change to make as a side effect of a documentation fix. Callers who need byte-identical binary can set persist.raw.bytes: "true" deliberately.
Tests
DdlReplayIdempotencyTest (new, 5 tests) pins a guard on each generated statement, on every clause of a multi-clause statement — guarding only the first leaves the same defect with a narrower trigger — and on the deliberate MODIFY exception.
Failing-first verified. With the guards reverted, 4 of 5 fail, each naming the exact production error code:
testAddColumnIsGuarded ... Code: 15 DUPLICATE_COLUMN
testDropColumnIsGuarded ... Code: 10, retry loop stalls replication
testRenameColumnIsGuarded ... Code: 10, old name no longer exists
testEveryClause...Guarded ... expected:<2> but was:<0>
145 tests green overall. Pinned expectations in MySqlDDLParserListenerImplTest were updated for the guarded output; the MySQL input statements in those tests are deliberately unchanged — only the ClickHouse expectations moved.
Resolves the conflict on #1398. Three files conflicted; two of the three needed content in NEITHER parent, because taking either side verbatim would have dropped a fix the other side depends on. MySqlDDLParserListenerImpl -- the keyless row identity ------------------------------------------------------ This branch carried the _row_key MATERIALIZED fingerprint from #1399 (ORDER BY cityHash64 over every column, PRIMARY KEY tuple()). The base has since REVERTED that approach in #1401: a column named by the sort-key expression cannot be dropped (Code: 44), and a column added later is absent from the expression, so two rows differing only in the new column collapse to one. #1401 replaced it with MySQL generated invisible primary keys plus KeylessTablePreflight, which refuses to start on a source that already holds a keyless table. Taking ours here would not merely have reinstated a known-broken identity -- it would not have compiled: #1401 removed the rowKeyColumnDefinition local that the branch block reads. Dropped, base kept. The branch OTHER contribution in the same hunk, the ALIAS column emission from column_type_override.alias, exists only on this branch and is preserved. MySqlDDLParserListenerImpl -- postProcessModifyColumn ----------------------------------------------------- Both sides fixed something real and neither subsumes the other: base RENAME COLUMN becomes RENAME COLUMN IF EXISTS (#1402), so replaying an already-applied rename is a no-op instead of a Code: 10 that stalls the whole stream -- DDL is retried indefinitely. branch quotes the database identifier, so a database whose name needs quoting parses. Taking base loses the quoting; taking ours loses the replay guard. Merged as both, formatting the quoted database into the base template. DebeziumChangeEventCapture -------------------------- Additive on this branch only (PostgresConnectorConfig initialisation); the base has no competing line. Branch kept. MySqlDDLParserListenerImplTest -- 59 hunks ------------------------------------------ Every hunk is the same shape: the base expectation carries semantic guards this branch predates (CREATE TABLE IF NOT EXISTS, RENAME and DROP COLUMN IF EXISTS, the #1401 sort-key revert), and the branch expectation carries the quoted database name. Resolved uniformly by taking the base expectation and re-applying the quoting -- never by picking a side. 46 of the 59 were equivalent once both transformations are accounted for. Eleven stale expectations fixed while here ------------------------------------------ The branch quoting change never reached the assertions that sit OUTSIDE the conflict hunks, so they still spelled the database bare and failed against this branch own generator -- on the pre-merge head, not only after the merge. Verified on an untouched worktree at 4fb183f, where MySqlDDLParserListenerImplTest is 96 of 107. Fixed in testAlterAddColumnWithInterleavedAlgorithmClauses, testAlterAddColumnWithInterleavedLockClause, testAlterAddColumnWithTrailingHintsLeavesNoDanglingComma, testAlterDatabaseModifyColumns, testChangeColumnWithExplicitNull, testChangeColumnWithNotNull, testModifyColumnWithExplicitNull, testModifyColumnWithNotNull, testPartitionByRange, testCreateTableWithPartitionByRange, truncateTableWithQualifiedName, and CreateTableIdempotentTest.testGuardPlacement -- a base test whose table-name lookup the branch quoting broke. Verification ------------ Build: both modules, mvn clean install -DskipTests, green. Tests: surefire cannot discover tests in this module on the build host. junit-platform-engine 1.8.2 lands on the test classpath while jupiter needs 1.9.1, so discovery dies with NoSuchMethodError getDiscoveryListener() and surefire then reports Tests run: 0 as a PASS. Reproduced byte-identically on the untouched baseline, so it is the host and not this merge -- but it also means a green surefire run here would prove nothing. Executed instead through the JUnit Platform launcher directly, with the stale 1.8.2 jars removed from the classpath: MySqlDDLParserListenerImplTest 96 of 107 becomes 107 of 107 all 23 unit classes, this merge 314 of 317 all 19 unit classes, baseline 264 of 278 The 3 remaining failures are pre-existing on this branch and untouched by the merge: the three PostgresSchemaReconcilerTest timestamp cases expect a bare DateTime64 precision while this branch own commits a7049bd and 78f2618 made ClickHouseDataTypeMapper emit the UTC-tagged form -- both commits are on this branch and on neither base -- plus one testcontainers case with no Docker on the host. Left for the branch owner rather than papered over in a merge commit. Docker-dependent integration suites were not run locally; CI exercises them. Destructive-Op-Check: sites=14; result=pass
Fixes a replication-stalling bug found while verifying the
2.10.0image end to end, plus a documentation defect that makes correct behaviour look like data corruption. Both are pre-existing and unrelated to the recent keyless/GIPK work.Bug 1 — a restart after a schema change stalls the whole stream
Debezium flushes offsets periodically, so a connector restart re-delivers every DDL event committed since the last flush — including events already applied to ClickHouse. The generated statements carried no existence guard, so the replay failed. Because DDL is retried indefinitely, one failed replay stalls the entire replication stream, not just the offending table, and the ClickHouse copy silently falls behind its source.
Observed end to end: a connector restarted after a
DROP COLUMNretried the replayed statement 20 times and stopped applying row events altogether — MySQL had 3 rows, ClickHouse 2.Reproduced directly on ClickHouse 24.8.14, which showed the problem is wider than the
DROPthat surfaced it:So
ADD/DROP/RENAMEare now guarded, andMODIFYdeliberately is not: re-applying the same type is already a no-op, and addingIF EXISTSthere would swallow a modification of a column that genuinely does not exist — turning a real schema divergence into silence. A test pins that asymmetry so it is not "tidied up" later.RENAMEneeded more than the constantThe rename is emitted from two paths that bypass
Constants.RENAME_COLUMN:postProcessModifyColumnbuilt the statement from a hardcoded literal, andparseRenameColumnechoes the source tokens verbatim (so the guard is injected as theCOLUMNkeyword goes past). Fixing only the constant would have left both unguarded — the grep-and-fix-one-site trap.Not a regression
DROP_COLUMNhas been unguarded sincee56f0418(2025-04-04); the recent keyless work never touched it. The same file already carries the commit "Added if exists to drop constraint" (7d186e66) — this completes a fix that was started for one operation and never applied to its siblings.Bug 2 —
persist.raw.bytesdocumentation was misleadingBLOB/BINARY/BIT values are stored hex-encoded as text by default. The MySQL literal
x'0102030405'becomes the 10-character string0102030405, sohex(col)in ClickHouse returns30313032303330343035— the hex of that text — and a byte-for-byte comparison against the source does not match.That is a configured default, not corruption, but the old one-line comment described only the
truebranch and never said what the default actually does. A keyed control table renders identically, confirming it is independent of row identity. The comment now documents both modes, the exact consequence, and when to flip it (e.g. whendb_comparechecksum jobs compare binary values). The default is unchanged — flipping it would require rewriting existing tables.Tests — 145 green
DdlReplayIdempotencyTest(new, 5) pins a guard on each generated statement, on every clause of a multi-clause statement (guarding only the first leaves the same defect with a narrower trigger), and on the deliberateMODIFYexception.Failing-first verified — with the guards reverted, 4 of 5 fail, each naming the exact production error code:
Pinned expectations in
MySqlDDLParserListenerImplTestwere updated for the guarded output. The MySQL input statements in those tests are deliberately unchanged — only the ClickHouse expectations moved.