Repository navigation
decode, checkpoint, applier: slot lag ceiling and orphan-slot reaper - #156
Conversation
A replication slot is cluster state that retains WAL on the volume until it is confirmed or dropped. Two of ST-3's three guards were still planned: nothing bounded what a slot could retain, and nothing cleaned up a slot a run left behind. pkg/decode judges a slot: SlotStatus.WithinLagCeiling returns nil under the ceiling, SlotLostError for a slot the server reports lost (before any measure is read), ErrSlotLagUnknown for a retained amount the server could not measure (never read as nothing retained), and SlotLagExceededError carrying both figures over it. A ceiling below one byte is an invariant violation. DefaultSlotLagCeiling is one GiB (D11). InspectSlot now reads on a server in recovery, reporting retained WAL unknown instead of failing on pg_current_wal_lsn(). SlotNamePrefix and IsEngineSlotName tell the engine's name shape from a name merely wearing its prefix; Stream.Slot names the slot a stream decodes. pkg/checkpoint owns the reaper, because the checkpoint row is what makes a slot owned: Store.ReapOrphanSlots reads the logical, engine-prefixed slots of current_database() in one statement with the non-terminal rows that name them, drops each slot no live row names and no process holds through decode.DropSlot, publication and all, and reports the rest as Live, Active, or Foreign without touching them. Slots of other databases are never read. A database without the checkpoint table is refused with ErrTableMissing rather than read as all orphans. pkg/applier enforces the ceiling where the slot is consumed: Catchup ends every cycle by inspecting the stream's slot on the pool and judging it against CatchupOptions.SlotLagCeiling (zero takes the default; it cannot be switched off). A slot that vanished is SlotLostError; over the ceiling, Run ends with SlotLagExceededError, the table lock intact and the slot in place for the operator to read. Tests: WithinLagCeiling unit cases; a catch-up integration test pins the slot with an uncommitted multi-MiB transaction and asserts the typed abort with the lock held and the slot present; reaper integration tests cover orphan versus live row, a row turning terminal, a held slot, another database's slot, a prefix-wearing foreign slot, and a missing table.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
🤖 Adversarial correctness review, requested by Armand and performed by his agent. Reviewed at head Verdict: the lag ceiling and the typed lost state are correct and fail closed; the reaper has one gap between its read and its drop worth closing before the orchestrator calls it. I attacked every branch of Findings1. The reaper waits out, then drops, a slot that becomes active after its read. 2. Two reapers running at once can drop a slot a new run has just created. Slot names are derived per target ( Action items
Verified (tried to break, couldn't)
This review was generated by Claude Code (claude-opus-5-5). |
|
🤖 Product review, the second of two: adoption by a standalone user, and fit for the engine that consumes pg-sprite. OSS adoption ease
Integration with the engine that consumes it
This review was generated by Claude Code (claude-opus-5-5). |
aparajon
left a comment
There was a problem hiding this comment.
🤖 Approving on Armand's behalf after the adversarial correctness review above. The two reaper findings there are yours to pick up, ideally before the orchestrator starts calling ReapOrphanSlots. I'm flagging them, not gating on them.
This stamp was left by Claude Code (claude-opus-5-5).
morgo
left a comment
There was a problem hiding this comment.
🤖 Automated adversarial review, posted on Morgan Tocker's behalf.
Approving. The error taxonomy is right, the fail-closed directions are all set the way round they should be, and the "Decisions to veto" section made it easy to go straight at the choices rather than rediscover them. Two notes: the reaper's safety argument rests on a consistency guarantee the catalog does not provide, and the two new ST-3 guards do not compose — the ceiling abort creates exactly the state the reaper is written to refuse.
Non-blocking
The reaper's one-statement join does not give the consistency its comment claims, because pg_replication_slots is not MVCC.
candidateSlotsSQL's comment says: "The checkpoint table is read in the same statement, so a row saved before the slot it names is created is seen whenever the slot is." That inference holds for two heap tables. It does not hold here, because the two sides of the join are read under different rules. pg_replication_slots is a view over pg_get_replication_slots(), a set-returning function that walks the slot array in shared memory under ReplicationSlotControlLock; it reports what exists at the moment the executor reaches it. pgsprite_checkpoint is a heap read under the statement's snapshot, taken at statement start.
So a slot can be visible while the row that owns it is not:
- The reaper's statement begins; snapshot
Sis taken. - Run R commits its checkpoint row at
T1 > S. - R creates its slot at
T2, then closes the replication connection, leaving the slot inactive atT3. - The reaper's function scan executes at
Tf > T3, sees R's slot, and joins it against snapshotS, where R's row does not exist. liveis false,activeis false, the name has the engine's shape — so R's slot falls todefault:and is dropped, publication and all, under a live run.
The active flag is the only thing standing between this and a dropped slot, and it is false for the whole window between CreateSlot closing its connection and the stream opening — which spans the initial copy, the longest phase of a route. The ordering guarantee the design relies on (row committed before slot created) is real; it just is not sufficient when the slot list is newer than the snapshot the rows are read under.
The window is narrow in practice: Tf - S is one statement's execution, so hitting it needs the reaper's statement to outlast R's commit-to-slot-close sequence, and CREATE_REPLICATION_SLOT waiting for a consistent point makes that sequence slow. And ReapOrphanSlots has no caller in this PR, so nothing can hit it today. That is what makes it cheap to close now.
Reversing the reads fixes it without a lock: list the slots first, then read the checkpoint rows in a second statement. The second statement's snapshot is then strictly newer than the slot list, so any row committed before a slot it names was created is guaranteed visible — which is exactly the invariant the current comment asserts. It also costs one extra round trip on a path that runs once at startup. Requiring two consecutive passes to agree before dropping would also work, at the cost of leaving a genuine orphan for one more pass.
The ceiling abort leaves a slot the reaper is specifically written not to reclaim, so the two ST-3 guards do not compose.
Crossing the ceiling ends Run with the lock held and the slot in place, and the invariants entry says so plainly. I agree with the decision on its own terms: the run may be resumable once a pinning transaction ends, dropping is irreversible, keeping is not, and an operator gets Retained and Ceiling to read.
The gap is what happens next. After the abort, nothing is advancing confirmed_flush_lsn any more, so the slot keeps accruing WAL — the guard's response to the bound being crossed removes the only thing that was holding retention down. And the reaper will not take it: the route's checkpoint row is in a non-terminal phase, so the slot classifies as Live and is "reported and left", which is correct for a resumable run and wrong for an abandoned one. The two guards meet only if something moves the row to Done or Failed, and Catchup.Run does not — it returns the error and leaves the phase alone.
So ST-3 as worded, "a hard ceiling on what a slot may retain", is not what the ceiling enforces: it bounds how long a run keeps confirming, not what the slot retains, and retention after the abort is bounded only by whatever eventually marks the route failed. This repo already has the convention for saying that — the *Open:* sentences elsewhere in docs/invariants.md — and one of them here would keep the ST-3 entry from reading as fully enforced. The concrete composition point is worth naming too: whichever caller handles ErrSlotLagCeiling has to move the checkpoint row to a terminal phase, or the reaper will never collect the slot the ceiling just stranded.
Smaller, same entry: the doc says the ceiling "cannot be switched off", and validate does refuse anything below one byte, but math.MaxInt64 passes and disables it in every practical sense. "Cannot be set below one byte" is the claim the code actually makes.
What I tried to break and could not
likePrefix double-escaping the prefix. strings.NewReplacer with \→\\, %→\%, _→\_ is the one correct way to write this: a Replacer makes a single left-to-right pass and never re-examines what it just emitted, so the backslash it inserts for _ is not itself escaped. The obvious sequential alternative — three chained strings.Replace calls in that order — would turn pgsprite_ into pgsprite\\_ and match nothing. Since the pattern reaches the server as a parameter rather than a literal, LIKE's default backslash escape applies and pgsprite\_% matches the literal underscore, which is what the body's example shows.
A foreign name being dropped. The classification switch puts !IsEngineSlotName first, so a name wearing the prefix without the shape is Foreign before live or active are consulted — it can never fall through to the drop regardless of its row or its holder. Ordering the cheap boolean checks first would have been the natural way to write it and would have been wrong.
An empty checkpoint table reading as all-orphans. The two ways to lose the rows are handled differently and both correctly: a missing table is ErrTableMissing from missingTable(err) on both the Query and the CollectRows error, refusing the whole pass; a present-but-empty table legitimately means every engine slot is an orphan. Checking missingTable on the CollectRows error too is the part that is easy to omit, since the relation-missing error can surface at either point depending on the plan.
WithinLagCeiling reading an unknown measure as zero. RetainedWAL.Known is a separate field rather than a sentinel in Bytes, the check is !s.Retained.Known before any comparison, and TestWithinLagCeilingFailsClosedOnAnUnknownMeasure asserts the sentinel is neither loss nor the ceiling. The standby change is what makes this reachable rather than theoretical: wrapping the measure in CASE WHEN pg_is_in_recovery() THEN NULL converts a read that failed outright into a row with an unmeasured figure, and the Known flag is what keeps that from becoming "retains nothing".
Loss being inferred from a measure. Lost() reads WALStatus == WALStatusLost only, and WithinLagCeiling checks it before Known and before the comparison, so a lost slot that still reports a small retained figure is the lost state. TestWithinLagCeilingReportsALostSlotBeforeMeasuring constructs exactly that — WALStatusLost with Retained{Bytes: 1, Known: true} — and asserts NotErrorIs(ErrSlotLagCeiling). Testing the negative is what makes it an ordering test rather than a happy-path one.
The boundary being off by one. Retained.Bytes > ceiling with TestWithinLagCeilingAbortsOnlyAboveTheCeiling asserting both that exactly the ceiling is within it and that one byte over is not, against the same fixture. The ceiling < 1 refusal is checked before the slot is read at all, so a zero ceiling is a programming error rather than an abort of every slot.
Zero meaning "off" rather than "default". withDefaults maps zero to DefaultSlotLagCeiling and validate then refuses anything below one, so zero is the only value that takes the default and a negative is a refusal naming zero as the way to get it. That matches the MaxChanges shape already in the file, so the two options cannot drift in how they read their zero value.
The vanished-slot case being folded into the ceiling check. checkSlot handles found == false with its own &SlotLostError{Slot: ...} before WithinLagCeiling is reached, and leaves Found false so the error reads "no longer exists" rather than quoting an empty wal_status. The two spellings of loss stay distinguishable through one sentinel, which is what the ST-4 entry needs.
* origin/main: dbconn: have the replication connection ask for warnings itself (#154) Amp-Thread-ID: https://ampcode.com/threads/T-01a0dbfe-ca87-726b-a2cb-406197a863c6 # Conflicts: # SAFETY.md # docs/copy-and-swap-design.md
|
🤖 Adversarial review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — pull/156, follow-up commit The reaper now lists slots first and reads their owning rows in a later statement, re-reads each orphan's row right before dropping it, and drops without
Reviewed head |
Why
A replication slot is cluster state: it retains WAL on the volume until it is confirmed or dropped. ST-3 names three guards — a recognizable name prefix, a reaper for slots a run left behind, and a hard ceiling on what a slot may retain. Only the first existed. A run that died after
CreateSlotleft a slot pinning WAL forever, and a run that stayed alive but fell behind (a long-open transaction on the source, a stalled catch-up) had no bound on how much WAL its slot could keep.This PR lands the other two guards and the typed lost state ST-4 needs to tell "the server gave up on the slot" from a stream failure.
Before / after
What
pkg/decode/slot_lag.go(new) —DefaultSlotLagCeiling(1 GiB, D11);SlotStatus.Lost();SlotStatus.WithinLagCeiling(ceiling): lost →*SlotLostErrorbefore any measure, unknown retained →ErrSlotLagUnknown(never read as zero), over →*SlotLagExceededError{Slot, Retained, Ceiling}, ceiling < 1 →ErrInvariantViolation. SentinelsErrSlotLagCeiling,ErrSlotLost,ErrSlotLagUnknown.pkg/decode/slot_status.go—InspectSlotreads on a server in recovery:pg_wal_lsn_diff(pg_current_wal_lsn(), …)is wrapped inCASE WHEN pg_is_in_recovery() THEN NULL, so a demoted writer reports retained WAL as unknown instead of failing the whole read.pkg/decode/slot.go—SlotNamePrefix,IsEngineSlotName.pkg/decode/stream.go—Stream.Slot().pkg/checkpoint/reap.go(new) —Store.ReapOrphanSlots(ctx, cfg) (Reaped, error). Two reads, in order:pg_replication_slotsfiltered todatabase = current_database(),slot_type = 'logical',slot_name LIKE 'pgsprite\_%'; thenpgsprite_checkpoint WHERE slot_name = ANY($1) AND phase NOT IN (Done, Failed).pg_replication_slotsreads shared memory, not a snapshot, so a single joined statement could list a slot while the row committed before it was still invisible; reading the rows second makes their snapshot newer than the slot list. Each candidate lands in exactly one ofDropped/Live/Active/Foreign. An orphan's row is re-read right before its drop (a run that claimed the slot since the pass began keeps it), then it goes throughdecode.DropIdleSlot; a55006 object_in_userefusal files it asActive. Drops stop at the first failure and report what landed. Missing checkpoint table →ErrTableMissing, slots or not.pkg/decode/slot_drop.go—DropIdleSlot:DropSlotwithoutWAIT. A held slot isErrSlotActiveand left in place, publication and all; the verdict is the server's at the drop, not a catalog read before it.DropSlotkeeps its waiting behaviour for the route's own cleanup.SlotLagExceededError.Error()renders1.2 GiB of WAL, over the 1 GiB ceiling (1288490188 > 1073741824 bytes)and names the usual cause (a long-open transaction on the source) and when the run can resume.pkg/applier/catchup.go—CatchupOptions.SlotLagCeiling(0 → default; < 1 refused, so the ceiling is always in force — "cannot be set below one byte", not "cannot be switched off", sincemath.MaxInt64is accepted).cycleends withcheckSlot:InspectSloton the pool bystream.Slot(); not found →SlotLostError{Found: false}; thenWithinLagCeiling.slot_lag_test.go(unit, including the rendered error text — the renderer exception);slot_drop_integration_test.go—DropIdleSlotrefused withErrSlotActivewhile a walsender holds the slot, context intact, slot and publication left, then dropping both once released;catchup_slot_integration_test.go— an uncommitted 4 MiB insert pinsrestart_lsnunder a 1 MiB ceiling, assertingErrSlotLagCeiling, theErrorAsfields, notErrSlotLost, notErrTableLockLost, lock intact, slot still present;reap_integration_test.go— orphan dropped and live kept, live row turningDonereaped on the next pass, clean database reaps nothing, held slot reportedActive, another database's slot never read,pgsprite_backupreportedForeign, no checkpoint table refused.docs/invariants.mdST-3 / ST-4 Enforced, plus an ST-3 Open: sentence — a ceiling abort leaves the slot under a non-terminal row the reaper reads asLive, so whoever handlesErrSlotLagCeilingmust move the row to a terminal phase (or resume and drain) before the reaper can reclaim it; D11 namesReapOrphanSlotsand thepg_drop_replication_slot/DROP PUBLICATIONfallback for an operator reclaiming a slot by hand;SAFETY.mdapplier, checkpoint, slot-lifecycle rows;docs/copy-and-swap-design.mdD11 / D12 Where enforced and thepkg/decode/pkg/applier/pkg/checkpointpackage-map rows;docs/architecture.mdanddocs/low-level-design.mdmove the ceiling off the plannedpkg/throttlerrow.Decisions to veto
applier.Catchup, not deferred to the orchestrator. The catch-up loop is the one live consumer of the slot today and already readspg_current_wal_lsn()on the pool every cycle; judging the slot there costs one more catalog row read per cycle and means the bound holds without an orchestrator. The orchestrator will callReapOrphanSlotsat startup when it exists; the reaper is aStoremethod with no caller in this PR.pkg/checkpoint. "Orphan" is defined by the checkpoint row, andcheckpointalready importsdecode; the reverse edge would cycle.Activeslots are left, not waited out — judged at the drop. A holder is the judge of its slot; waiting under the reaper's context and then dropping would race a run that is about to write its row. The listedactiveflag is advisory; the server's55006at the no-wait drop is the verdict, so a slot that becomes held between the list and the drop is also left.Runends with the typed error; dropping the slot is a lifecycle decision for the caller (the run may be resumable once the pinning transaction ends). The error carriesRetainedandCeilingso an operator can read what happened.InspectSlothas no kept test. The harness has no standby fixture. It was validated by hand against a throwawaypg_basebackup -Rstandby on PostgreSQL 16: the previous expression fails withrecovery is in progress; the new one reads the slot row with retainedNULLon the standby and a byte count on the primary. The live "lost mid-stream" transition (ST-4 reconcile) is a separate follow-up.Verification
make lint— 0 issues.SKIP_INTEGRATION=1 go test ./...— ok.go test -count=1 ./pkg/decode/ ./pkg/applier/ ./pkg/checkpoint/— ok (PostgreSQL 16 testcontainers).scripts/test-flaky.sh TestCatchupEndsWhenTheSlotRetainsMoreThanTheCeiling 8 ./pkg/applier/— 8/8.scripts/test-flaky.sh TestReapOrphanSlotsLeavesHeldSlotsAndOtherDatabasesAlone 8 ./pkg/checkpoint/— 8/8.scripts/test-flaky.sh TestDropIdleSlotRefusesAHeldSlotWithoutWaiting 8 ./pkg/decode/— 8/8.