Repository navigation
Conversation
…py pass The pass compares only through the watermark, so a shadow row above it holding a unique value the source has since moved into a differing chunk made every recopy of every reconcile collide; only --restart escaped.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
🤖 Review on Jash’s behalf at Verdict: no concrete blocker found in the full diff, with the verification limits below. Clearing the shadow above the persisted watermark before comparing the retained prefix addresses the collision that repeated prefix-only comparisons cannot reach. Findings and action items
Verified and limitationsStatic full-diff review: clear errors return before the verifier runs; an invalid watermark still skips the pass; the wrapper binds work to the lock context and delegates to the existing clear implementation; options carry the pass's timeouts, chunker and clock. The added test checks zero collisions, repair coverage, unchanged watermark/phase, absence of rows above it, and final convergence after copy resumes. No existing tests were removed. GitHub reports 16 successful checks, including PostgreSQL 14–18. Exact-head surrounding-source retrieval failed ( Generated by Codex. |
|
🤖 Product review at OSS adoption: recovery now handles the documented partial-watermark collision without requiring an operator to discard the run with SchemaBot integration: the fix is inside Generated by Codex. |
JashLal
left a comment
There was a problem hiding this comment.
🤖 Approving on Jash’s behalf after the full-diff reviews at 4d8c1d6. No concrete blocker found; the optional interrupted-clear regression and verification limits are recorded above. No local build or tests ran. Generated by Codex.
|
🤖 Review 1/2 of 4d8c1d6: 0 blocking, 5 non-blocking. I read the whole diff at The change is correct. There are no blocking findings. The non-blocking ones are one stale sentence in the ST-4 entry, a placement question, a typed-error and partial-completion coverage gap, and two surviving mutants. Invariants: upholds CO-4 (the reconcile now owes the same clear a resumed copier owes, because the pass is what reads the shadow below the watermark; the Enforced paragraph still describes only the resumed copier, see 5), upholds ST-4 (the durable marker still precedes every slot change, and the shadow is not rebuilt nor the watermark moved), upholds CO-3 (the pass still states BlockingNone. Non-blocking1. The ST-4 entry still says the shadow is unchanged when the pass exhausts its attempts, which is no longer true on a partial watermark. invariants.md#L1125-L1127 ends the collision paragraph with "the pass then ends in the last collision's error with the row still in 2. The clear runs after the new slot exists, though it needs only the lock session and the shadow. 3. A fence timeout ends the reconcile with an untyped lock timeout, and nothing tests the interrupted-clear boundary. The first clear batch waits for any straggling Test case (passes on 4d8c1d6; run here)// A straggling writer on the shadow keeps the first clear batch waiting past
// lock_timeout. The attempt fails and leaves the row in reconciling; once the
// writer ends, the next attempt recovers the collision and lands.
func TestReconcileRetriesAfterTheClearFenceTimesOut(t *testing.T) {
f := newReconcileFixture(t, 500)
shadow := f.buildShadow(t)
copyAll(t, f, newCopier(t, f, shadow))
cp := f.row(shadow, checkpoint.PhaseCopying, copier.NewWatermark(300), "", 0)
require.NoError(t, f.store.Save(t.Context(), f.lock, cp))
f.exec(t, `UPDATE %s SET amount = 0 WHERE id = 150`)
f.exec(t, `UPDATE %s SET uniq = -400 WHERE id = 400`)
f.exec(t, `UPDATE %s SET uniq = 400 WHERE id = 160`)
table := pgx.Identifier{f.table.Schema, shadow.ShadowTable()}.Sanitize()
tx, err := f.pool.Begin(t.Context())
require.NoError(t, err)
defer func() { _ = tx.Rollback(context.Background()) }()
_, err = tx.Exec(t.Context(), `DELETE FROM `+table+` WHERE id = -1`) // ROW EXCLUSIVE on the shadow
reconciler, err := schemachange.NewReconciler(f.cfg, f.lock, f.store, f.target, shadow, checksum.Options{
LockTimeout: time.Second,
Chunker: copier.ChunkerOptions{InitialRows: 100, MaxRows: 100},
})
require.NoError(t, err)
_, err = reconciler.Reconcile(t.Context(), f.pool, cp)
require.Error(t, err)
row := f.load(t, shadow)
assert.Equal(t, checkpoint.PhaseReconciling, row.Phase)
require.NoError(t, tx.Rollback(t.Context()))
reconciled, err := reconciler.Reconcile(t.Context(), f.pool, row)
require.NoError(t, err)
t.Cleanup(func() {
ctx := context.WithoutCancel(t.Context())
assert.NoError(t, reconciled.Slot.Close(ctx))
assert.NoError(t, decode.DropSlot(ctx, f.cfg, f.pool, reconciled.Slot.Name()))
})
assert.Equal(t, 0, reconciled.Outcome.Collisions)
assert.Equal(t, 0, f.shadowRowsAbove(t, shadow, 300))
}
4. Nothing pins "nothing landed skips the clear". Moving the Test that fails on the mutant and passes on 4d8c1d6func TestReconcileWithNothingLandedLeavesTheShadowAlone(t *testing.T) {
f := newReconcileFixture(t, 500)
shadow := f.buildShadow(t)
copyAll(t, f, newCopier(t, f, shadow))
cp := f.row(shadow, checkpoint.PhaseCopying, copier.Watermark{}, "", 0)
require.NoError(t, f.store.Save(t.Context(), f.lock, cp))
reconciler, err := schemachange.NewReconciler(f.cfg, f.lock, f.store, f.target, shadow, checksum.Options{})
require.NoError(t, err)
reconciled, err := reconciler.Reconcile(t.Context(), f.pool, cp)
require.NoError(t, err)
t.Cleanup(func() {
ctx := context.WithoutCancel(t.Context())
assert.NoError(t, reconciled.Slot.Close(ctx))
assert.NoError(t, decode.DropSlot(ctx, f.cfg, f.pool, reconciled.Slot.Name()))
})
assert.Equal(t, 500, f.shadowRowsAbove(t, shadow, 0), "nothing landed, so the shadow is the resumed copier's to clear")
}
5. The exported Test case (passes on 4d8c1d6; run here): complete and zero watermarkfunc TestClearAboveHonorsCompleteAndZeroWatermarks(t *testing.T) {
f := newReconcileFixture(t, 500)
shadow := f.buildShadow(t)
copyAll(t, f, newCopier(t, f, shadow))
table := pgx.Identifier{f.table.Schema, shadow.ShadowTable()}.Sanitize()
count := func() (n int) {
require.NoError(t, f.pool.QueryRow(t.Context(), `SELECT count(*) FROM `+table).Scan(&n))
return n
}
require.NoError(t, copier.ClearAbove(t.Context(), f.pool, f.target, shadow, f.lock, copier.NewWatermark(math.MaxInt64), copier.Options{}))
assert.Equal(t, 500, count(), "a complete watermark has nothing above it")
require.NoError(t, copier.ClearAbove(t.Context(), f.pool, f.target, shadow, f.lock, copier.Watermark{}, copier.Options{}))
assert.Equal(t, 0, count(), "a zero watermark leaves the whole key space above it")
}
Verified
Mutants, each run against
This review was generated by Claude Code (claude-sonnet-5-5). |
|
🤖 Review 2/2 of 4d8c1d6: OSS adoption and SchemaBot integration lenses, 0 blocking, 2 non-blocking. This is the second of two comments; the correctness pass is the one above. The change adds one exported function, OSS adoption. A standalone user sees no new setting and one fewer dead end: a run whose slot was lost over a shadow holding rows above its watermark used to stay in BlockingNone. Non-blocking1. 2. A new class of error can come out of Verified
This review was generated by Claude Code (claude-sonnet-5-5). |
aparajon
left a comment
There was a problem hiding this comment.
🤖 Stamping with comments: 0 blocking, 7 non-blocking across the two review comments above (5 in the correctness pass, 2 in the OSS adoption and integration pass).
This stamp was left by Claude Code (claude-sonnet-5-5).
A failed clear now ends the attempt with no slot made; the failed-clear boundary and the nothing-landed skip are pinned by tests.
|
🤖 Adversarial review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — pull/279, follow-up commit All non-blocking findings are fixed in the follow-up commit except the
Source: block/pg-sprite#279, review comments 6489269295, 6489273700, 6567734719 and 6567735556, and reviews 5693511705 and 5731553933, at head |
Reconcile mode now clears the shadow above a partial watermark before it touches the slot, so a shadow row no comparison can reach never makes the recopy collide.
Why
When a copy-and-swap run loses its replication slot, reconcile mode makes a new slot and compares the shadow with the source to recopy whatever the lost slot never delivered. It compares only through the copier's saved watermark, but the shadow can hold rows above it: chunks the earlier run committed but never checkpointed. Those rows are outside every comparison and still hold their values. If the source has since moved a unique value out of one of them into a chunk below the watermark that the pass finds differing, the recopy collides with the row above on every attempt of every reconcile, and the run stayed in
reconcilinguntil the operator ran--restart. The copier that resumes from the watermark already clears the shadow above it first (CO-4); the reconcile ran before that clear.What
copier.ClearAboveexports the clear a resumedRunperforms before its first chunk: same batches, sameSHARE ROW EXCLUSIVEfence on the first batch, bound to the table lock session (LK-1). A complete watermark clears nothing. Its doc comment names the options it honors (timeouts, chunk size, clock).Reconcilerruns it right after confirming the relations and before any slot command, whenever the watermark says anything landed; nothing landed skips both the clear and the pass. A new slot then retains WAL only while the pass runs, and a clear that fails ends the attempt with no slot made and the row unchanged.Reconcile's doc comment says the clear can end an attempt with the copier's refusals or an untyped lock timeout, and that a retry recovers it.TestReconcileClearsTheShadowAboveAPartialWatermarkBeforeItsRecopyfails onmainwith five23505collisions and passes here;TestReconcileRetriesAfterTheClearFenceTimesOutpins the failed-clear boundary (55P03, no slot, row unchanged, the retry converges);TestReconcileWithNothingLandedLeavesTheShadowAlone;TestClearAboveHonorsCompleteAndZeroWatermarks.docs/invariants.mdST-4 and CO-4, anddocs/copy-and-swap-design.mdD14, drop the Known limit and describe the clear and its place before the slot.Before / after
The example is the regression test: 500 rows copied, the row checkpointed in
copyingat watermark 300 when the slot was lost, so the shadow holds keys 301–500 that the row never recorded. While the slot is gone the source changes key 150 (chunk 101–200 now differs) and moves key 400's unique value to key 160, in that same chunk. The pass itself, the recopy policy, and the phase the row returns to are unchanged.