checkpoint: persist the one resume row per target with a guarded upsert and a typed load - #141
Conversation
7d5fc03 to
3796a08
Compare
…rt and a typed load (ST-1, ST-2) Add pkg/checkpoint's Store over a pkg/dbconn pool. Ensure creates the engine-owned pgsprite.pgsprite_checkpoint table on first use under the table's own advisory key, so concurrent first use serializes. Save is a single INSERT ... ON CONFLICT DO UPDATE whose WHERE clause refuses to overwrite a row written for another format_version or another source/target fingerprint, and reports the refusal as a typed *IncompatibleError. Load distinguishes ErrNotFound (a completed read with no row, the only outcome a caller may start fresh from) from *IncompatibleError and from a transient failure, which it retries a bounded number of times through an injected sleep before returning an error that is neither. Delete is the explicit, idempotent fresh start. The watermark column is NULL for a zero copier.Watermark; the LSN column is pg_lsn, written from its text form and read back through decode.ParseLSN; phase is the stable Phase name, so an operator reading the row sees "copying", not a number. dbconn.Retryable now treats 57P01/57P02/57P03 as transient. decode gains ParseLSN. Integration proof: a copy pinned mid-chunk is cancelled, the watermark saved, loaded, and a new Copier started from it converges on the shadow with exactly the rows the first run did not land.
57P01/57P02/57P03 are what a backend reports when the server ends the session from outside it, which is how a failover looks from the client. A write interrupted that way has an ambiguous outcome, so the engine-wide dbconn.Retryable must not repeat it. Store.Load is a read, so repeating it is safe: the checkpoint package widens the classifier for its own retry loop instead of the engine doing so for everyone.
3796a08 to
9b5bd63
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
🤖 1/2: adversarial correctness review of 0 blocking, 5 non-blocking. The core of the store is right. The primary key on Non-blocking1. The guard keys on the statement, not on the run that holds the table, so a stale writer of the same statement overwrites newer state. Two runs of the same Every other write path in LK-1's Enforced list (shadow build, drop and inspect, the copier, the verifier) takes the
Probe: a stale save of the same statement regresses the row (passes on
|
| Mutant | Caught by |
|---|---|
Guard AND → OR (source or target line) |
TestSaveRefusesToOverwriteAnotherStatementsRow |
| Zero-rows branch never taken | TestSaveRefusesToOverwriteAnotherStatementsRow |
Refused save skips the IncompatibleError |
TestSaveRefusesToOverwriteAnotherStatementsRow |
Save skips validate |
TestSaveRefusesAnInvalidCheckpointBeforeTheDatabase |
UpdatedAt from the record, not the clock |
TestSaveThenLoadRoundTripsTheRecord, TestSaveUpsertsTheOneRowPerTarget |
No Commit |
8 integration tests |
| No primary key | 9 integration tests |
Delete deletes nothing |
TestDeleteIsIdempotent, TestSaveRefusesToOverwriteAnotherStatementsRow |
SET drops watermark / slot_name |
TestSaveUpsertsTheOneRowPerTarget (+ resume test) |
SET drops publication_name / shadow_table |
survives (finding 4) |
Ensure without the advisory lock |
survives 10/10 (finding 3) |
ErrNoRows not mapped to ErrNotFound |
TestLoadReportsNoRowAsNotFound, TestLoadReportsNotFoundForATargetWithNoRow, TestDeleteIsIdempotent |
Load skips the incompatibility check |
both …AsIncompatible integration tests |
readRetryable drops 57P01 |
unit classifier tests + TestLoadRetriesAcrossATerminatedBackend |
| Retry every error | TestRetryTransientReturnsPermanentErrorAtOnce, …ReturnsNoRowsAtOnce, TestLoadReportsNoRowAsNotFound |
| Flat backoff | TestRetryTransientExhaustsItsAttempts, …RetriesThroughTransientErrors |
| Bad LSN decoded anyway | TestRowCheckpointRefusesValuesSaveCannotHaveWritten |
| Source/target mismatch order swapped | TestRowIncompatibilityOrdersFormatThenSourceThenTarget |
parsePhase misses the last phase |
TestParsePhaseInvertsString |
validate skips the shadow table |
TestCheckpointValidate |
ParseLSN high half widened to 64 bits |
survives (finding 4) |
Delete swallows its error |
survives (no test of Delete without Ensure) |
| Refused-save read-back via the pool instead of the transaction | survives (behaves the same under the tests) |
This review was generated by Claude Code (claude-opus-5-5).
|
🤖 2/2: OSS adoption and integration ease, at For an importer, the surface is in good shape. 1. A least-privilege deployment cannot pre-provision the table: PostgreSQL checks the database-level Managed PostgreSQL shops that keep 2. Failing closed is right, since this must never read as 3. The row format is versioned, but the table's shape is not.
This review was generated by Claude Code (claude-opus-5-5). |
aparajon
left a comment
There was a problem hiding this comment.
🤖 Approving 9b5bd63 with 0 blocking findings. The guarded upsert keeps one row per target, and a refused save leaves the row untouched (ST-1). Load keeps resume, no row, incompatible and a failed read apart (ST-2). I checked both on real PostgreSQL and with mutations. The 1/2 comment has 5 non-blocking findings. The main one: Save and Delete don't take the table lock, so a stale writer of the same statement can overwrite newer state, and the signature is cheapest to change before the orchestrator calls it. The others: Ensure trusts a pgsprite schema another role owns; the concurrency test passes without the advisory lock; two upsert columns go unchecked; and the retry docs drifted. The 2/2 comment has 3 non-blocking adoption notes.
This stamp was left by Claude Code (claude-opus-5-5).
…oint * origin/main: schemachange: swap the verified shadow in under a bounded lock (D5, D8, D9, LK-2, LK-4) (#139) # Conflicts: # docs/copy-and-swap-design.md
…, type the missing table Save and Delete now take the target's *dbconn.TableLockSession, run under its Bind context, and confirm from their own transaction that the session still holds the table (LK-1). Two runs of the same statement share fingerprints, so the guard alone could not stop a run whose lock had moved to another engine from stamping stale state over the newer row. Delete becomes a compare-and-delete on the row's Identity (format version and both fingerprints) — the one the caller was shown as IncompatibleError.Stored or as a loaded Checkpoint's Identity(). A row another engine wrote in the meantime survives and comes back as a new IncompatibleError; no row is still the idempotent success. Ensure reads the owners of the engine schema and the checkpoint table from pg_catalog under the advisory key before creating anything, and creates only what is absent. A schema or table another role owns, or a relation under the table's name that is not a plain table, is refused with ErrForeignObject instead of adopted — a foreign trigger would otherwise run with the engine's privileges on every Save. Because CREATE SCHEMA checks database CREATE before existence, this also lets a deployment pre-provision both objects for an engine role that never holds CREATE; docs/engine-role.md describes that path. Load, Save and Delete report a database where Ensure never ran as ErrTableMissing, distinct from ErrNotFound and still carrying the server's 42P01. Tests: Ensure waits behind an uncommitted first create (fails without the advisory lock), accepts a pre-provisioned table without database CREATE, refuses a foreign-owned schema, a foreign-owned table, and a view under the table's name; Save and Delete refuse a missing lock and a lock another backend holds; Delete leaves a row the caller was not shown; the upsert asserts publication_name and shadow_table; ParseLSN pins the 32-bit high half.
|
🤖 Adversarial review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — pull/141, follow-up commit Seven of the eight non-blocking findings are fixed in this PR; one (table-shape versioning) is tracked as an internal follow-up. The reviewer's
Source: block/pg-sprite#141, review comments 5986357613 and 5986358273, review 5409090188 at head |
pkg/checkpointgains aStorethat keeps the one resume row per target in an engine-owned table, writes it under the target's table lock with a guarded upsert, and loads it with a typed outcome so a restarted run knows whether to resume, refuse, or start fresh.Why
Build, copy, verify, gate, and cutover are landed, but nothing survives a process restart. D3 in
docs/copy-and-swap-design.mdrequires a single checkpoint row per target in the target database; ST-1 / ST-2 indocs/invariants.mdrequire that a restart never resumes from a row written for a different statement and never mistakes a failed read for "no checkpoint"; LK-1 requires that every write for a target happens under the table lock session that keeps a second engine off it. This is that storage layer; the orchestrator that drives phase transitions is a later PR.What
Ensuremakespgsprite.pgsprite_checkpointusable once per database. Under the table's advisory key it reads the owners of the schema and the table frompg_catalog, refusesErrForeignObjectif either exists under another role or the table's name is taken by a relation that is not a plain table, and creates only what is absent. A deployment that keeps database-levelCREATEaway from the engine role can therefore pre-provision both objects for it (docs/engine-role.md).Save(ctx, lock, cp)writes the target's one row withINSERT … ON CONFLICT DO UPDATEguarded onformat_versionand both model fingerprints; zero rows affected means another statement's row is in the way, and Save returns*IncompatibleError(carrying the stored row'sIdentity) instead of overwriting it. The write runs underlock.Bindand confirms the lock from its own transaction, as the copier does per chunk.Loadreturns exactly one ofCheckpoint(resume),ErrNotFound(a completed read found no row),*IncompatibleError(another format or statement),ErrTableMissing(Ensurenever ran here; wraps the server's42P01), or a plain error after bounded retries.Delete(ctx, lock, schema, table, stored)is the explicit fresh start: a compare-and-delete of the row the caller was shown (IncompatibleError.StoredorCheckpoint.Identity()). A row that changed in the meantime survives and comes back as a new*IncompatibleError; no row is the idempotent success.readRetryable: everythingdbconn.Retryabletreats as transient, plus a session the server ended from outside it (57P01/57P02/57P03, how a failover looks from the client). Only the read widens the set; a write those codes interrupt has an ambiguous outcome and is not repeated.pkg/schemachange's model digests, not SQL text;phaseis stored by name;updated_atcomes from the injected clock.Before / after
One run of
ALTER TABLE orders DROP COLUMN noteon a 2000-row table, killed after the copier reached key 1000. The only thing that changed is that the watermark now outlives the process; the copier, the shadow builder, and the preflight are untouched.If the new process runs a different statement against the same table,
Loadreturns*IncompatibleError{Mismatch: MismatchTarget, Stored: …}andSaverefuses to overwrite the row;Deletewith thatStoredidentity is how the operator starts fresh. If the first engine is still alive but lost the table lock, its nextSaveis refused asErrInvariantViolation(LK-1) rather than regressing the row. If the read itself fails after the bounded retries, the error is neitherErrNotFoundnor incompatible, so a blip never triggers a fresh start.Decisions worth a look
readRetryableadds57P01/57P02/57P03on top ofdbconn.RetryableforLoad;pkg/dbconnis unchanged, so no write path anywhere retries through those codes. ST-2 indocs/invariants.mdrecords the split.current_user. A schema or table owned by a role the engine is merely a member of is still refused. This keeps the rule the same asDropShadow's and makes the pre-provisioning recipe one line (ALTER … OWNER TO <engine>); loosening to membership can come later if a deployment needs it.format_versionis part of the match.format_versionprotects a row's meaning; the first column change will needEnsureto evolve the table and a typed answer for a table from another version. Tracked as an internal follow-up.Tests
TestEnsureWaitsForAConcurrentFirstCreateholds a first engine's create open and proves the second waits on the advisory key (fails with23505when the lock is removed).TestEnsureAcceptsAPreProvisionedTableWithoutDatabaseCreaterunsEnsureas a role with no databaseCREATEagainst objects it owns.TestEnsureRefusesASchemaAnotherRoleOwns/…ATableAnotherRoleOwns/…AViewUnderTheTableNamepinErrForeignObject.TestSaveAndDeleteRequireTheTargetsTableLockandTestSaveRefusesWhenAnotherBackendHoldsTheTable(the session's backend terminated, another backend takes the key) pin LK-1.TestDeleteRefusesARowTheCallerWasNotShownpins the compare-and-delete.TestWritesAndReadsWithoutEnsureReportTheMissingTablepinsErrTableMissing. Mutations verified against the new tests;go test -raceandscripts/test-flaky.sh5/5 on the two concurrency tests.Stack
Based on
main, merged forward after #139 (the cutover swap) landed. No capability, verdict, or CLI surface changes;demo/tour.shis unaffected. The orchestrator that chains build → copy → verify → gate → cutover → drop, and the WAL-drainDrainFunc, are the next rows.