Add opt-in auto re-sync on schema incompatibility - #97
Conversation
IsBinlogsExpirationError drops the saved position and lets the next iteration run a historical sync, so the connection recovers on its own. The schema-incompatibility path added in #89 returns an error instead, so a deploy that changes the layout of a replicated table stalls the connection until someone triggers a historical re-sync by hand. Until they do, every sync retries from the same position and fails on the same binlog event. Because a Read() error returns out of the whole Sync handler rather than skipping the affected table, one table in this state stalls every table in the connection. Unlike expired binlogs the operator has a real choice here, since the data is still readable once the projection is rebuilt, and an automatic historical sync costs monthly active rows. So this is opt-in via a new auto_resync_on_schema_change configuration field, defaulting to false, which preserves the existing behaviour exactly. With it enabled the cursor is reset and the stream recovers unattended. The cursor carries SCHEMA_INCOMPATIBILITY_ERROR rather than reusing BINLOG_EXPIRATION_ERROR, so the two causes stay distinguishable downstream and an unexpected historical sync remains attributable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
e63d6c9 to
5738240
Compare
|
Validated end-to-end against a live PlanetScale branch rather than mocks. Setup reproduces the production failure this is aimed at: capture a cursor, write rows, apply a trailing
The 7 rows are the entire table, since the reset drops the position and the copy phase re-reads it. That is the intended behaviour and also a fair illustration of why this is opt-in rather than default: the recovery is a full historical sync, with the monthly-active-row cost that implies. Worth noting the default path is unchanged and still surfaces the error with recovery guidance, so this is purely additive for anyone who doesn't opt in. For completeness, the same harness was used to reproduce all four schema-incompatibility shapes across direct |
The configuration form declares a field name and SourceFromRequest reads a map key. Nothing tied the two together, so if they ever drifted the toggle would fail silently: an operator enables it, the connector keeps the old behaviour, and no error is raised anywhere. Assert the round trip instead of the two literals independently -- read the field name out of the form, feed it through SourceFromRequest, and check the parsed value. Also covers the absent-key default staying false, so the pre-existing behaviour is pinned, and a non-boolean value being rejected rather than silently ignored. Verified the test fails when the parser's key is changed, so it guards the drift it claims to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fivetran tells us per table whether new columns should be synced
(TableSelection.include_new_columns), and the SDK guide makes acting on it
the connector's job. We never read the field: includedColumns() looks only at
TableSelection.columns, so a column added after a connection was set up is
never named in the VStream projection and its values never arrive. No error
is raised, syncs report success, and only a historical re-sync recovers.
Widening the projection is not something we can simply do up front, though.
Naming a column that did not exist at the replay position is exactly what
produces "column X not found in table Y" against a pre-DDL TABLE_MAP. So the
stream starts on Fivetran's selection as-is and rebuilds the projection only
once it observes the DDL event for the table.
Resuming there is safe because the cursor is already past the schema change:
vstreamer emits a GTID event carrying EncodePosition(vs.pos) immediately
before the DDL event, vs.pos already includes the DDL's own GTID, and vtgate
converts that GTID to the VGTID we consume in place. Only post-DDL row events
are replayed, so their TABLE_MAP carries the new column and the plan resolves
without help from vttablet's schema historian -- this needs no
--track-schema-versions.
The serializer needed the same treatment. columnSelection and columnWriters
are both built from TableSelection.columns, so a widened projection would
have been discarded on the way out, and a missing writer is a hard error
rather than a silent skip.
Both opt-ins must be set: the new propagate_new_columns source setting and
Fivetran's per-table include_new_columns. A column Fivetran explicitly
deselected is present-and-false in the map and stays excluded; only columns
it never named at all are adopted. The setting defaults to false and is
labelled experimental.
Scope, verified against a live branch with tracking off rather than inferred:
- Fixes new columns never arriving. The pre-DDL row is delivered on the
narrow projection and the post-DDL row carries the new column's value.
- Does not change hard stalls. When Fivetran's selection already names the
new column, filterExistingColumns keeps it (it exists live), so the first
window fails before reaching the DDL and the gate never fires. The error
is identical with the setting on or off and is still matched by
IsVStreamSchemaIncompatibilityError, so recovery stays with #96/#97.
- Does not change dropped columns on a lagging cursor. The width check
fires on the pre-DDL row before the DDL is reached, so that case still
depends on the historian. rebuildProjection drops vanished columns for
coherence, which matters only for a drop landing mid-Read.
Note for anyone tempted to loosen the projection instead: falling back to
SELECT * silently corrupts data across a mid-table ADD. analyzeExprs copies
Table.Fields positionally for a StarExpr and never calls findColumn, so
nothing catches the skew from the st.Fields[:len(tm.Types)] truncation. On a
lagging cursor with ADD COLUMN newcol AFTER a over (id, a, b, c), * delivered
b's value as newcol and c's value as b with no error at all. The explicit
projection is a guardrail, not the defect.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Problem
IsBinlogsExpirationErrordrops the saved position and lets the next loop iteration run a historical sync, so the connection recovers on its own. The schema-incompatibility path added in #89 returns an error instead.So a deploy that changes the layout of a replicated table stalls the connection until a human triggers a historical re-sync. Until they do, every subsequent sync retries from the same saved position, hits the same undecodable binlog event, and fails again. And because a
Read()error returns out of the wholeSynchandler rather than skipping the affected table, one table in this state stalls every table in the connection.In the incident that prompted this, replication was stalled for hours across six tables before anyone noticed, and the eventual fix was a manual full re-sync of the entire connection.
Why this is opt-in rather than just mirroring the binlog path
Unlike expired binlogs, the operator has a real choice here: the data is still readable from the saved position once the projection is rebuilt, and an automatic historical sync costs monthly active rows. #73 shows MAR is already a consideration in this connector, so silently re-syncing a large table isn't a call this PR should make on a user's behalf.
New
auto_resync_on_schema_changeconfiguration field, defaulting tofalse, which preserves current behaviour exactly — the error is surfaced with recovery guidance and the sync waits for an operator-triggered re-sync. The message now also mentions the flag, so anyone hitting this finds the option.With it enabled, the cursor is reset and the stream recovers unattended.
It follows the existing convention for booleans here (
use_replica,treat_tiny_int_as_boolean): a["true","false"]dropdown parsed withstrconv.ParseBool, so an absent key leaves itfalsewith no special-casing. SDK v2 also has aToggleFieldwhich would render more naturally — happy to switch if you'd prefer to start migrating.Change
auto_resync_on_schema_changeform field, config parsing, andPlanetScaleSourcefieldPositionandLastKnownPk, mark the cursor, andcontinue, exactly as the binlog-expiration branch does — the copy phase reads the live table, so it re-anchors past the DDLSCHEMA_INCOMPATIBILITY_ERRORcode rather than reusingBINLOG_EXPIRATION_ERROR, so the two causes stay distinguishable downstream and an unexpected historical sync remains attributableTests
TestRead_ReturnsVStreamSchemaIncompatibilityErrorsasserted the old contract, so it becomes two tests covering both sides of the flag:TestRead_SchemaIncompatibilityResetsCursor— opt-in enabled; mirrorsTestRead_BinlogExpirationReturnsResetCursor. Verifies the position is cleared,LastKnownPkis nil, and the code and message are set. Verified to fail against the currentRead().TestRead_SchemaIncompatibilityWithoutOptInReturnsError— default; the original contract, plus an assertion that the message points at the flag.TestRead_ReturnsGenericNonTimeoutErrorsandTestRead_BinlogExpirationReturnsResetCursorare unchanged and still pass, confirming no over-reach into other error paths.TestVStreamSchemaIncompatibilityReturnsFailedPreconditioninsync_test.gomocksReaddirectly, so it still exercises the handler mapping and is untouched.Full
go test ./...passes,cmd/...included.Relationship to #96
Independent, and they compose. #96 determines what counts as a schema incompatibility — it currently misses three of the five inner causes vstreamer can raise. This PR decides what to do once one is detected. Either can merge first.
🤖 Generated with Claude Code