Skip to content

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

Open
Kiran01bm wants to merge 1 commit into
mainfrom
kiran01bm/cs-pd24-reconcile-clear-above
Open

Kiran01bm wants to merge 1 commit into
mainfrom
kiran01bm/cs-pd24-reconcile-clear-above

Conversation

@Kiran01bm

Copy link
Copy Markdown
Collaborator

Reconcile mode now clears the shadow above a partial watermark before its recopy pass, so a shadow row no comparison can reach never makes the recopy collide.

Why

A reconcile on resume compares the source with the shadow only through the row's watermark, but the shadow can hold rows above it: chunks the earlier run committed but never checkpointed before its slot was lost. Those rows are outside every comparison the pass makes and yet 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 of that chunk inserts the value and collides with the row above; comparing again cannot clear it, because that row is never compared. Every attempt of every reconcile collided 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.
  • Reconciler.recopy calls it before verifier.Check whenever the watermark says anything landed; nothing landed still skips both. The clear runs under options derived from the pass's checksum.Options (timeouts, chunk size, clock), validated when the Reconciler is built.
  • Regression test TestReconcileClearsTheShadowAboveAPartialWatermarkBeforeItsRecopy fails on main with five 23505 collisions and passes here with none.
  • docs/invariants.md ST-4 and docs/copy-and-swap-design.md D14 drop the Known limit and describe the clear.

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 only thing that changes is the clear before the pass; 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
  ├─ new slot                                           ├─ new slot
  ├─ (no clear)                                         ├─ copier.ClearAbove(300)
  │                                                     │    deletes shadow keys 301–500,
  │                                                     │    key 400's stale uniq=400 with them
  ├─ 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

🤖 Generated with Amp (Claude Opus 4.6)

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

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.

2 participants