Skip to content

Add opt-in auto re-sync on schema incompatibility - #97

Open
orware wants to merge 2 commits into
mainfrom
feat/schema-incompat-cursor-reset
Open

Add opt-in auto re-sync on schema incompatibility#97
orware wants to merge 2 commits into
mainfrom
feat/schema-incompat-cursor-reset

Conversation

@orware

@orware orware commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Problem

IsBinlogsExpirationError drops 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 whole Sync handler 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_change configuration field, defaulting to false, 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 with strconv.ParseBool, so an absent key leaves it false with no special-casing. SDK v2 also has a ToggleField which would render more naturally — happy to switch if you'd prefer to start migrating.

Change

  • auto_resync_on_schema_change form field, config parsing, and PlanetScaleSource field
  • when enabled, clear Position and LastKnownPk, mark the cursor, and continue, exactly as the binlog-expiration branch does — the copy phase reads the live table, so it re-anchors past the DDL
  • the cursor carries a distinct SCHEMA_INCOMPATIBILITY_ERROR code rather than reusing BINLOG_EXPIRATION_ERROR, so the two causes stay distinguishable downstream and an unexpected historical sync remains attributable

Tests

TestRead_ReturnsVStreamSchemaIncompatibilityErrors asserted the old contract, so it becomes two tests covering both sides of the flag:

  • TestRead_SchemaIncompatibilityResetsCursor — opt-in enabled; mirrors TestRead_BinlogExpirationReturnsResetCursor. Verifies the position is cleared, LastKnownPk is nil, and the code and message are set. Verified to fail against the current Read().
  • TestRead_SchemaIncompatibilityWithoutOptInReturnsError — default; the original contract, plus an assertion that the message points at the flag.

TestRead_ReturnsGenericNonTimeoutErrors and TestRead_BinlogExpirationReturnsResetCursor are unchanged and still pass, confirming no over-reach into other error paths. TestVStreamSchemaIncompatibilityReturnsFailedPrecondition in sync_test.go mocks Read directly, 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

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>
@orware
orware force-pushed the feat/schema-incompat-cursor-reset branch from e63d6c9 to 5738240 Compare August 3, 2026 20:54
@orware orware changed the title Reset cursor and re-sync on schema incompatibility Add opt-in auto re-sync on schema incompatibility Aug 3, 2026
@orware

orware commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

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 ADD COLUMN, write more rows, then resume from the now-stale cursor with the new column in the projection. Schema-version tracking off, matching the configuration the original incident occurred under. Same database, same data, same DDL — the only variable is the flag.

auto_resync_on_schema_change=false (default) =true
error column v97_false not found in table repro none
rows delivered 0 — sync stalls, needs manual re-sync 7 — full table, unattended

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 ALTER and deploy requests, with and without schema tracking — 16 cells, and every deploy-request result was identical to its direct-ALTER equivalent. Details in the #96 thread.

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>
orware added a commit that referenced this pull request Aug 5, 2026
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>
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.

1 participant