fix(datasync): per-table --force wipe, source identity in checkpoints, schema check for pre-existing targets#1043
Draft
morgo wants to merge 3 commits into
Draft
fix(datasync): per-table --force wipe, source identity in checkpoints, schema check for pre-existing targets#1043morgo wants to merge 3 commits into
morgo wants to merge 3 commits into
Conversation
--force with no resumable checkpoint executed DROP DATABASE IF EXISTS on the target. But sync's own fresh-run model tolerates a target database shared with unrelated tables (checkTargetEmpty only validates tables named after source tables), so --force could destroy tables the sync never owned - e.g. an unrelated reporting table sharing the target database. Scope the wipe per-object, mirroring move's wipeTargets: drop only the target copies of the source tables plus the sync checkpoint table, then proceed as a fresh sync. The decision moves from ensureTargetDatabase (no-database admin connection, pre-open) into setup(), after source table discovery, where the table list and the target pool are available; per-table DROPs do not need the admin connection that DROP DATABASE did, so forceTargetResumable's connection dance is gone too. TestSyncForce now pins that a foreign table survives the force wipe, and TestSyncForcePreservesForeignTables covers the shared-database recovery end to end (stale non-resumable checkpoint + junk data: foreign table survives, sync-owned tables recreated from the source). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The checkpoint persisted a bare file:pos string, and resume only checked that a binlog file with that NAME still exists. Binlog file names are sequential on every server, so after an Aurora/RDS failover behind a stable endpoint, a replica promotion, or a rebuilt source, the position usually "exists" on the new server too - and StartFromPosition would succeed there, silently skipping or replaying the wrong events. GTID mode is immune; the default file:pos mode was not. Wrap the checkpointed position in a small versioned JSON payload (inside datasync's own Position field; the shared checkpoint schema is unchanged) carrying @@server_uuid - which changes across all of those transitions - plus the source address for diagnostics. On resume with the built-in file:pos client, hard-error when the recorded server_uuid does not match the current source's, pointing at --force (fresh sync) or --gtid (failover-safe). GTID and injected change.Source positions are unwrapped without verification. The payload decodes strictly (unknown fields rejected + version check) so an injected source's opaque position can never be misread as ours. Old checkpoints without identity are unverifiable and now refuse to resume in file:pos mode unless --force is given; hasResumableCheckpoint applies the same gate so --force wipes and starts fresh instead of resuming into the same error. This is a deliberate breaking change for an EXPERIMENTAL feature. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A fresh sync accepted any pre-existing target table as long as it was empty (checkTargetEmpty), with zero schema comparison. move closed exactly this hole in its target_state check: the dangerous case is a primary-key collation difference (e.g. source utf8mb4_bin vs a target pre-created with the default utf8mb4_0900_ai_ci), where the REPLACE/INSERT IGNORE write path silently collapses case-distinct rows and the continuous checksum then never converges. Port the same comparison to datasync's fresh-run path by lifting move's schemaDiff into the shared pkg/statement as statement.SchemaDiff (it is pure CREATE TABLE canonicalization/diffing, which is that package's domain), updating move's three call sites (target_state, resume_state, source_schema_consistency) to use it. checkTargetEmpty now compares SHOW CREATE TABLE of every pre-existing empty target table against its source table and errors before any copy starts; identical pre-created tables (the declarative pre-created-schema workflow) still pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes three verified safety gaps in the EXPERIMENTAL
sync(datasync) runner, one commit per gap.Gap 1:
--forcedropped the entire target database, including tables the sync never ownedProblem. When no resumable checkpoint existed,
--forceexecutedDROP DATABASE IF EXISTSon the target. But sync's own fresh-run model tolerates foreign tables in the target database (checkTargetEmptyonly validates tables named after source tables), and move's equivalent wipe is per-table. A target database shared with, say, an unrelated reporting table meant--forcedestroyed it.Fix. The wipe is now per-object, mirroring move's
wipeTargets: only the target copies of the source tables plus the sync checkpoint table are dropped, and the database is still ensured to exist (CREATE DATABASE IF NOT EXISTS). The decision moved fromensureTargetDatabase(pre-open, no-database admin connection) intosetup()after source-table discovery, which also removesforceTargetResumable's extra connection dance.--forcehelp/doc text updated.Testing.
TestSyncForcePreservesForeignTables: a target database holding a foreign table (with rows), a stale non-resumable checkpoint (present, empty watermark), and junk in a source-named table.--forcerecovers: the foreign table and its rows survive, the source-named tables are recreated with exactly the source's data, and the checkpoint is recreated. Pre-fix, the foreign table was destroyed.TestSyncForcenow pins that a non-source table survives the force wipe.Gap 2: checkpoints recorded no source identity — resume against a replaced source replayed a colliding file:pos silently
Problem. The checkpoint persisted a bare
binlog.NNNNNN:offset, and resume only validated that a binlog file with that name exists (pkg/change/binlog.go). Binlog file names are sequential on every server, so after an Aurora/RDS failover behind a stable endpoint, a replica promotion, or a rebuilt source,StartFromPositionsucceeded on a different server's file and silently skipped or mis-replayed events. GTID mode is immune; the default file:pos mode was not.Fix. The position is now stored wrapped in a small versioned JSON payload inside datasync's own
Positionfield (the sharedpkg/checkpointschema is unchanged):{"v":1,"position":...,"server_uuid":...,"source_addr":...}.@@server_uuidis the identity (it changes across failover/promotion/rebuild); the address is diagnostics only. On resume with the built-in file:pos client, aserver_uuidmismatch is a hard error pointing at--force(fresh sync) or--gtid(failover-safe). GTID and injectedchange.Sourcepositions are unwrapped without verification, and the payload decodes strictly (unknown fields rejected + version check) so an injected source's opaque position can never be misread as ours.hasResumableCheckpointapplies the same gate, so--forcewipes and starts fresh instead of resuming into the same error.Breaking change (deliberate, experimental feature): old checkpoints without identity are unverifiable, so a file:pos resume now refuses them with a clear error unless
--forceis given (or--gtidis used).Testing.
TestSyncResumeSourceIdentity: a real run records the payload (asserted to carry the source's actual@@server_uuidand a non-empty position); a direct SQLUPDATEswaps in a wrongserver_uuid(inner position untouched and still valid on this server — pre-fix this resume succeeded silently) and resume hard-errors; restoring the correct uuid resumes normally with data intact; rewriting the position to the legacy bare form errors with the unverifiable-checkpoint message;--forcethen recovers with a fresh copy.TestSyncPositionEncodeDecodeunit-tests the codec (round-trip, legacy/foreign-JSON/wrong-version fallbacks).Gap 3: pre-existing empty target tables were accepted with zero schema comparison
Problem.
createTargetTablesskips tables that already exist on the target, and the fresh-run gate (checkTargetEmpty) validated only emptiness. move closed exactly this hole withschemaDiff(pkg/move/check/target_state.go), whose comment names the danger: a primary-key collation difference makesREPLACE/INSERT IGNOREsilently collapse case-distinct rows, and the continuous checksum then never converges.Fix. move's
schemaDiffis lifted into the sharedpkg/statementasstatement.SchemaDiff(it is pure CREATE TABLE canonicalization/diffing — that package's domain); move's three call sites (target_state,resume_state,source_schema_consistency) now call it there. datasync's fresh-run path comparesSHOW CREATE TABLEof every pre-existing empty target table against its source table via the same function and errors — with a runnableALTERin the message — before any copy starts. Identical pre-created tables (the declarative pre-created-schema workflow) still pass.Testing.
TestSyncFreshTargetSchemaMismatch: a pre-created empty target table with a different column collation is rejected before any row is copied (asserts zero rows and no checkpoint table were produced); an identical pre-created table passes and the copy proceeds.TestSchemaDiff*unit tests moved topkg/statementwith a new collation-mismatch case; move'sTestTargetStateCheck*,TestResumeStateCheck*andTestSourceSchemaConsistencyCheckstill pass against the relocated function.Test runs
All against MySQL 8.0.45 (GTID enabled), targeted per the repo's shared-MySQL conventions:
./pkg/datasync/— full package test list (E2E, initial copy, copy-only, status task, resume ×4, force ×2, identity ×2, schema check, legacy default, defer-secondary-indexes ×2, continuous checksum ×2, validate): all pass./pkg/datasync/with-raceon the three new integration tests + codec test: pass./pkg/statement/,./pkg/move/check/(target_state/resume_state/source_schema_consistency): passgolangci-lint run(uncapped) onpkg/datasync,pkg/statement,pkg/move: 0 issues🤖 Generated with Claude Code