chore: stop an interrupted repack from slowing every write - #1439
Conversation
Time Submission Status
Submit or update total time with: Add time on top of previous submission with: See available commands to help comply with our Guidelines. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe Changespg_repack lifecycle
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Caller
participant Run
participant Cleanup as clearInterruptedRepack
participant DB as PostgreSQL
participant Repack as pg_repack
Run->>Cleanup: Check and clear leftover repack objects
Cleanup->>DB: Count triggers and repack tables
DB-->>Cleanup: Return counts
Cleanup->>DB: Recreate extension if leftovers exist
Cleanup-->>Run: Return cleanup result
Run->>Repack: Start command if cleanup succeeds
Caller->>Run: Cancel context
Run->>Repack: Send SIGINT
Merge Risk: ⚪ Minimal · up to The change clears interrupted-repack leftovers before starting new work and allows graceful cancellation. No actionable merge-blocking issue remains; merge after normal checks. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The recovery design addresses interrupted maintenance and prevents new work after cleanup fails. Its database-wide cleanup nevertheless assumes exclusive ownership that is enforced only within one worker. Concurrent maintenance and database permissions remain unresolved; no attacker-reachable exploit was established. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@holdex pr submit-time 4h |
1 participant
tn_vacuumrunspg_repack --allevery 50,000 blocks, underexec.CommandContext. When the node shuts down during a run, the context kills pg_repack with SIGKILL, which skips the cleanup pg_repack runs when interrupted. Itsrepack_triggerstays on the table it was repacking and copies every later write into arepack.log_<oid>table, and nothing removes it. The next--allrun then repacks those log tables as well, so the triggers chain and one write becomes many.One mainnet sentry had collected 53 leftover triggers and 96 log tables holding 226 GB, in a 255 GB database. A write to
primitive_event_typethere cascaded into 19 log tables, and a write toprimitive_eventsinto 17. The replication publication covers every table, so the node decoded all of those rows for every block. On 26-Sep oneauto_digestblock pushed it past the 30 s the node then allowed for a block's commit ID, and it failed that block on every retry for three days. A second sentry had 37 triggers and 48 GB of log tables. The leader, and a node run by another operator, had none.What changed
Leftovers are cleared before each run. Before starting pg_repack, the mechanism counts
repack_triggers and the tables in therepackschema. A fresh extension owns no tables there, so anything it finds was left by an interrupted run. It then drops the extension with CASCADE and creates it again in one transaction, which removes the triggers, the log tables and their types.lock_timeoutis 5 s, because block execution queues behind that lock. If the lock is not free, the run is reported as failed and pg_repack does not start on top of the leftovers. The next scheduled run tries again.A cancelled pg_repack gets SIGINT.
cmd.Cancelsends SIGINT, andWaitDelaykills the process only after 30 s. pg_repack 1.5.3 handles SIGINT by cancelling its query and callingrepack.repack_dropfrom its exit handler (on_interruptinpgut.c,repack_cleanup_callbackinpg_repack.c). It installs no handler for SIGTERM. The extension'sClosedoes not wait for the run, so on a node shutdown this is best effort. The check before each run is what makes sure nothing stays behind.Nothing in the
repackschema is consensus state. The leader has none of it, and the sentry cleared by hand has applied more than 14,000 blocks since then with no app hash mismatch.Tests
TestClearInterruptedRepack(kwiltest, on thekwil-postgresimage nodes run). It leaves an interrupted run on a table the way pg_repack does, by running thecreate_pktype,create_logandcreate_triggerstatements from pg_repack's ownrepack.tablesview. Then it does the same on that run's log table, so the triggers chain, and checks that one insert lands in both log tables. After clearing, norepack_triggerand no table inrepackremain, the extension exists again, and the table keeps its rows and takes writes. A log table left without its trigger is cleared too. With nothing left behind, the extension is not dropped.TestRunInterruptsPgRepackOnCancel: a stand-in pg_repack records SIGINT when its run's context is cancelled.TestRunSkipsPgRepackWhenLeftoversCannotBeCleared: when clearing fails, the run fails and pg_repack does not start.Seven mutations, seven caught: SIGKILL instead of SIGINT, the clearing error ignored, clearing never called, leftovers counted but not dropped, the extension not created again, the extension dropped when nothing was left, and tables in
repacknot counted.go test ./extensions/tn_vacuum/...andgo test -tags kwiltest ./extensions/tn_vacuum/...pass.go vet(both tags) andgolangci-lintwith the repo's config are clean.Not covered
A node that already carries leftovers is cleared at its next scheduled run after it upgrades, not at startup.
--allis unchanged; once this lands, therepackschema holds no tables when a run starts, so pg_repack no longer repacks its own log tables.There is no Problem issue for this in node, so the PR carries no closing keyword.
Summary by CodeRabbit