Skip to content

feat: add unused_replication_slot lint for stale replication slots - #173

Draft
hunleyd wants to merge 3 commits into
mainfrom
hunleyd/add-unused-replication-slot-lint
Draft

feat: add unused_replication_slot lint for stale replication slots#173
hunleyd wants to merge 3 commits into
mainfrom
hunleyd/add-unused-replication-slot-lint

Conversation

@hunleyd

@hunleyd hunleyd commented Jul 23, 2026

Copy link
Copy Markdown

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_lsn on 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 exceeded max_slot_wal_keep_size (WARN) or that Postgres has already invalidated (ERROR).

(bot-generated information collapsed below)


Details
  • Adds 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 whose wal_status is unreserved (WARN — retained WAL has passed max_slot_wal_keep_size, 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.
    • level is the first per-row case-expression severity in this repo's lint views. .claude/skills/new-lint/SKILL.md documents this as an accepted pattern, and now documents the one-physical-line, no-trailing-comment requirement the level column must follow for bin/check_lints.py's new check (below) to parse it.
    • metadata sets entity (Studio's getLintEntityString needs schema+name or entity to render anything, and a slot has no schema), type, slot_type, and wal_status unconditionally, plus plugin/database (null for a physical slot) — a logical slot's drop must run against the database it was created in.
    • cache_key includes wal_status, not just the slot name (unused_replication_slot_<slot>_<wal_status>). A slot's two rows (unreserved then lost) would otherwise share one key, so muting the WARN row via advisor_notification_exceptions would silently suppress the later ERROR row for the same slot once it escalates.
    • This lint needs max_slot_wal_keep_size set to a finite value. Supabase's managed Postgres always sets one; a self-hosted instance left at Postgres's own -1 default will not be caught by this lint. docs/0031_unused_replication_slot.md documents this limit.
    • The remediation doc warns against dropping a platform-managed slot directly (ip_x_x_x_x read-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.py gains check 7: for every doc page, it extracts the level(s) the matching lint's own SQL emits from its ... as level line 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.md said INFO while the view emits ERROR — fixed in this PR.
    • The extractor is line-anchored: the level column (bare literal or case expression) must sit on one physical line ending as level, with no trailing comment. A case expression split across multiple physical lines fails loud with an explicit error rather than being silently parsed — a genuine single-line case, however nested, always has as many ends as cases; a truncated tail line left behind by a split expression always has more ends than cases, however deep the leftover nesting. No existing lint needs more than one line.
  • .claude/skills/new-lint/SKILL.md gains: the one-physical-line level-column requirement on Step 2's own table row, the mkdocs.yaml nav step it was missing, a python bin/check_lints.py checklist item (naming the two specific items it doesn't cover, rather than letting it imply full coverage), the entity metadata note for schema-less objects, an explicit-cleanup exception for non-transactional test fixtures (replication slots and ALTER SYSTEM cannot run inside rollback), and placeholder text updated from a stale 0024 reference to <highest-numbered-existing-lint>.
  • bin/installcheck adds wal_level=logical to the shared test cluster's startup flags — needed for this lint's logical-slot fixtures. Every other lint's test/expected/*.out is generated against the same cluster, now running with this flag.

Testing
  • Added test/sql/0031_unused_replication_slot.sql, covering: a baseline with no slots; a reserved physical and logical slot (negative); an extended slot with max_slot_wal_keep_size disabled (negative); the unreserved to lost transition on a physical and a logical slot (positive), with level/entity/plugin/database metadata checked on the logical case.
    • Coverage gap, disclosed rather than fixed here: no fixture exercises active = truepg_regress cannot hold open a real walsender connection, so the view's not prs.active predicate is untested.
  • The Docker pg_regress suite 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.
  • Added bin/test_check_lints.py, an assert-based pinning test (no new framework — this repo's test/ is pg_regress-only) covering seven scenarios for the new level extractor: a single-line case (this lint's own shape), a multi-line case with a bare tail line, a multi-line case with a value inlined on the tail line, a multi-line case with a fully-nested case on the tail line, a trailing SQL comment on the level line, a same-line nested case, and a bare literal. Wired into .pre-commit-config.yaml alongside check-lints so 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.sql updated to union the new view; bin/compile.py regenerated splinter.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 lintInfoMap entry 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: replace bin/installcheck's hand-maintained lint list with a glob), INDATA-1475 (follow-up: consolidate 0031'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 lintInfoMap entry ships).

@hunleyd hunleyd self-assigned this Jul 23, 2026
@hunleyd
hunleyd marked this pull request as ready for review August 8, 2026 04:36
@hunleyd
hunleyd marked this pull request as draft August 9, 2026 03:47
@hunleyd
hunleyd marked this pull request as ready for review August 22, 2026 22:52
hunleyd added a commit that referenced this pull request Aug 23, 2026
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.
@soedirgo
soedirgo removed their request for review August 26, 2026 08:41
@hunleyd

hunleyd commented Aug 26, 2026

Copy link
Copy Markdown
Author

@claude review

Comment thread bin/check_lints.py Outdated
@hunleyd

hunleyd commented Sep 3, 2026

Copy link
Copy Markdown
Author

@claude review

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread bin/check_lints.py Outdated
@hunleyd

hunleyd commented Sep 3, 2026

Copy link
Copy Markdown
Author

@claude review

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

@hunleyd

hunleyd commented Sep 3, 2026

Copy link
Copy Markdown
Author

@claude review

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread bin/test_check_lints.py
@hunleyd
hunleyd force-pushed the hunleyd/add-unused-replication-slot-lint branch from 9124b12 to b48e041 Compare September 3, 2026 15:47
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.
@hunleyd
hunleyd force-pushed the hunleyd/add-unused-replication-slot-lint branch from b48e041 to 80ae625 Compare September 3, 2026 21:50
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.
@hunleyd

hunleyd commented Sep 4, 2026

Copy link
Copy Markdown
Author

@claude review

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hunleyd
hunleyd marked this pull request as draft September 8, 2026 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant