feat: add unused_replication_slot lint for stale replication slots#173
Draft
hunleyd wants to merge 12 commits into
Draft
feat: add unused_replication_slot lint for stale replication slots#173hunleyd wants to merge 12 commits into
hunleyd wants to merge 12 commits into
Conversation
Read replica provisioning now creates a replication slot on the primary. If
the consumer of that slot goes away for good, the slot retains WAL forever
and silently eats disk. Add a Performance Advisor lint that flags any
replication slot that is inactive and whose wal_status has moved past
max_slot_wal_keep_size ('unreserved', WARN) or is already invalidated
('lost', ERROR). A slot that's merely inactive but still 'reserved' or
'extended' does not fire, so a brief replica restart doesn't generate noise.
test/sql/0030_unused_replication_slot.sql can't use the usual
begin/savepoint/rollback pattern: replication slots are non-transactional
(they survive a rollback) and ALTER SYSTEM is rejected inside a transaction
block outright, so cleanup is explicit instead.
test/expected/0030_unused_replication_slot.out baked in literal LSN values returned by pg_create_physical_replication_slot and pg_switch_wal. Since pg_regress runs every test file against one shared instance, those LSNs depend on cumulative WAL from every earlier test and shift whenever an earlier test's WAL volume changes, breaking this test with an unrelated diff. Wrap the calls in `do $$ begin perform ...; end $$;` so only the side effect (creating the slot / switching WAL) happens and the LSN never reaches the test output.
…ot lint The lint's WHERE clause has no slot_type filter, so it's meant to cover both physical and logical slots, but the test only ever exercised physical slots. Add a negative-case fixture for a freshly created logical slot (still wal_status='reserved' while inactive, must not fire), same guarantee as the existing physical case. Requires wal_level=logical, which the test harness's initdb/pg_ctl start didn't set (community Postgres default is 'replica'), so add it to bin/installcheck's server start options.
…lication_slot lint docs/0030_unused_replication_slot.md explicitly documents 'extended' as a benign, must-not-fire state, but only 'reserved' was tested. Add a fixture that shrinks max_wal_size (with max_slot_wal_keep_size left at its disabled default, so the slot can only ever reach 'extended', never 'unreserved'/ 'lost') and drives WAL past it, confirming the lint stays silent. Per src/backend/access/transam/xlog.c's GetWALAvailability, reserved vs extended is pure segment-count arithmetic against max_wal_size, independent of data volume or checkpoints -- so the fixture advances WAL via repeated pg_switch_wal() calls instead of writing real rows. pg_switch_wal() is a no-op once already sitting at a segment boundary, so each loop iteration emits one trivial WAL record first (pg_logical_emit_message) to move off the boundary before switching again.
…I display Studio's getLintEntityString (apps/studio/components/interfaces/Linter/Linter.utils.tsx) only builds a display string when metadata.entity is set, or both metadata.schema and metadata.name are set. This lint had name but no schema (replication slots aren't schema-scoped) and no entity, so every finding rendered as 'N/A' in the Advisor UI despite the slot name being known. Setting metadata.entity to the slot name fixes this without any Studio-side change, since the entity check short-circuits before the schema check.
…lots
pg_replication_slots.plugin names the decoding plugin (pgoutput, wal2json,
test_decoding, etc.) a logical slot uses -- null for physical slots, but
the one field that identifies which CDC tool/consumer owned an abandoned
logical slot, which the doc's remediation advice ("investigate the
disconnect") assumes a user can see.
… fixture pg_switch_wal() forces a full segment advance regardless of preceding data volume, and max_slot_wal_keep_size (1MB) is far below one WAL segment, so a single trivial WAL record plus a switch already exceeds the retention limit. The 5000-row insert added test runtime/IO for no determinism benefit; replaced with the same lightweight emit-then-switch pattern already used for the extended-state fixture.
…nt test ~/.claude/CLAUDE.md: a comment must be exactly one line, never a run of several. Five explanations were spread across 2-4 consecutive -- lines; collapsed each into one line, no content change.
~/.claude/CLAUDE.md SQL section: always schema-qualify every function reference, never rely on search_path resolving an unqualified name. Every other system function in this file (pg_create_physical_replication_slot, pg_drop_replication_slot, pg_switch_wal) was already pg_catalog.-qualified; pg_reload_conf() was the one exception.
…cation_slot level's CASE and the WHERE clause's wal_status allowlist are two independently-maintained expressions. Not wrong today (WHERE only ever admits 'unreserved'/'lost', and 'unreserved' correctly falls to WARN), but widening WHERE to admit another wal_status value without updating the CASE would silently mislabel it WARN. One-line comment for future maintainers, no logic change.
…ixes bin/compile.py, reflecting the entity/plugin metadata keys and coupling comment added to lints/0030_unused_replication_slot.sql during review.
…ments The RESOLUTION comment on dropping a stale slot restated the action instead of explaining why dropping is necessary. Also adds why-comments to the WARN/ERROR case and the wal_status WHERE filter in the new 0030_unused_replication_slot lint, which previously had no rationale for why only 'lost' escalates to ERROR and why 'reserved'/'extended' are excluded.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Read replica provisioning creates a replication slot on the primary. If the consumer of that slot goes away for good (replica deleted, abandoned), the slot retains WAL indefinitely and silently eats disk on the primary — there was no user-facing surface calling this out.
Adds a new Performance Advisor lint,
0030_unused_replication_slot, that flags any replication slot (physical or logical, regardless of what created it) that is inactive and whosewal_statushas moved pastmax_wal_size:wal_status = 'unreserved'→WARN— retained WAL now exceedsmax_slot_wal_keep_size, recoverable if the consumer catches up before the next checkpoint.wal_status = 'lost'→ERROR— slot already invalidated, unusable.A slot that's merely
active = falsebut stillreservedorextendeddoes not fire —reservedcovers a replica restarting,extendedcovers a currently-healthy slot that's simply using more thanmax_wal_sizeright now (both benign per Postgres's ownwal_statussemantics).Metadata includes
entity/pluginso Supabase Studio's Advisor UI can display the slot name (instead of "N/A") and, for logical slots, which decoding plugin/consumer owned it.Test plan
pg_regresssuite green (All 29 tests passed), includingtest/sql/0030_unused_replication_slot.sqlcovering: baseline (no slots), a physicalreservedslot (negative), a logicalreservedslot (negative), anextendedslot withmax_slot_wal_keep_sizedisabled (negative), and theunreserved→losttransition on a physical slot (positive), plus the updatedtest/sql/queries_are_unionable.sql.splinter.sqlregenerated viabin/compile.py.Note:
test/sql/0030_unused_replication_slot.sqlcan't use the standardbegin;/savepoint/rollback;pattern other lint tests use — replication slots andALTER SYSTEMare both non-transactional (slots survive a rollback;ALTER SYSTEMis rejected outright inside a transaction block). Cleanup (dropping slots, resetting GUCs) is explicit instead; see comments in the test file. The test harness'sinitdb/pg_ctl startalso now setswal_level=logical(bin/installcheck) so the logical-slot fixture can run.