feat: add unused_replication_slot lint for stale replication slots - #173
feat: add unused_replication_slot lint for stale replication slots#173hunleyd wants to merge 3 commits into
Conversation
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.
|
@claude review |
|
@claude review |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I checked whether the gap I flagged in my prior review of _level_line_error (a leftover placeholder like **Level:** WARN|ERROR slipping past validation) was fixed. It now compares the doc's stated levels directly against the levels extracted from the lint's own SQL rather than against a fixed placeholder set, so that specific gap is closed.
Extended reasoning...
This run's inline findings stand on their own (the comment-stripping issue in _levels_in_lint_sql), so per the guidelines I'm not writing a full multi-section review here. I did, however, re-verify a concern from my prior review of an earlier version of this PR: _level_line_error previously validated a doc's **Level:** line against a hardcoded placeholder set, which meant a half-edited template value like WARN|ERROR could slip through undetected. Reading the current bin/check_lints.py (lines 67-73), the function now takes sql_levels (the actual set of levels extracted from the lint's own SQL via _levels_in_lint_sql) and requires doc_levels == sql_levels, erroring otherwise. This means a doc that keeps two of three template options, or otherwise diverges from what the SQL actually emits, will now fail the check regardless of what literal words appear in the doc. That closes the specific gap I raised previously, so I'm noting it rather than repeating it.
|
@claude review |
|
@claude review |
9124b12 to
b48e041
Compare
Read replica provisioning creates a replication slot on the primary. If the slot's consumer never returns, the slot keeps every WAL segment since its restart_lsn on disk without limit, which can fill the primary's disk. Adds a new Performance Advisor lint, 0030_unused_replication_slot, that flags a slot whose retained WAL has exceeded max_slot_wal_keep_size (WARN) or that Postgres has already invalidated (ERROR). It fires on a replication slot (physical or logical) that is inactive and whose wal_status is unreserved (WARN, still recoverable if the consumer catches up before the next checkpoint) or lost (ERROR, Postgres has already invalidated the slot; it cannot be reused). A slot that is active = false but still reserved or extended does not fire -- reserved covers a replica restarting, extended covers a healthy slot currently using more than max_wal_size.
b48e041 to
80ae625
Compare
Both branches added a new lint at slot 0030 (main's autovacuum_disabled, this branch's unused_replication_slot). Renumbered this branch's lint to 0031 across its lint/doc/test files, regenerated splinter.sql via bin/compile.py, added the missing -f lints/0031*.sql load to bin/installcheck, and re-ran the full regression suite locally to confirm the merged queries_are_unionable golden file is byte-accurate.
…numbering The lint's own renumbering to 0031 left a mislabeled pinning-test case name and two newly-added 2-3 line stacked comments behind.
|
@claude review |
There was a problem hiding this comment.
I reviewed this PR and found no bugs. Since my last review, the test-check-lints pre-commit hook I'd flagged as missing has been wired into .pre-commit-config.yaml, and the lint's renumbering from 0030 to 0031 (to resolve the collision with the autovacuum_disabled lint merged into main) is consistent across bin/installcheck, mkdocs.yaml, splinter.sql, and queries_are_unionable.sql. Given bin/check_lints.py's new level-extraction regex has needed several rounds of edge-case fixes already, a human look is still worthwhile before merging.
Extended reasoning...
This run's bug hunt reported zero new findings, and the diff in this scope (cd6d16a..HEAD) is dominated by a merge that renumbers the feature's lint from 0030 to 0031 to avoid colliding with autovacuum_disabled (added to main in the meantime) plus a small follow-up commit fixing a stale test label and stacked comments left over from that renumbering. I independently verified the renumbering is applied consistently everywhere it needs to be (installcheck's psql fixture list, mkdocs nav, splinter.sql's compiled UNION, and queries_are_unionable.sql/its golden file).
I also checked on two concerns from my own prior review rounds on this PR: (1) the missing pre-commit wiring for bin/test_check_lints.py — confirmed fixed, the test-check-lints hook now exists in .pre-commit-config.yaml; and (2) the level-extractor's earlier approach of scanning raw text between a case and end as level (vulnerable to comment-injected level words) — the current _levels_in_lint_sql/LEVEL_COL_RE implementation is a strictly single-physical-line match instead, which structurally avoids that class of bug and is covered by bin/test_check_lints.py's pinning cases (including a dedicated trailing-comment case). A narrower residual gap from an earlier thread (a doc **Level:** line that happens to enumerate exactly the same level words the SQL actually emits, even if left over from an unfilled template) is unchanged and already visible as an existing thread, so I'm not restating it.
Given no CODEOWNERS apply, no third-party CHANGES_REQUESTED review is outstanding (all substantive threads are this bot's own prior comments, self-resolved by the author), and the exit reason was dry_streak, this PR is eligible for either approve or defer. I'm choosing defer rather than approve because the PR still introduces a moderately intricate piece of self-referential tooling (a regex-based extractor that reasons about SQL case expressions to validate documentation), and that exact class of logic has already needed multiple correction rounds during this PR's review history — that track record argues for a human's eyes even though this run turned up nothing new. The core lint addition itself (a new Advisor view following the existing lint pattern, with docs and pg_regress fixtures) is otherwise unremarkable and well tested.
Summary
Read replica provisioning creates a replication slot on the primary. If the slot's consumer never returns, the slot keeps every WAL segment since its
restart_lsnon disk without limit, which can fill the primary's disk. This PR adds a new Performance Advisor lint,0031_unused_replication_slot, that flags a slot whose retained WAL has exceededmax_slot_wal_keep_size(WARN) or that Postgres has already invalidated (ERROR).(bot-generated information collapsed below)
Details
lints/0031_unused_replication_slot.sql: a new Advisor lint,unused_replication_slot. It fires on a replication slot (physical or logical) that is inactive and whosewal_statusisunreserved(WARN — retained WAL has passedmax_slot_wal_keep_size, still recoverable if the consumer catches up before the next checkpoint) orlost(ERROR — Postgres has already invalidated the slot; it cannot be reused).active = falsebut stillreservedorextendeddoes not fire —reservedcovers a replica restarting,extendedcovers a healthy slot currently using more thanmax_wal_size.levelis the first per-rowcase-expression severity in this repo's lint views..claude/skills/new-lint/SKILL.mddocuments this as an accepted pattern, and now documents the one-physical-line, no-trailing-comment requirement the level column must follow forbin/check_lints.py's new check (below) to parse it.metadatasetsentity(Studio'sgetLintEntityStringneedsschema+nameorentityto render anything, and a slot has no schema),type,slot_type, andwal_statusunconditionally, plusplugin/database(null for a physical slot) — a logical slot's drop must run against the database it was created in.cache_keyincludeswal_status, not just the slot name (unused_replication_slot_<slot>_<wal_status>). A slot's two rows (unreservedthenlost) would otherwise share one key, so muting theWARNrow viaadvisor_notification_exceptionswould silently suppress the laterERRORrow for the same slot once it escalates.max_slot_wal_keep_sizeset to a finite value. Supabase's managed Postgres always sets one; a self-hosted instance left at Postgres's own-1default will not be caught by this lint.docs/0031_unused_replication_slot.mddocuments this limit.ip_x_x_x_xread-replica slots,supabase_realtime_*slots, or any other name that doesn't obviously trace back to something you created yourself) — that breaks replication or Realtime delivery instead of fixing anything.bin/check_lints.pygains check 7: for every doc page, it extracts the level(s) the matching lint's own SQL emits from its... as levelline and requires the doc's**Level:**line to name exactly that set. This caught one pre-existing, unrelated drift:docs/0007_policy_exists_rls_disabled.mdsaidINFOwhile the view emitsERROR— fixed in this PR.caseexpression) must sit on one physical line endingas level, with no trailing comment. Acaseexpression split across multiple physical lines fails loud with an explicit error rather than being silently parsed — a genuine single-linecase, however nested, always has as manyends ascases; a truncated tail line left behind by a split expression always has moreends thancases, however deep the leftover nesting. No existing lint needs more than one line..claude/skills/new-lint/SKILL.mdgains: the one-physical-line level-column requirement on Step 2's own table row, themkdocs.yamlnav step it was missing, apython bin/check_lints.pychecklist item (naming the two specific items it doesn't cover, rather than letting it imply full coverage), theentitymetadata note for schema-less objects, an explicit-cleanup exception for non-transactional test fixtures (replication slots andALTER SYSTEMcannot run insiderollback), and placeholder text updated from a stale0024reference to<highest-numbered-existing-lint>.bin/installcheckaddswal_level=logicalto the shared test cluster's startup flags — needed for this lint's logical-slot fixtures. Every other lint'stest/expected/*.outis generated against the same cluster, now running with this flag.Testing
test/sql/0031_unused_replication_slot.sql, covering: a baseline with no slots; areservedphysical and logical slot (negative); anextendedslot withmax_slot_wal_keep_sizedisabled (negative); theunreservedtolosttransition on a physical and a logical slot (positive), withlevel/entity/plugin/databasemetadata checked on the logical case.active = true—pg_regresscannot hold open a real walsender connection, so the view'snot prs.activepredicate is untested.pg_regresssuite passes against this branch:docker rmi -f dockerfiles-test && SUPABASE_VERSION=15.1.1.13 docker-compose -f dockerfiles/docker-compose.yml run --rm test.bin/test_check_lints.py, an assert-based pinning test (no new framework — this repo'stest/ispg_regress-only) covering seven scenarios for the new level extractor: a single-linecase(this lint's own shape), a multi-linecasewith a bare tail line, a multi-linecasewith a value inlined on the tail line, a multi-linecasewith a fully-nestedcaseon the tail line, a trailing SQL comment on the level line, a same-line nestedcase, and a bare literal. Wired into.pre-commit-config.yamlalongsidecheck-lintsso it actually runs on every commit. Confirmed it catches a real regression by mutating the extractor back to each of two earlier, broken versions and re-running it.test/sql/queries_are_unionable.sqlupdated to union the new view;bin/compile.pyregeneratedsplinter.sql.Misc
Resolves INDATA-1084 (its original scope also covered an "orphaned slot with no matching read replica" case this lint does not check — INDATA-1084's own description already notes this narrower design and marks itself superseded rather than completed).
Related issues: INDATA-1175 (a known limitation: this lint may miss a connected-but-lagging consumer), INDATA-1326 (Studio's Advisor UI has no
lintInfoMapentry for this lint yet — companion PRs supabase/platform#37386 and supabase/supabase-nimbus#38 close that gap; neither blocks this PR), INDATA-1474 (follow-up: replacebin/installcheck's hand-maintained lint list with a glob), INDATA-1475 (follow-up: consolidate0031's test-fixture GUC reset into one shared block), INDATA-1476 (follow-up:check_lints.py's Level-line validator can false-fail on negated prose).Changelog: supabase/changelog#242 (internal-only; goes public once Studio's
lintInfoMapentry ships).