Skip to content

Feature: Handle read only columns - #437 followup - #457

Open
driv3r wants to merge 15 commits into
feat/handle-generated-columnsfrom
feat/improvements-on-generated-columns
Open

Feature: Handle read only columns - #437 followup#457
driv3r wants to merge 15 commits into
feat/handle-generated-columnsfrom
feat/improvements-on-generated-columns

Conversation

@driv3r

@driv3r driv3r commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

@shauns description

#437 + fixes.

Addressing @milanatshopify 's unresolved review

His comment (19 May, never resolved, posted after both approvals) said generated columns were missing from the WHERE clause of binlog updates, that this would be "slow, potentially", and that there was "an edge case where it will just fail" — with a test attached.

  • He was right, and it was worse than slow — it silently destroyed rows. Stripping generated columns from WHERE means SQL = matches under collation, so the remaining columns need not identify the row. On his exact table, a one-row source DELETE emptied a four-row target. main never had this. Fixed by restoring main's WHERE clause
    (26a513c, fc29762).
  • His performance concern, quantified. On a 10,000-row table of that shape: type=ALL, key=NULL, 100,001 rows read per binlog event, versus type=range, key=PRIMARY, 1 row after the fix. Roughly 42×.
  • His attached test is now in the suite, plus an end-to-end version through a real ferry (d68ac4b).
  • The over-match rule turned out wider than his example. It isn't "the primary key is generated" — it's "after removing generated columns, no subset of the rest forms a unique key". A composite-key table with three ordinary columns still in the predicate over-matches too.

Addressing the "validate it against shops" feedback

That bar isn't met yet — no shop has been moved. What replaced it:

  • Cross-version execution — 17 scenarios on MySQL 5.7.30, 8.0.32 and Percona 8.4.4, against an immutable snapshot of the final tree.
    - The PR's founding premise disproved. It claimed MySQL 8.0.23+ omits virtual columns from binlog row images. False on every supported version, in every row-image mode.
  • Blast radius measured, not asserted — a 27-column table across every major MySQL type produces byte-identical SQL on main, the branch and the final tree.

Test integrity — a finding nobody was looking for

  • Three of the branch's eight new tests had never executed. They sat below private in types_test.rb, invisible to Minitest, through a green CI run.
  • Two more ran but could not fail, for two independent reasons: assertions inside a callback handler are swallowed because Minitest::Assertion escapes a rescue StandardError, and a running datawriter masked the regression.
  • A sixth unfalsifiable assertion in interrupt_resume_test.rb, unreachable twice over.
  • This matters for @grodowski's approval specifically: it said the copy path was manually tested but the streaming path was deferred to the integration tests added in 2717bfd — which are the three that never ran. The binlog path had never been validated by anyone.
  • All six fixed and mutation-verified (c0415c2).

Robustness we found ourselves

  • A reachable panic. MySQL accepts a table whose columns are all generated when a STORED column is the primary key. Ghostferry accepted it, then panicked in strings.Repeat("?,", -1) part-way through a move. Now refused at schema load (1432ea2).
  • A second panic path in AsSQLQuery that the schema-load guard can't cover, since the column list comes from result-set names — reachable by a narrowing CopyFilter or an embedder. Now returns an error (674ec9d).
  • A pre-existing nil-pointer panic in StopTargetVerifier for embedders who defer it around Run (da8cfb0).
  • A false error message. The VIRTUAL pagination-key rejection claimed those values "are unavailable during data iteration" — untrue; they're selectable, orderable and indexable. Restriction kept, reasoning corrected.

Simplification — from devx pair-review, after five people had signed off

  • Removed a second index space on the INSERT path. The row was filtered into a new slice, then re-paired against the schema through a separate cursor. Mixing index spaces is the core risk in this whole change, and we'd left one in that didn't need to exist (0fe714e).
  • iterative_verifier.go reverted to comment-only against main — its rewrite existed solely to host a comment (f4d88c8).
  • buildStringMapForWhere is now byte-identical to main (fc29762, from your rename question).
  • −212 comment lines, 15% of everything we'd added (6e2ed7f).

What still ships unfixed, deliberately

  • Target schema drift — ghostferry never reads the target schema; a drifted expression in a UNIQUE key can silently lose whole rows. Documented, not guarded.
  • INVISIBLE generated columns — cannot be moved at all. Loud, identical on main, but an incompleteness in the feature, so it's in the description's first sentence.
  • Over-match on tables with no unique key — still present, identical on main, unrelated to generated columns.

shauns and others added 13 commits August 26, 2026 14:45
@shauns @plisandro
@driv3r @grodowski
Keep generated columns in binlog UPDATE and DELETE WHERE clauses
1836271 
An earlier commit on this branch removed generated columns from the WHERE
clause of replayed UPDATE and DELETE statements, on the stated grounds that
MySQL 8.0.23+ omits VIRTUAL generated columns from binlog row images. That
premise is false on every version we support. Row images on 5.7.30, 8.0.32 and
Percona 8.4.4 carry VIRTUAL and STORED values under FULL, and treat a generated
column exactly like an ordinary column of the same type under MINIMAL and
NOBLOB.

Removing them introduced silent data loss that main does not have. SQL `=`
compares under the column's collation rather than by value, so the columns that
remain need not identify the row. Given

    docs(doc TEXT,
         doc_hash BINARY(32) AS (UNHEX(SHA2(doc,256))) STORED,
         PRIMARY KEY (doc_hash))

holding 'cafe', 'café', 'CAFE' and 'cafe  ' under utf8mb4_unicode_ci — the
collation ghostferry's own compose files configure — replaying a one-row DELETE
as WHERE doc='cafe' destroys all four. Ghostferry exits 0 and reports success.
The destroyed rows' pagination keys never appear in a binlog event, so they are
never enqueued for re-verification.

The WHERE clause now names every column again, as it does on main.
buildStringMapForSet and the INSERT column list still exclude generated
columns, because MySQL rejects assignment to them with error 3105 — a hard
failure that neither INSERT IGNORE, UPDATE IGNORE nor REPLACE downgrades, on
any supported version. That asymmetry is deliberate and the comment above
buildStringMapForWhere exists to stop a future change tidying it away.

Restoring the column also answers the review objection about index usage. On a
10,000-row table of the shape above, MySQL 8.0.32 plans the stripped DELETE as
type ALL, key NULL, 10441 rows estimated, and the corrected one as type range,
key PRIMARY, 1 row.

Unsigned generated columns keep going through the normalisation loop in
NewBinlogDMLEvents. go-mysql decodes an unsigned generated column as a negative
signed integer, exactly as it does any unsigned column, and those values now
reach the WHERE clause.

test/go/generated_columns_test.go drives real binlog row images through
BinlogStreamer and executes the generated SQL against a real target. Reinstating
the removed filter fails five of its tests, including a four-row table emptied
by a one-row DELETE.

Co-authored-by: Lisandro Pin <lisandro.pin@proton.ch>
Co-authored-by: Leszek Zalewski <leszek.zalewski@shopify.com>
Co-authored-by: Jan Grodowski <jan.grodowski@shopify.com>
@shauns @plisandro
@driv3r @grodowski
Reject at schema load the table shapes ghostferry cannot copy
ef23be7 
Two table shapes reach the write path and fail there. Both are now refused when
schemas are loaded, with an error that names the table and the reason.

A table whose columns are all generated. MySQL accepts one, because a STORED
generated column may be a primary key:

    CREATE TABLE t (a BIGINT AS (1) STORED,
                    b BIGINT AS (2) STORED,
                    PRIMARY KEY (a));
    INSERT INTO t () VALUES ();     -- accepted, row is (1,2)

Such a table passes the pagination key check, reaches RowBatch.AsSQLQuery with
an empty column list, and panics in strings.Repeat("?,", -1) part-way through a
move. The new test asserts that MySQL accepts both the DDL and the row before
asserting that ghostferry refuses the table, so it cannot pass by accident if
MySQL's own behaviour changes.

A VIRTUAL generated column as the pagination key. The restriction already
existed on this branch; only its stated reason changes. It claimed VIRTUAL
values "are unavailable during data iteration", which is false — MySQL computes
them on read, they can be indexed, and EXPLAIN shows a range scan over an index
on one. The real reasons are that a VIRTUAL column cannot be a primary key, so
nothing guarantees the uniqueness a pagination key needs, and that an unindexed
one costs a full scan and a filesort per batch. The same false claim in the
neighbouring test comment is corrected too.

NonGeneratedColumnNames goes: it returns schema order, and the last caller
stopped wanting schema order when the INSERT column list moved to query-result
order to preserve the gh-285 fix.

Co-authored-by: Lisandro Pin <lisandro.pin@proton.ch>
Co-authored-by: Leszek Zalewski <leszek.zalewski@shopify.com>
Co-authored-by: Jan Grodowski <jan.grodowski@shopify.com>
@shauns @driv3r
Hold targetVerifierWg by value so StopTargetVerifier cannot panic
04195b6 
targetVerifierWg was allocated inside Run(). Callers that defer
StopTargetVerifier() before starting Run() — test/lib/go/integrationferry does
exactly this — reach Wait() on a nil pointer if anything returns early in
between. The bug predates this branch.

An earlier commit here guarded the nil. That hid the meaning of the nil: it
says the target verifier goroutine never started, and the guard let cutover
proceed quietly as though it had.

Holding the WaitGroup by value removes the question. Wait() on a zero-count
group is a genuine no-op, so a run that never started the verifier simply never
added to it. StopTargetVerifier now has no diff against main at all.
@shauns @driv3r @grodowski
Make this branch's new integration tests capable of failing
683cbaa 
Six of the tests on this branch could not detect the behaviour they name. Three
mechanisms, none of them visible to CI, all reporting green.

Three tests in types_test.rb sat below the `private` keyword. Minitest collects
only public instance methods, so test_binlog_insert/update/delete_with_generated
_columns never executed — not locally, and not in the green CI run on this PR.
The file reported 10 tests against 13 definitions. They are the branch's only
end-to-end coverage of the binlog path, and 2717bfd, the commit that added them,
is the commit cited in this PR's second approval as the reason to trust that
path. Moved above `private`. They now run, and mutating quotedColumnNames,
buildStringMapForSet and buildStringMapForWhere in turn fails one each.

Two tests in iterative_verifier_test.rb asserted inside an on_status handler.
Minitest::Assertion descends from Exception, not StandardError, and the callback
server rescues StandardError, so a failed assertion never reached the test. The
surviving assertion checked only that verification ran, which the integration
ferry reports unconditionally. Assertions moved after ghostferry.run.

The same two tests were also masked by a second, unrelated cause: with the
datawriter running, a divergent generated column stops every replayed UPDATE
matching, the ordinary data column goes stale, and the verifier reports the
table for that instead. The tests passed whether or not the verifier looked at
generated columns at all. They now run against a static source, so the generated
column is the only thing that can differ. Excluding generated columns from
IterativeVerifier.columnsToVerify now fails both.

One assertion in interrupt_resume_test.rb could not fail either. Its enclosing
condition compared binlog filenames while the test containers run
max_binlog_size=4096 and the test writes ~2000 rows, so the log rotates and the
guard is almost never true. Replaced with a rotation-safe comparison of the
(file, position) pair. Removing the position update from the inline verifier now
fails it.

inline_verifier_test.rb regains the four exact-checksum assertions that were
deleted when the default test table gained generated columns. The values are
derived from the documented RowMd5Query expression, not copied from output, and
they now cover the VIRTUAL and STORED columns too. Dropping generated columns
from the fingerprint returns the first of them to 7dfce9db8fc0f2475d2ff8ac3a5382
e9 — precisely the value this branch deleted — and fails the test.

Two comments in types_test.rb stated the false premise this branch was built on.
One documented the removed WHERE-clause behaviour as correct, directly above the
test that guards against it.

Co-authored-by: Leszek Zalewski <leszek.zalewski@shopify.com>
Co-authored-by: Jan Grodowski <jan.grodowski@shopify.com>
@shauns @driv3r
Add end-to-end coverage for the binlog WHERE-clause over-match
cb38b9b 
Drives a real ferry over a table whose primary key is a STORED generated column,
with sibling rows that differ only in ways utf8mb4_unicode_ci treats as equal.

Both tests fail against 53d68b0, the branch tip before the WHERE-clause fix. The
DELETE case is the one that matters: one row deleted on the source, three gone
on the target, ghostferry exiting 0 and reporting success.

The fixture obeys a constraint that is easy to miss. Over-match needs the
siblings to be indistinguishable on every non-generated column. A fixture with
any extra distinguishing column passes against the broken code and proves
nothing.
@shauns @pi-mono @driv3r
Update CHANGELOG for 1.4.0
4d1ed9a 
Co-authored-by: AI (Pi/Claude Opus 5) <noreply@pi.dev>
@shauns @pi-mono @driv3r
Build INSERT values from the full-width row, not a filtered copy
b2734af 
BinlogInsertEvent.AsSQLString filtered the row into a fresh slice, then handed
that slice to buildStringListForValues, which had to re-pair it against
table.Columns through a second cursor. Two index spaces where one will do, and
subtle enough that the misalignment needed its own regression test.

buildStringListForValues already walks table.Columns and already skips generated
indexes, so it can index the unfiltered row with the same counter. The emitted
SQL is identical and every existing test passes unchanged.

The old comment justified the filtering as keeping the hot path off the
allocator. It did not: the caller allocated per row anyway. FilterGeneratedColumns
OnRowData had no callers left, so it and the length check it duplicated are gone.

Mixing index spaces is the failure this whole change has to avoid. There is now
one on the binlog INSERT path.

Co-authored-by: AI (Pi/Claude Opus 5) <noreply@pi.dev>
@shauns @driv3r
Return an error from AsSQLQuery when no column can be written
6b413b7 
strings.Repeat("?,", -1) panics. nonGeneratedColumnIdxs can legitimately be
empty, where the previous expression derived its length from the schema and
could not be.

LoadTables refuses a table whose columns are all generated, but that guard is
not sufficient here. nonGeneratedColumnIdxs is built from result-set column
names, so a CopyFilter that narrows ColumnsToSelect to generated columns reaches
this line, as does an embedder that populates Ferry.Tables directly. Ghostferry
is consumed as a library.

AsSQLQuery already returns an error. A panic part-way through a move is strictly
worse than the signature it already offers.
@shauns @driv3r
Restore the fast path in columnsToVerify
0941ad5 
The rewrite dropped the early return for a table with no ignored columns, so
every call copied the whole column slice into a nil slice with no reserved
capacity. columnsToVerify runs twice per verification batch.

Generated columns were always included here: table.Columns contains them and the
early return handed them straight back. The rewrite therefore bought only the
placement of a comment. The comment stays; the copy goes.

iterative_verifier.go is now comment-only against main.
@shauns @pi-mono @driv3r
Record the panic fixes and new exported API in the CHANGELOG
b93bb71 
Co-authored-by: AI (Pi/Claude Opus 5) <noreply@pi.dev>
@shauns @pi-mono @driv3r
Say why the generated column comes first in these fixtures
042fab7 
Both tests only detect the bug they exist for because 'gen' sits before 'u8'
and before 'payload'. The two index spaces diverge only after the first
generated column, so with 'gen' last every index coincides and the mutation the
tests guard against survives them.

The note sits on the column slice, which is where the temptation is, and it
covers the test name too, since "GeneratedColumnBeforeJSON" reads like a
description of the fixture rather than a constraint on it.

Co-authored-by: AI (Pi/Claude Opus 5) <noreply@pi.dev>
@shauns @pi-mono @driv3r
Cut the commentary down to what a future change could break
01b02b8 
The comments had grown to the point of not being read: dml_events.go carried
61 added comment lines against 30 of code. A comment nobody reads protects
nothing.

Kept where deleting it would let a plausible tidy-up reintroduce a bug — the
WHERE/SET asymmetry, why generated columns still need unsigned normalisation,
why the INSERT column list follows result order, why the empty-column guard is
reachable, and why two test fixtures put the generated column first. Each is
now a few lines: the rule, the reason, and a pointer to the test that proves it.

Cut everything else. The worked examples and the evidence live in the pull
request, which cites where each was executed. Narration of what earlier commits
on this branch got wrong belongs in the history, not in the source that
outlives it.

One comment was stale as well as long: the JSON-casting test still described
buildStringListForValues as receiving a pre-filtered row, which stopped being
true when the second index space was removed.

Also `var wg sync.WaitGroup` over `&sync.WaitGroup{}`, and `to_h` over
`each_with_object({})` to match its neighbour.

Comments only, apart from those two. No behaviour, no assertion, no exported
identifier changed. 476 Go tests and 84 Ruby tests green before and after.

Co-authored-by: AI (Pi/Claude Opus 5) <noreply@pi.dev>
@shauns @pi-mono @driv3r
Give buildStringMapForWhere back main's signature
8554d8c 
Once the WHERE clause stopped filtering generated columns, the function no
longer needed the TableSchema it had been given in order to call
IsColumnIndexGenerated. It only reads Columns[i].

The body is now byte-identical to main. The only thing this change leaves on
the function is the comment saying why it must stay that way.

Co-authored-by: AI (Pi/Claude Opus 5) <noreply@pi.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@shauns shauns

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@driv3r @shauns