Skip to content

reconcile: clear the shadow above a partial watermark before the recopy pass - #279

Open
Kiran01bm wants to merge 2 commits into
mainfrom
kiran01bm/cs-pd24-reconcile-clear-above
Open

Kiran01bm wants to merge 2 commits into
mainfrom
kiran01bm/cs-pd24-reconcile-clear-above

Conversation

@Kiran01bm

@Kiran01bm Kiran01bm commented Oct 29, 2026 •

Copy link
Copy Markdown
Collaborator

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 reconciling until 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.ClearAbove exports the clear a resumed Run performs before its first chunk: same batches, same SHARE ROW EXCLUSIVE fence 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).
  • Reconciler runs 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.
  • Tests: the regression TestReconcileClearsTheShadowAboveAPartialWatermarkBeforeItsRecopy fails on main with five 23505 collisions and passes here; TestReconcileRetriesAfterTheClearFenceTimesOut pins the failed-clear boundary (55P03, no slot, row unchanged, the retry converges); TestReconcileWithNothingLandedLeavesTheShadowAlone; TestClearAboveHonorsCompleteAndZeroWatermarks.
  • docs/invariants.md ST-4 and CO-4, and docs/copy-and-swap-design.md D14, 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 copying at 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.

Before                                                After
──────                                                ─────
source: key 160 uniq=400, key 400 uniq=-400           source: key 160 uniq=400, key 400 uniq=-400
shadow: keys 1–500, key 400 still uniq=400            shadow: keys 1–500, key 400 still uniq=400
row:    copying, watermark 300                        row:    copying, watermark 300

  reconcile                                             reconcile
  ├─ (no clear)                                         ├─ copier.ClearAbove(300)
  │                                                     │    deletes shadow keys 301–500,
  │                                                     │    key 400's stale uniq=400 with them
  ├─ new slot                                           ├─ new slot
  ├─ pass compares chunks 1–300                         ├─ pass compares chunks 1–300
  │    chunk 101–200 differs                            │    chunk 101–200 differs
  ├─ recopy 101–200 inserts key 160 uniq=400            ├─ recopy 101–200 inserts key 160 uniq=400
  │    collides with shadow key 400 (23505)             │    lands: nothing above 300 holds it
  ├─ compare again: key 400 is above 300,               ├─ row saved: copying, watermark 300,
  │    never compared, still holds uniq=400             │    Collisions 0, repairs {150, 160}
  ├─ … five attempts, five collisions                   └─ copier resumes from 300 and lands
  └─ pass ends in the last error;                            keys 301–500 again; shadow converges
       row stays in reconciling, only
       --restart escapes

…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.
@Kiran01bm
Kiran01bm marked this pull request as ready for review October 29, 2026 05:42
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ⚠️ Failed 2026-10-29T05:42:38.820879Z
We couldn’t complete this review. Comment @codex review to try again.
4d8c1d6 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@Kiran01bm
Kiran01bm requested a review from morgo as a code owner October 29, 2026 05:42
@JashLal

JashLal commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator

🤖 Review on Jash’s behalf at 4d8c1d6e8c0c120e34bf43c81ad5998aaddddfdf.

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

  1. (Optional) Add interruption coverage around pkg/schemachange/reconcile.go:258: stop or lose the lock after one clear batch commits, then resume reconciliation. Assert that the row remains recoverable, the watermark does not advance, and a later pass converges. The new regression in pkg/schemachange/reconcile_clear_integration_test.go:43 exercises the successful collision fix well; this would cover the newly earlier destructive step's partial-completion boundary.

Verified and limitations

Static 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 (error connecting to api.github.com), so unchanged helper semantics, repository instructions outside the diff, and inline review threads were not independently verified. No local build or tests ran.

Generated by Codex.

@JashLal

JashLal commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator

🤖 Product review at 4d8c1d6.

OSS adoption: recovery now handles the documented partial-watermark collision without requiring an operator to discard the run with --restart. The design and invariant text describe why the prefix comparison alone was insufficient and what the clear changes. No new CLI setting or operator decision is introduced in this diff.

SchemaBot integration: the fix is inside Reconciler, so an existing library caller using that path receives the repair without a new orchestration step. ClearAbove returns errors directly and does not mark the clear successful after a failure. No integration blocker is demonstrated by the diff; the current adapter and unchanged error-classification paths were not available for independent verification.

Generated by Codex.

@JashLal JashLal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.

@aparajon

aparajon commented Nov 2, 2026

Copy link
Copy Markdown
Collaborator

🤖 Review 1/2 of 4d8c1d6: 0 blocking, 5 non-blocking.

I read the whole diff at 4d8c1d6 (6 files) and the code around it (Reconcile, claimSlot, copier.NewCopier, clearAbove, clearBatch, startAfter), ran the PR's regression test on both trees, ran the reconcile tests under 10 mutants, and ran three probes against PostgreSQL 16 (the only version I had; CI covers 14 to 18).

The change is correct. Reconciler.recopy now calls the new exported copier.ClearAbove before verifier.Check (reconcile.go#L249-L264). ClearAbove builds a Copier with the pass's timeouts, chunk size and clock (reconcile.go#L85-L92), binds the lock session's context, and runs the same clearAbove a resumed copier runs before its first chunk (resume.go#L28-L37), so the batching, the SHARE ROW EXCLUSIVE fence on the first batch, and the guard (lock, relation, and conversion confirmation) are the ones CO-4 already pins. A complete watermark clears nothing (startAfter reports done; probe below), an unset watermark skips both the clear and the pass as before, and a clear error returns before the verifier runs and closes the slot the attempt made. The ordering is safe for a crash at any point: the row stays reconciling, the clear is idempotent, and the resumed copier clears again. The regression test fails on the base commit with the collision the PR describes (recopy of 1 chunks ... collided ... on each of 5 attempts ... SQLSTATE 23505) and passes at the head in about three seconds; the clear needs the shadow rows above the watermark to be gone and nothing else, so no other property of the pass changes.

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 recopy), upholds LK-1 (the clear is bound to the lock session and confirms the lock in every batch's guard).

Blocking

None.

Non-blocking

1. 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 reconciling and the shadow unchanged". The clear commits before the pass starts, so a reconcile whose pass then fails leaves every shadow row above the watermark deleted. That is safe (the copier would delete them on resume anyway, and the watermark does not move), but the entry is the registry reviewers rely on, and "unchanged" is now false in exactly the case this PR adds. Suggested wording: "with the row still in reconciling, no recopy committed, and the watermark unmoved". The reflow in copy-and-swap-design.md#L556-L560 also leaves a two-line tail (collision is the pass's error: a repair then assumes the source rows ...) well short of the file's wrap width.

2. The clear runs after the new slot exists, though it needs only the lock session and the shadow. claimSlot (reconcile.go#L176) replaces the slot, and the clear then runs, then the pass, then the stream reopens. Everything the clear takes time over is WAL the new slot retains with no consumer, and a failed clear (below) means a slot made and closed for nothing. Moving the clear to just after confirmShadowRelations and before claimSlot would keep the slot's unconsumed window to the pass and let a failed clear end before any slot command. The ST-4 marker ordering is unaffected (the clear is not a slot change), and a crash after the clear and before the marker leaves the row in its earlier phase, where the clear is idempotent. This is by reading, not measured: I did not time a large shadow, and the slot-lag ceiling is judged at catch-up (catchup.go#L416), so it matters only when the clear is long.

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 ROW EXCLUSIVE holder on the shadow, bounded by lock_timeout. I held such a transaction open and reconciled with a one-second LockTimeout: the attempt ended with lock "..."."..._new" in SHARE ROW EXCLUSIVE mode for the copy: ERROR: canceling statement due to lock timeout (SQLSTATE 55P03), not a refusal cause, the row stayed in reconciling at watermark 300 with the slot name recorded, and after the holder ended the next Reconcile on that row completed with 0 collisions and no row above 300. So the behavior is fail-closed and recoverable; it is simply unpinned (Jash's optional item asks for the same). An adapter that keys retries on typed refusals will see a new untyped class from Reconcile.

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))
}

--- PASS on 4d8c1d6 (4.7s); the first attempt's error is the 55P03 above. It is a coverage test, so it does not fail on a tree without the clear for the lock reason; there the first attempt fails on the collision instead.

4. Nothing pins "nothing landed skips the clear". Moving the !cp.Watermark.Valid() check below the ClearAbove call (mutant m4) survives the whole reconcile test set: with an unset watermark ClearAbove deletes the entire shadow, then the pass is skipped. The outcome is harmless (a zero watermark makes the resumed copier delete the whole shadow anyway), but the new comment on recopy and ST-4 both say nothing landed skips both steps, and the work is pure waste on a large shadow.

Test that fails on the mutant and passes on 4d8c1d6
func 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")
}

--- PASS on 4d8c1d6. With the check moved below the clear: --- FAIL: TestReconcileWithNothingLandedLeavesTheShadowAlone (2.38s), expected: 500, actual: 0.

5. The exported ClearAbove has no direct test, and three pieces of its plumbing survive mutation; CO-4's Enforced text does not name the second caller. Mutants that drop the chunk size from clearOptions (m6), drop the lock and statement timeouts from it (m7), or drop the Bind inside ClearAbove (m10) all survive: the reconcile test has one partial watermark and 200 rows above it with a 100-row chunker, which exercises the clear but not what bounds it. The Bind is redundant under Reconcile (which binds already), so only a direct caller of the exported function depends on it, and the doc comment promises it (resume.go#L25-L27). A copier-level test of ClearAbove (complete watermark clears nothing; zero watermark clears everything; a lost lock cancels the batch in flight; a wrong proof is refused) would cover the three. Separately, the CO-4 Enforced paragraph (invariants.md#L205-L211) says "a resumed copy first deletes every shadow row above W", and the reconcile is now a second place that does, so the line should name it. Nit: clear (reconcile.go#L35, L75) shadows the Go builtin; clearOpts reads cleaner.

Test case (passes on 4d8c1d6; run here): complete and zero watermark
func 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")
}

--- PASS on 4d8c1d6 (2.0s). It does not kill m6, m7, or m10 on its own; those need a small chunker with more rows above than one batch, a short LockTimeout against a held lock, and a lost lock mid-clear.

Verified

  • The regression test fails on the base commit (0d96b2d) with recopy of 1 chunks of ... collided with a shadow row holding one of their unique values on each of 5 attempts ... SQLSTATE 23505 and passes at the head.
  • A complete watermark clears nothing and a zero watermark clears the whole shadow (probe above); a stalled fence ends the attempt fail-closed and the retry converges (finding 3).
  • NewReconciler validates the clear's options at construction, so a bad option cannot start a reconcile (m8 survives only because the verifier's own validation already refuses the same values; equivalent).
  • CI at the head: all 16 checks green, PostgreSQL 14 to 18 included.

Mutants, each run against TestReconcile*, TestNewReconciler*, and TestCutoverRowNotSwapped* in pkg/schemachange:

Mutant Caught by
m1: drop the ClearAbove call TestReconcileClearsTheShadowAboveAPartialWatermarkBeforeItsRecopy
m2: clear after the pass the same test
m3: clear from a zero watermark (everything) the same test, TestReconcileRecoversFromADroppedSlot, TestReconcileSurvivesAUniqueValueMovedDuringItsRecopy, TestCutoverRowNotSwappedWithALostSlotReconcilesOnceWrittenBack
m4: clear before the Valid() skip survived (finding 4)
m5: clear from W-1 the new test, TestReconcileSurvivesAUniqueValueMovedDuringItsRecopy
m6: drop the chunk size from clearOptions survived (finding 5)
m7: drop the lock and statement timeouts from clearOptions survived (finding 5)
m8: drop clear.Validate() survived, equivalent
m9: clear from W+1 the new test, TestReconcileRecoversFromADroppedSlot, TestReconcileSurvivesAUniqueValueMovedDuringItsRecopy, TestCutoverRowNotSwappedWithALostSlotReconcilesOnceWrittenBack
m10: drop Bind in ClearAbove survived (finding 5)

This review was generated by Claude Code (claude-sonnet-5-5).

@aparajon

aparajon commented Nov 2, 2026

Copy link
Copy Markdown
Collaborator

🤖 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, copier.ClearAbove, and changes what Reconciler.Reconcile does on a partial watermark. No flag, option field, verdict or plan JSON field, or refusal cause is added, removed, or renamed, and the proofs the reconcile takes (CopySwapTarget, the table lock session, BuiltShadow) are unchanged.

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 reconciling until --restart, and now recovers on its own. The ST-4 entry and design D14 describe the new clear instead of the limit, and CopySwapOptions.Restart is untouched. Integration. An adapter that already calls Reconciler.Reconcile gets the repair with no new orchestration step, and the failure stays fail-closed: the row stays reconciling, the watermark does not move, and the next attempt converges (I ran the stalled-fence case; see 1/2, finding 3).

Blocking

None.

Non-blocking

1. ClearAbove takes the whole copier Options but honors three of its fields. The signature is seven positional parameters (resume.go#L28), and it builds a full Copier only to run its clearAbove. Workers and Tracker are accepted and have no effect, and the doc comment says only that it "bounds each batch as a chunk transaction is bounded". A caller that sets Workers: 8 or a Tracker expecting progress gets neither and no error. The repo's rule is to export the smallest interface that does the job (AGENTS.md#L145-L147). Cheapest fix: say in the doc comment which fields apply (LockTimeout, StatementTimeout, Chunker, Clock) and that the rest are ignored; a dedicated small options type would be the cleaner seam if a second caller appears.

2. A new class of error can come out of Reconcile for an adapter to classify. The clear runs the copier's guard, so a reconcile can now end in the copier's typed refusals before the pass (a changed conversion fingerprint as copy-conversion-drift, a replaced relation, a lost lock) and, when a straggling writer holds the shadow past lock_timeout, in an untyped 55P03 lock timeout wrapped as lock ... in SHARE ROW EXCLUSIVE mode for the copy (reproduced in 1/2, finding 3). Reconcile's doc comment lists what it refuses and does not mention the clear's causes (reconcile.go#L94-L146), and the refusal catalog has no entry that says a reconcile can end this way. An adapter that retries typed refusals and pages on anything else would page on the timeout, though a retry is what recovers it. One sentence in the Reconcile doc comment, stating that the clear's refusals and lock timeout leave the row in reconciling for a retry, would let an importer key on it without reading the copier.

Verified

  • The only exported symbol added across the diff is copier.ClearAbove; Reconciler, NewReconciler, checksum.Options, and copier.Options keep their signatures and fields (clear is an unexported field).
  • No new CLI flag, environment variable, or configuration key; no change under cmd/.
  • The documented operator workaround for the limit (--restart) is removed only from the sentences that described the limit; CopySwapOptions.Restart and its other documentation are unchanged.
  • The failure of a stalled clear is fail-closed and recoverable on retry (probe in 1/2, finding 3).

This review was generated by Claude Code (claude-sonnet-5-5).

@aparajon aparajon left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.
@Kiran01bm

Copy link
Copy Markdown
Collaborator Author

🤖 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 ClearAbove plumbing tests, which are deferred.

# Finding Status Explanation
C3-F2 The clear runs after the new slot exists, though it needs only the lock and the shadow fixed The clear now runs right after the relations are confirmed, before the marker and any slot command. A new slot retains WAL only while the pass runs, and a failed clear ends the attempt with no slot made. Moving the clear back after the slot claim fails the new fence-timeout test.
C3-F3, C1-F1 A fence timeout ends the reconcile untyped, and nothing tests the interrupted-clear boundary fixed Added TestReconcileRetriesAfterTheClearFenceTimesOut, the review's case adapted to the new order. It asserts SQLSTATE 55P03, the row still in copying at watermark 300, no slot under the derived name, and that the retry converges with no collision.
C4-F2 Reconcile can now end in the clear's refusals or a lock timeout, undocumented fixed The Reconcile doc comment says so, says the row and watermark are left as they were, and says to retry the timeout.
C3-F1 ST-4 still says the shadow is unchanged when the pass exhausts its attempts; D14 has a short tail fixed ST-4 now reads "no recopy committed, and the watermark unmoved", and also covers a failed clear and the nothing-landed case. The D14 paragraph is rewrapped.
C3-F4 Nothing pins "nothing landed skips the clear" fixed Added TestReconcileWithNothingLandedLeavesTheShadowAlone, the review's case. A mutant that clears without the skip now fails it.
C3-F5 ClearAbove has no direct test; CO-4 does not name the second caller; clear shadows a builtin fixed Added TestClearAboveHonorsCompleteAndZeroWatermarks. CO-4's Enforced text now names the reconcile's clear. The field is clearOpts.
C3-F5 The chunk-size, timeout and Bind mutants survive deferred Tracked as an internal follow-up: copier-level tests for a multi-batch clear, a short lock timeout, and a lock lost mid-clear.
C4-F1 ClearAbove takes the whole copier.Options but honors three fields fixed The doc comment now names the fields it honors (LockTimeout, StatementTimeout, Chunker, Clock) and says Workers and Tracker have no effect. A dedicated options type waits for a second caller.
C2 Product review no action No actionable finding.
Reviews 5693511705 and 5731553933 and each comment's Verified section Approvals no action —

Source: block/pg-sprite#279, review comments 6489269295, 6489273700, 6567734719 and 6567735556, and reviews 5693511705 and 5731553933, at head 4d8c1d6e; fixes in the follow-up commit.

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.

3 participants