feat: add unused_replication_slot lint for stale replication slots - #173
Open
hunleyd wants to merge 29 commits into
Open
feat: add unused_replication_slot lint for stale replication slots#173hunleyd wants to merge 29 commits into
hunleyd wants to merge 29 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.
Both comments chained the wal_status classification and its rationale into one long clause. Split into two short sentences on the same line, and regenerated splinter.sql from the updated lint source.
…plication-slot-lint * origin/main: ci: fail the build when the test suite fails feat: release splinter.json to S3 chore: format fix: only release on feat/fix/breaking-change commits; defer S3 upload feat: generate splinter.json manifest and publish via release workflow
bin/check_lints.py (pulled in from main) requires every docs/*.md page to have a nav entry; the new lint's page was missing one, which failed CI's pre-commit hook.
The extended-slot fixture forces max_wal_size=2MB plus 10 WAL switches, which triggers an async checkpoint. If that checkpoint lands after the positive fixture's max_slot_wal_keep_size=1MB takes effect instead of before, it invalidates the slot immediately and the 'unreserved' assertion flakes straight to 'lost'.
Without the rationale in the file, the identical 'name'/'entity' values in metadata read as dead duplication to a future reader; the entity key exists only so Studio's getLintEntityString short-circuits to the slot name instead of rendering N/A (slots have no schema).
'lost' isn't always caused by exceeding max_slot_wal_keep_size -- Postgres 16+ can also invalidate a slot for rows_removed or wal_level_insufficient, neither of which is a WAL-retention problem. The lint's detail text and doc both asserted the retention cause unconditionally; reword to describe the observed wal_status rather than presuming its cause.
… own doc 'at risk of exceeding' implies the limit hasn't been hit yet, but the lint only fires once retained WAL has already exceeded max_slot_wal_keep_size (or the slot is already invalidated) -- the doc's own Summary and False Positives sections already say this correctly; the description column contradicted them.
hunleyd
marked this pull request as ready for review
August 8, 2026 04:36
hunleyd
marked this pull request as draft
August 9, 2026 03:47
The positive fixture drove WAL past max_slot_wal_keep_size with a single pg_switch_wal() call, unlike the extended-case fixture's 10-iteration loop — a single switch only advances the LSN by however much is left in the current WAL segment, so it could land short of the 1MB threshold depending on WAL already emitted earlier in the script, turning an unrelated future change into a spurious failure here. Use the same 10-iteration loop as the extended case for a comfortable margin, and add a level column to the positive assertions so the WARN/ERROR escalation is actually verified instead of inferred from the comments. Also add a logical-slot positive case: the only prior logical-slot fixture was negative, so `metadata.plugin` (the field this lint added specifically for logical slots) was never observed with a non-null value anywhere.
…naged slots The `**Level:** WARN|ERROR` header reused the skill template's pipe-separated placeholder syntax (meaning "pick one"), but this lint is the first to genuinely emit both depending on wal_status — on the rendered docs site that reads as an unfilled template. State explicitly which wal_status maps to which level instead. Read replica provisioning and Realtime both create their own replication slots (named after the replica's IP, or supabase_realtime_replication_slot*), and this lint has no way to tell those apart from a user-created slot. The remediation doc led with an unqualified "drop the slot" — for a platform-managed slot that doesn't fix anything and can break replication or realtime delivery outright. Add the carve-out.
Supabase's own managed Postgres always sets this GUC, but splinter also runs against self-hosted instances. Left at Postgres's own default (-1, never invalidate for size), an abandoned slot stays 'extended' forever and this lint never fires -- the exact "silently eats disk" scenario the lint exists to catch. State the dependency so a self-hosted user knows to set it.
…hecklist 0030 is the first lint whose level varies per row (a case expression, WARN/ERROR depending on wal_status) instead of a fixed literal like every other lint. Document it as an accepted pattern in SKILL.md's column table and doc template, and add a check_lints.py assertion so a future dynamic- level lint can't ship with the doc's Level line still showing the unfilled WARN|ERROR|INFO placeholder.
CI's pre-commit black hook flagged the DYNAMIC_LEVEL_RE regex assignment as unformatted -- black wraps it across three lines at this line length.
The regex-based DYNAMIC_LEVEL_RE check only fired for a lint with a case expression matching one exact spelling (end as level, unquoted) and, even then, only caught the placeholder for that one lint -- a static-level lint left unfilled passed clean, and the regex missed realistic variants like a quoted identifier or a cast. Drop the SQL-shape dependency entirely and check unconditionally: no doc may contain the unfilled placeholder, for any lint. Strictly stronger, no parsing. Also stopped pasting instruction prose into Step 4's copy-paste fence -- an author copying it verbatim was shipping a sentence of meta-guidance into their own doc page. The guidance now sits outside the fence; the fence itself stays exactly what should land in the file.
supabase_realtime_replication_slot* misses Realtime's own second slot family (supabase_realtime_messages_replication_slot_<version>_<hash>, per platform's realtime-service stack) -- a user with an inactive slot from that family would read the doc as permission to drop a platform-managed slot. Widen to the supabase_realtime_* prefix platform's own guards already use. Also spell out the ip_<x>_<x>_<x>_<x> shape instead of leaving the reader to infer it from "named after the IP".
The test file only cleaned up at the tail, trusting a clean start -- a prior aborted run (a failed assertion, a killed process) could leave a stray splinter_test_* slot or a non-default GUC that turns the unrelated file running right after it (queries_are_unionable) into a confusing failure. Converge to a clean state at the top too: drop any stray slot, reset both GUCs before the fixtures rely on them being at their defaults. Documented two facts a future editor needs and wouldn't otherwise see: the logical positive fixture depends on max_slot_wal_keep_size still being set from the physical fixture above it, and the WAL-margin loop's 10-iteration count assumes the default 16MB wal_segment_size. The level case expression relied on the WHERE clause (25 lines away) to guarantee wal_status could only be 'lost' or 'unreserved', with a comment explaining the coupling instead of the case expression enforcing it. Made it exhaustive by listing both values explicitly instead of an else branch, so a future widening of the WHERE clause surfaces as a null level instead of silently mislabelling a new status as WARN.
docs[0] named the first matching doc even when a later one in the list
was the actual match -- irrelevant today (every lint has exactly one doc
page) but wrong in general. Iterate and report the doc that actually
contains the placeholder.
Also: the SKILL.md prose moved outside the Step 4 fence kept its
fence-era backslash escapes, which don't work inside a markdown code
span and rendered as broken/backslash-visible text once outside it. And
the case comment's semicolon got silently eaten by bin/compile.py's
line.replace(";", "") pass, producing a run-on sentence in the published
splinter.sql -- reworded without a semicolon and restored the note that
the case's exhaustiveness depends on the WHERE clause.
hunleyd
marked this pull request as ready for review
August 22, 2026 22:52
Replace em-dash-as-separator style in the new lint's doc, SQL comments, and test comments with plain punctuation (periods, colons, semicolons). Regenerated splinter.sql and the pg_regress expected output to match.
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, and if the consumer of that slot goes away for good, the slot retains WAL indefinitely and silently eats disk with no user-facing warning. This PR adds a new Performance Advisor lint,
0030_unused_replication_slot, that flags any inactive replication slot whose retained WAL has moved pastmax_slot_wal_keep_size.Details
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.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).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.max_slot_wal_keep_sizebeing finite. Supabase's managed Postgres always sets one; a self-hosted instance left at Postgres's own default (-1, never invalidate for size) can accumulate WAL on an abandoned slot without this lint ever firing. This is documented indocs/0030_unused_replication_slot.md.ip_x_x_x_x-pattern slot, or Realtime'ssupabase_realtime_replication_slot*) should never be dropped directly. Dropping it breaks replication/realtime delivery instead of fixing anything.levelis the first per-row (case-expression) severity in this repo instead of a fixed literal. This is formalized as an accepted pattern in.claude/skills/new-lint/SKILL.md, with abin/check_lints.pycheck that the doc's**Level:**line states the mapping explicitly instead of the unfilled placeholder.Testing
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), theunreserved→losttransition on both a physical and a logical slot (positive,levelandentity/pluginmetadata asserted on the logical case), plus the updatedtest/sql/queries_are_unionable.sql.pg_switch_wal()loop (matching theextendedfixture's own technique) instead of a single call, so it can't land short of the threshold depending on how much WAL earlier statements in the script already emitted.splinter.sqlregenerated viabin/compile.py.test/sql/0030_unused_replication_slot.sqlcan't use the standardbegin;/savepoint/rollback;pattern other lint tests use, because 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 fixtures can run.Misc
Changelog: supabase/changelog#150 documents the platform PR that creates the physical replication slots this lint detects. Its internal notes already have a TODO to add a reference to this PR into its public text once this PR ships.
Studio doesn't yet know how to render this lint (no
lintInfoMapentry), tracked as INDATA-1326, with companion PRs supabase/platform#37386 and supabase/supabase-nimbus#38 already open. Neither blocks this PR merging.