feat(mysql): carry MySQL column DEFAULT through to ClickHouse on ADD COLUMN - #4632
feat(mysql): carry MySQL column DEFAULT through to ClickHouse on ADD COLUMN#4632dtunikov wants to merge 5 commits into
Conversation
|
|
||
| // retry without default if ch rejected the expression | ||
| addColumnWithDefaultFallback := func(tableName string) error { | ||
| err := addColumn(tableName, columnDef) |
There was a problem hiding this comment.
try to add with full col def (including default)
then retry with simple clickHouseColType if it's failed
if we want to, we can also add a metric + alert when this happens
or just fail with an error
There was a problem hiding this comment.
Metric and a non-paging alert sounds like a good idea. Could also be a task to check these logs in a month, but alert is a more reliable way to have it happen and we want our defaults to be good
❌ Test FailureAnalysis: Not flaky — Test_MySQL_AlterTableAddColumnDefault, the new test for this PR's own feature, fails deterministically on the MySQL 5.7 (mysql-pos) leg with "cannot convert bytes [98] to uint16enum" on every retry until timeout, because qvalue_convert.go has no ENUM-label handling for byte-string defaults. |
Which defaults are carried through
Literals are rendered with SQL quote doubling rather than backslash escapes, so the value stays valid in any destination dialect. Which are declinedThe column is still added, just without a
If a literal is rendered but ClickHouse rejects it anyway, |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
| nullable = false | ||
| break | ||
| case ast.ColumnOptionDefaultValue: | ||
| // Servers without binlog row metadata carry enums and sets as their ordinal or |
There was a problem hiding this comment.
that is unfortunate
but not much we can do here
on the qrep path we handled it using enum as unsigned conversion
but here can't avoid getting a plain string
ilidemi
left a comment
There was a problem hiding this comment.
Not a blocker for this PR, but is there a plan for ensuring we don't skip over DDL?
@fable Please look through the logic of mysql cdc.go and determine all the cases where a DDL with a column add might not get propagated as a schema delta. Ignore the ones where we fail to parse the query and emit a metric, but past that everything that belong to our tables should result in a delta applied on the destination
|
|
||
| // retry without default if ch rejected the expression | ||
| addColumnWithDefaultFallback := func(tableName string) error { | ||
| err := addColumn(tableName, columnDef) |
There was a problem hiding this comment.
Metric and a non-paging alert sounds like a good idea. Could also be a task to check these logs in a month, but alert is a more reliable way to have it happen and we want our defaults to be good
| } | ||
| return "'" + strings.ReplaceAll(v, "'", "''") + "'", true | ||
| default: | ||
| // DEFAULT NULL is a nil datum, bit literals are types.BinaryLiteral |
There was a problem hiding this comment.
Nit: could make exhaustive and warn/alert if something changes
|
Fable is ignoring us but there's another option https://claude.ai/code/session_01Sf6nJJgyfu4T8z1KxSmNar |
❌ Test FailureAnalysis: The PR's new Test_MySQL_AlterTableAddColumnDefault fails deterministically (4/4 MySQL+MariaDB→ClickHouse suites) on the ch-lts leg with a constant c_time type mismatch (QValueTime 13h14m15s vs QValueTimestamp 1970-01-01), because the test/backfill path doesn't handle the legacy non-Time64 ClickHouse mapping that Flag_ClickHouseTime64Enabled gates. |
ALTER TABLE ... ADD COLUMN c INT DEFAULT 5is DDL, not DML: MySQL makes existing rows read back as5without emitting a single row event for them. CDC therefore has nothing to replicate, and rows untouched since the ALTER show ClickHouse's type default (0/''/NULL) indefinitely — until a full table resync.Emitting the
DEFAULTon the destinationADD COLUMNfixes exactly that set: ClickHouse evaluates a column default at read time for parts written before the column existed, which is by construction the rows whose last change predates the ALTER. Rows changed after it carry real binlog values and win on version dedup, so the two mechanisms don't overlap.Changes
FieldDescription.default_expr(optional string) carries a rendered SQL literal; unset means no default, or one we declined to translate.MyDecimal, both handled.DEFAULT <literal>toADD COLUMN, for shard and distributed tables.Declined (unset, no DDL change):
NULL(nullable columns already read back asNULL),CURRENT_TIMESTAMP/NOW()/UUID()and other expressions, bit/hex literals, and strings needing escapes beyond quote doubling. Expression defaults are deliberately not translated — read-time evaluation would give a different value per query, and MySQL gives each pre-existing row a distinct value for things likeUUID().If ClickHouse rejects a literal we translated, the
ADD COLUMNis retried without it and a warning is logged. Losing a default only forfeits the historical backfill; stalling schema replay would stop the table.Scope
MySQL source, ClickHouse destination, CDC DDL path only. Not covered: snapshot/resync
CREATE TABLE(so a resync drops the default from the DDL — data stays correct since it re-copies real values), the gh-ost path (binlog row metadata carries no defaults), Postgres sources, other destinations.Tests
TestAlterTableAddColumnDefaultcovers 27 rendering cases including the declined ones. Two e2e tests:Test_MySQL_AlterTableAddColumnDefaultinserts a row before the ALTER so a plain source↔destination comparison only passes if the default carried across (29 type/default combinations), andTest_MySQL_AlterTableAddColumnDefaultUntranslatedasserts declined and rejected defaults don't stall replication.🤖 Generated with Claude Code