From 80ae625bb840922b6c3bbfa1150a5a402735e39d Mon Sep 17 00:00:00 2001 From: Douglas J Hunley Date: Thu, 3 Sep 2026 17:50:40 -0400 Subject: [PATCH 1/2] feat: add unused_replication_slot lint for stale replication slots 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. --- .claude/skills/new-lint/SKILL.md | 31 +-- .pre-commit-config.yaml | 7 + bin/check_lints.py | 79 ++++++-- bin/installcheck | 4 +- bin/test_check_lints.py | 64 +++++++ docs/0007_policy_exists_rls_disabled.md | 2 +- docs/0030_unused_replication_slot.md | 61 ++++++ lints/0030_unused_replication_slot.sql | 34 ++++ mkdocs.yaml | 1 + splinter.sql | 36 +++- .../expected/0030_unused_replication_slot.out | 177 ++++++++++++++++++ test/expected/queries_are_unionable.out | 4 +- test/sql/0030_unused_replication_slot.sql | 85 +++++++++ test/sql/queries_are_unionable.sql | 4 +- 14 files changed, 561 insertions(+), 28 deletions(-) create mode 100644 bin/test_check_lints.py create mode 100644 docs/0030_unused_replication_slot.md create mode 100644 lints/0030_unused_replication_slot.sql create mode 100644 test/expected/0030_unused_replication_slot.out create mode 100644 test/sql/0030_unused_replication_slot.sql diff --git a/.claude/skills/new-lint/SKILL.md b/.claude/skills/new-lint/SKILL.md index 5d05c5b..224ab71 100644 --- a/.claude/skills/new-lint/SKILL.md +++ b/.claude/skills/new-lint/SKILL.md @@ -23,15 +23,17 @@ Create a SQL view in the `lint` schema. The view **must** return exactly these 1 |--------|------|-------| | `name` | text | snake_case identifier, e.g. `'my_lint_name'` | | `title` | text | Human-readable title | -| `level` | text | `'ERROR'`, `'WARN'`, or `'INFO'` | +| `level` | text | `'ERROR'`, `'WARN'`, or `'INFO'` — a `case` expression is fine if severity varies per row (see `lints/0030_unused_replication_slot.sql`) | | `facing` | text | `'EXTERNAL'` or `'INTERNAL'` | | `categories` | text[] | e.g. `array['SECURITY']` or `array['PERFORMANCE']` | | `description` | text | What the lint checks and why it matters | | `detail` | text | `format()`-interpolated message naming the specific object | | `remediation` | text | `'https://supabase.com/docs/guides/database/database-linter?lint=XXXX_'` | -| `metadata` | jsonb | `jsonb_build_object('schema', ..., 'name', ..., 'type', ...)` | +| `metadata` | jsonb | `jsonb_build_object('schema', ..., 'name', ..., 'type', ...)`. If the flagged object has no schema (e.g. a replication slot, a role), set `entity` instead of `name` — Studio's `getLintEntityString` needs either `schema`+`name` or `entity` to render anything. | | `cache_key` | text | `format('_%s_%s', schema, object)` — unique per violation | +Whether `level` is a literal or a `case` expression, it must sit on one physical line ending `as level`, with no trailing comment: `bin/check_lints.py`'s check is line-anchored, not SQL-aware. + **Copy guidance for Advisor surfaces:** - Keep `description` to 1-2 short sentences: what the lint detects and the first likely action. - Keep `detail` concise and object-specific. It should describe the failing object, not restate the full rationale. @@ -108,16 +110,20 @@ order by Read `bin/installcheck`. Find line 55 — the long `psql` command that loads all lint files. It currently ends with something like: ``` --f lints/0024*.sql -d contrib_regression +-f lints/*.sql -d contrib_regression ``` Insert the new lint **before** `-d contrib_regression`, maintaining numeric order: ``` --f lints/0024*.sql -f lints/XXXX*.sql -d contrib_regression +-f lints/*.sql -f lints/XXXX*.sql -d contrib_regression ``` ## Step 4 — Create `docs/XXXX_.md` +- Pick the level your lint always emits. +- If level varies per row (a `case` expression in Step 2), replace the `**Level:**` line with an explicit statement of which value maps to which level instead, e.g. "`**Level:** WARN (unreserved) or ERROR (lost)`". +- Never leave the `**Level:** WARN|ERROR|INFO` placeholder unfilled; `bin/check_lints.py` rejects it whenever it names a level set different from what the lint's own SQL emits. + ```markdown **Level:** WARN|ERROR|INFO @@ -166,7 +172,7 @@ Cases where this lint may fire when the pattern is intentional, and how to handl ## Step 5 — Create `test/sql/XXXX_.sql` -Structure the test file exactly as follows: +Structure the test file exactly as follows, unless the lint touches non-transactional catalog state (replication slots, `ALTER SYSTEM`) that a `rollback` can't undo — see `test/sql/0030_unused_replication_slot.sql` for the explicit-cleanup pattern that case needs instead. ```sql begin; @@ -229,7 +235,7 @@ cp results/XXXX_.out test/expected/XXXX_.out ## Step 7 — Update `test/sql/queries_are_unionable.sql` -Read the file. Before the final semicolon (on the last `select * from lint."0024_..."` line), append: +Read the file. Before the final semicolon (on the last `select * from lint.""` line), append: ```sql union all select * from lint."XXXX_" @@ -237,6 +243,10 @@ Read the file. Before the final semicolon (on the last `select * from lint."0024 The file ends with a semicolon after the last view reference, then `rollback;`. Add the new entry before that semicolon. +## Step 7b — Add the doc page to `mkdocs.yaml`'s nav + +Read `mkdocs.yaml` and add `docs/XXXX_.md` under `nav:` -> `Lints:`, alongside the existing entries. `bin/check_lints.py` (see the Verification Checklist below) fails the build if a doc page exists but isn't linked from the nav. + ## Step 8 — Verify and promote `test/expected/queries_are_unionable.out` The Docker run from Step 6 already produced `results/queries_are_unionable.out`. **Read and verify it:** @@ -278,11 +288,9 @@ docker rmi -f dockerfiles-test && SUPABASE_VERSION=15.1.1.13 docker-compose -f d Then check: - [ ] `results/regression.diffs` is empty (no unexpected diffs) - [ ] `git diff test/expected/` shows only the new files you intentionally added/changed -- [ ] `splinter.sql` includes the new lint in the `UNION ALL` -- [ ] `lints/XXXX_.sql` exists -- [ ] `docs/XXXX_.md` exists -- [ ] `test/sql/XXXX_.sql` exists -- [ ] `test/expected/XXXX_.out` exists +- [ ] `lints/XXXX_.sql` exists — `check_lints.py` iterates this glob, so a missing file just means fewer lints get checked, not an error +- [ ] `splinter.sql` includes the new lint in the `UNION ALL` — regenerated automatically by the `compile-script` pre-commit hook, not checked by `check_lints.py` +- [ ] `python bin/check_lints.py` passes — covers everything else: registration, nav linkage, and that each doc's `**Level:**` line matches what its view actually emits --- @@ -295,4 +303,5 @@ Then check: | pgrst.db_schemas API exposure check | `lints/0023_sensitive_columns_exposed.sql`, `lints/0016_materialized_view_in_api.sql` | | pg_graphql extension-enabled check | `lints/0014_extension_in_public.sql` (for the `pg_catalog.pg_extension` pattern) | | begin/savepoint/rollback test structure | `test/sql/0024_rls_policy_always_true.sql` | +| explicit-cleanup test structure (non-transactional catalog state) | `test/sql/0030_unused_replication_slot.sql` | | Doc format | `docs/0024_permissive_rls_policy.md` | diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 6186cb4..dee1119 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -36,3 +36,10 @@ repos: language: python always_run: true pass_filenames: false + + - id: test-check-lints + name: Pinning tests for check_lints.py's level extractor + entry: python bin/test_check_lints.py + language: python + always_run: true + pass_filenames: false diff --git a/bin/check_lints.py b/bin/check_lints.py index b563206..8fe8284 100644 --- a/bin/check_lints.py +++ b/bin/check_lints.py @@ -12,10 +12,14 @@ 3. `test/sql/queries_are_unionable.sql` unions it 4. `docs/NNNN_*.md` exists 5. `test/sql/NNNN_*.sql` and `test/expected/NNNN_*.out` exist - 6. `mkdocs.yaml` links that doc page -It also checks that no two lints share a number, and that every doc page is in -the nav. Run via pre-commit, or directly: +It also checks that no two lints share a number, and, for every doc page +(including one with no matching SQL view, e.g. 0012, an auth config check): + + 6. it is linked from `mkdocs.yaml`'s nav + 7. its `**Level:**` line names exactly the level(s) the lint's own SQL emits + +Run via pre-commit, or directly: python bin/check_lints.py """ @@ -38,6 +42,34 @@ } STEM_RE = re.compile(r"^(\d{4})_[a-z0-9_]+$") +LEVEL_LINE_RE = re.compile(r"^\*\*Level:\*\*[ \t]*(.+)$", re.MULTILINE) +LEVEL_WORD_RE = re.compile(r"\b(?:warn|error|info)\b", re.IGNORECASE) +# Line-anchored on purpose: an unrelated `case`/`end` elsewhere in the query (a CTE, a nested case branch) never enters the extracted set. +LEVEL_COL_RE = re.compile( + r"^\s*(.+?)\s+as\s+level\s*,?\s*$", re.MULTILINE | re.IGNORECASE +) + + +def _levels_in_lint_sql(sql: str) -> set[str]: + """The level(s) a lint's `... as level` column emits, or an empty set if no single matching column line is found: zero matches, more than one match (e.g. a CTE's own level column shadowing the view's real one), or a `case` expression split across multiple physical lines. A genuine single-line `case`, however nested, always has as many `end`s as `case`s; a truncated tail line left behind by a split expression has more `end`s than `case`s, however deep the leftover nesting -- either way this line-anchored check treats it as unparseable rather than risk silently extracting an incomplete level set from just that tail line.""" + matches = LEVEL_COL_RE.findall(sql) + if len(matches) != 1: + return set() + column = matches[0] + if len(re.findall(r"\bend\b", column, re.IGNORECASE)) > len( + re.findall(r"\bcase\b", column, re.IGNORECASE) + ): + return set() + return {w.upper() for w in LEVEL_WORD_RE.findall(column)} + + +def _level_line_error(value: str, sql_levels: set[str]) -> str | None: + doc_levels = {w.upper() for w in LEVEL_WORD_RE.findall(value)} + if not doc_levels: + return "does not mention WARN, ERROR, or INFO" + if sql_levels and doc_levels != sql_levels: + return f"says {sorted(doc_levels)} but the lint's own SQL emits {sorted(sql_levels)}" + return None def find_by_number(directory: Path, number: str, suffix: str) -> list[Path]: @@ -52,17 +84,18 @@ def check() -> list[str]: mkdocs = MKDOCS.read_text() seen_numbers = {} + levels_by_number: dict[str, set[str]] = {} for lint_path in sorted(LINTS_DIR.glob("*.sql")): stem = lint_path.stem - match = STEM_RE.match(stem) - if not match: + stem_match = STEM_RE.match(stem) + if not stem_match: errors.append( f"{lint_path}: name must be NNNN_snake_case (four digits, " f"underscore, lowercase)" ) continue - number = match.group(1) + number = stem_match.group(1) if number in seen_numbers: errors.append( @@ -71,8 +104,20 @@ def check() -> list[str]: ) seen_numbers[number] = lint_path + sql = lint_path.read_text() + + sql_levels = _levels_in_lint_sql(sql) + if sql_levels: + levels_by_number[number] = sql_levels + else: + errors.append( + f"{lint_path}: could not determine the level(s) this lint " + f"emits (expected `'X' as level` or `case ... end as level`, " + f"on one physical line with no trailing comment)" + ) + # 1. view name matches the file stem - if f'create view lint."{stem}"' not in lint_path.read_text().lower(): + if f'create view lint."{stem}"' not in sql.lower(): errors.append( f'{lint_path}: must declare `create view lint."{stem}"` ' f"(view name has to match the file name)" @@ -93,8 +138,7 @@ def check() -> list[str]: ) # 4. documented - docs = find_by_number(DOCS_DIR, number, ".md") - if not docs: + if not find_by_number(DOCS_DIR, number, ".md"): errors.append(f"{lint_path}: missing docs page {DOCS_DIR}/{number}_*.md") # 5. tested @@ -108,14 +152,27 @@ def check() -> list[str]: f"{TEST_EXPECTED_DIR}/{number}_*.out" ) - # Step 6: every doc page is reachable from the nav. Covers docs for lints - # that are not SQL views too (e.g. 0012, an auth config check). + # steps 6-7 cover every doc page, including docs with no matching SQL view (e.g. 0012, an auth config check) for doc in sorted(DOCS_DIR.glob("[0-9]*.md")): + # 6. every doc page is reachable from the nav if doc.name not in mkdocs: errors.append( f"{doc}: not listed in {MKDOCS}; add it under `nav:` -> `Lints:`" ) + # 7. its Level line names exactly the level(s) the lint's own SQL emits + level_match = LEVEL_LINE_RE.search(doc.read_text()) + if not level_match: + errors.append(f"{doc}: missing a '**Level:**' line") + else: + # An empty set legitimately means "nothing to compare against" -- a doc with no + # matching lint view (e.g. 0012), or one whose own level column already errored + # above -- not a silent pass; _level_line_error skips the comparison either way. + doc_sql_levels = levels_by_number.get(doc.stem.split("_", 1)[0], set()) + level_error = _level_line_error(level_match.group(1), doc_sql_levels) + if level_error: + errors.append(f"{doc}: Level line {level_error}") + return sorted(set(errors)) diff --git a/bin/installcheck b/bin/installcheck index b54f633..18e73ef 100755 --- a/bin/installcheck +++ b/bin/installcheck @@ -30,7 +30,7 @@ rm -rf "$TMPDIR" # Initialize: setting PGUSER as the owner initdb --no-locale --encoding=UTF8 --nosync -U "$PGUSER" # Start the server -pg_ctl start -o "-F -c listen_addresses=\"\" -c log_min_messages=WARNING -k $PGDATA" +pg_ctl start -o "-F -c listen_addresses=\"\" -c log_min_messages=WARNING -c wal_level=logical -k $PGDATA" # Create the test db createdb contrib_regression @@ -52,7 +52,7 @@ else fi # Execute the test fixtures -psql -v ON_ERROR_STOP= -f test/fixtures.sql -f lints/0001*.sql -f lints/0002*.sql -f lints/0003*.sql -f lints/0004*.sql -f lints/0005*.sql -f lints/0006*.sql -f lints/0007*.sql -f lints/0008*.sql -f lints/0009*.sql -f lints/0010*.sql -f lints/0011*.sql -f lints/0013*.sql -f lints/0014*.sql -f lints/0015*.sql -f lints/0016*.sql -f lints/0017*.sql -f lints/0018*.sql -f lints/0019*.sql -f lints/0020*.sql -f lints/0021*.sql -f lints/0022*.sql -f lints/0023*.sql -f lints/0024*.sql -f lints/0025*.sql -f lints/0026*.sql -f lints/0027*.sql -f lints/0028*.sql -f lints/0029*.sql -d contrib_regression +psql -v ON_ERROR_STOP= -f test/fixtures.sql -f lints/0001*.sql -f lints/0002*.sql -f lints/0003*.sql -f lints/0004*.sql -f lints/0005*.sql -f lints/0006*.sql -f lints/0007*.sql -f lints/0008*.sql -f lints/0009*.sql -f lints/0010*.sql -f lints/0011*.sql -f lints/0013*.sql -f lints/0014*.sql -f lints/0015*.sql -f lints/0016*.sql -f lints/0017*.sql -f lints/0018*.sql -f lints/0019*.sql -f lints/0020*.sql -f lints/0021*.sql -f lints/0022*.sql -f lints/0023*.sql -f lints/0024*.sql -f lints/0025*.sql -f lints/0026*.sql -f lints/0027*.sql -f lints/0028*.sql -f lints/0029*.sql -f lints/0030*.sql -d contrib_regression # Run tests ${REGRESS} --use-existing --dbname=contrib_regression --inputdir=${TESTDIR} ${TESTS} diff --git a/bin/test_check_lints.py b/bin/test_check_lints.py new file mode 100644 index 0000000..577166e --- /dev/null +++ b/bin/test_check_lints.py @@ -0,0 +1,64 @@ +"""Pinning tests for check_lints.py's level-column extractor. + +Run directly: python3 bin/test_check_lints.py + +No test framework in this repo (test/ is pg_regress only) -- assert-based, +so a future edit that re-breaks the multi-line-case guard fails loudly here +instead of only in a mutation test someone has to remember to write by hand. +""" + +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).parent)) +from check_lints import _level_line_error, _levels_in_lint_sql # noqa: E402 + +CASES = { + "bare literal": ("'INFO' as level,", {"INFO"}), + "single-line case (0030's own shape)": ( + "case when prs.wal_status = 'lost' then 'ERROR' else 'WARN' end as level,", + {"ERROR", "WARN"}, + ), + "single-line nested case": ( + "case when a then 'ERROR' else case when b then 'WARN' else 'INFO' end end as level,", + {"ERROR", "WARN", "INFO"}, + ), + "multi-line case, bare end tail": ( + "case\n when a then 'ERROR'\n else 'WARN'\nend as level,", + set(), + ), + "multi-line case, value inlined on tail": ( + "case when a then 'ERROR'\n else 'WARN' end as level,", + set(), + ), + "multi-line case, nested case on tail": ( + "case when a then 'ERROR'\n else case when b then 'INFO' else 'WARN' end end as level,", + set(), + ), + "trailing comment": ("'WARN' as level, -- always warn", set()), +} + +failures = [] +for name, (column_line, expected) in CASES.items(): + sql = f"select\n {column_line}\n y\nfrom z;" + actual = _levels_in_lint_sql(sql) + if actual != expected: + failures.append(f"{name}: expected {expected!r}, got {actual!r}") + +# _level_line_error: an empty sql_levels set means "nothing to compare against", never a +# silent pass, and a doc mentioning no level word is always an error regardless. +if _level_line_error("WARN", set()) is not None: + failures.append( + "_level_line_error('WARN', set()) should skip comparison, not error" + ) +if _level_line_error("nothing here", set()) is None: + failures.append("_level_line_error with no level word should always error") +if _level_line_error("WARN", {"ERROR"}) is None: + failures.append("_level_line_error should error on a real mismatch") + +if failures: + print(f"{len(failures)} pinning test(s) failed:\n") + for failure in failures: + print(f" - {failure}") + raise SystemExit(1) +print(f"all {len(CASES)} level-extraction pinning tests passed") diff --git a/docs/0007_policy_exists_rls_disabled.md b/docs/0007_policy_exists_rls_disabled.md index 7201077..c1db1f2 100644 --- a/docs/0007_policy_exists_rls_disabled.md +++ b/docs/0007_policy_exists_rls_disabled.md @@ -1,5 +1,5 @@ -**Level:** INFO +**Level:** ERROR **Summary:** Security policy not enforced diff --git a/docs/0030_unused_replication_slot.md b/docs/0030_unused_replication_slot.md new file mode 100644 index 0000000..023c367 --- /dev/null +++ b/docs/0030_unused_replication_slot.md @@ -0,0 +1,61 @@ +**Level:** WARN (`unreserved`) or ERROR (`lost`). See Rationale below. + +**Summary:** Detects replication slots that are inactive and either retaining WAL beyond `max_slot_wal_keep_size` or already invalidated. + +**Ramification:** A replication slot with no active consumer keeps every WAL segment since its `restart_lsn` on disk indefinitely; left unattended, this can fill the primary's disk and cause an outage. If retained WAL grows past `max_slot_wal_keep_size` before the consumer catches up, Postgres invalidates the slot instead — at that point the disk risk is gone, but the slot itself is unrecoverable and replication for that consumer is permanently broken. + +--- + +### Rationale + +Postgres will never recycle WAL a replication slot still needs, even if nothing is reading from that slot anymore. This is normal and required for the slot to still be useful to a consumer that reconnects, but if the consumer (a read replica, a logical replication client, a CDC tool) is gone for good, the slot just accumulates WAL forever. + +Postgres itself tracks how close a slot is to actually causing harm via `pg_replication_slots.wal_status`: + +- `reserved`: normal, claimed WAL files are within `max_wal_size`. +- `extended`: `max_wal_size` is exceeded but the files are still retained (by the slot or by `wal_keep_size`). This is benign and can happen on perfectly healthy, currently-active slots (e.g. during a burst of write traffic); it does not by itself indicate a problem. +- `unreserved`: the slot no longer retains its required WAL and some of it is due to be removed at the next checkpoint. This is what actually happens once retained WAL exceeds `max_slot_wal_keep_size`, and it's still recoverable (can return to `reserved`/`extended` if the consumer catches up before the next checkpoint). +- `lost`: the slot has been invalidated (usually because its required WAL is already gone, though Postgres can invalidate a slot for other reasons too) and it can no longer be used to resume replication. + +This lint fires on `unreserved` or `lost`, not merely on `active = false`. A slot that's briefly inactive (e.g. its replica restarting) but still `reserved` (or even `extended`) is not yet a problem. + +This lint relies on `max_slot_wal_keep_size` being set to a finite value. Supabase's managed Postgres always sets one. On a self-hosted instance left at Postgres's own default (`max_slot_wal_keep_size = -1`, meaning "never invalidate for size"), an abandoned slot can stay `extended` and accumulate WAL indefinitely without ever reaching `unreserved`, so this lint will not catch it. Set `max_slot_wal_keep_size` to a finite value to get this protection. + +### How to Resolve + +**Option 1: Drop the slot if its consumer is gone for good** + +Only do this for a slot you created yourself. A slot named `ip____` (a read replica's IP) or prefixed `supabase_realtime_*` is owned by the platform (a read replica or Realtime), not by you. Dropping it does not fix anything and can break replication or realtime delivery outright. Remove the read replica from the dashboard, or contact support, instead of dropping a platform-managed slot directly. New platform features can introduce other reserved naming patterns over time — if a slot's name doesn't obviously trace back to something you created yourself, contact support before dropping it. + +```sql +select pg_drop_replication_slot(''); +``` + +A logical slot must be dropped from the database it was created in, not from `postgres` or any other database in the cluster — the lint's `database` metadata field names that database. + +**Option 2: If the slot is `unreserved` and a consumer is expected to reconnect, investigate the disconnect** + +Check why the replica/consumer isn't connecting (network issue, instance down, credentials) and monitor disk usage on the primary in the meantime. Once it reconnects and catches up before the next checkpoint, `wal_status` returns to `reserved` on its own. + +A `lost` slot has already been invalidated and cannot recover this way — Postgres will refuse to resume replication from it. Drop it (Option 1, unless it is platform-managed) and have the consumer re-establish a fresh slot instead of waiting for it to reconnect. + +### Example + +Given a physical replication slot whose replica was deleted recently enough that retained WAL has exceeded `max_slot_wal_keep_size` but the next checkpoint hasn't run yet: + +```sql +select slot_name, active, wal_status from pg_replication_slots; +-- slot_name | active | wal_status +-- ---------------------+--------+------------ +-- replica_abandoned | f | unreserved +``` + +Fix: + +```sql +select pg_drop_replication_slot('replica_abandoned'); +``` + +### False Positives + +A slot that is `active = false` but still `wal_status = 'reserved'` or `'extended'` will not fire. This covers a replica restarting, being briefly taken offline for maintenance, or a currently-healthy slot that's simply using more than `max_wal_size` right now, without generating noise. If this lint fires, the slot has already exceeded `max_slot_wal_keep_size` (or been invalidated entirely). diff --git a/lints/0030_unused_replication_slot.sql b/lints/0030_unused_replication_slot.sql new file mode 100644 index 0000000..a03e48c --- /dev/null +++ b/lints/0030_unused_replication_slot.sql @@ -0,0 +1,34 @@ +create view lint."0030_unused_replication_slot" as + +select + 'unused_replication_slot' as name, + 'Unused Replication Slot' as title, + case when prs.wal_status = 'lost' then 'ERROR' else 'WARN' end as level, + 'EXTERNAL' as facing, + array['PERFORMANCE'] as categories, + 'Detects replication slots that are inactive and either retaining WAL beyond max_slot_wal_keep_size (risking disk bloat) or already invalidated (unrecoverable, breaking replication for that consumer).' as description, + format( + 'Replication slot `%s` is inactive and its wal_status is `%s`', + prs.slot_name, + prs.wal_status + ) as detail, + 'https://supabase.com/docs/guides/database/database-linter?lint=0030_unused_replication_slot' as remediation, + jsonb_build_object( + -- Studio's getLintEntityString needs schema+name or entity to render anything, and slots have no schema, so entity is set to short-circuit straight to the slot name. + 'entity', prs.slot_name, + 'type', 'replication_slot', + 'slot_type', prs.slot_type, + 'wal_status', prs.wal_status, + 'plugin', prs.plugin, + -- a logical slot's drop only succeeds when run against the database it was created in. physical slots have no database. + 'database', prs.database + ) as metadata, + format('unused_replication_slot_%s_%s', prs.slot_name, prs.wal_status) as cache_key +from + pg_catalog.pg_replication_slots prs +where + not prs.active + -- 'reserved'/'extended' are still within retention limits, or a replica is reconnecting. Only 'unreserved' (limit already exceeded) and 'lost' (already invalidated) are worth flagging. + and prs.wal_status in ('unreserved', 'lost') +order by + prs.slot_name; diff --git a/mkdocs.yaml b/mkdocs.yaml index 1f5d05d..3e32107 100644 --- a/mkdocs.yaml +++ b/mkdocs.yaml @@ -37,6 +37,7 @@ nav: - Signed-In Users Can See Object in GraphQL Schema: '0027_pg_graphql_authenticated_table_exposed.md' - Public Can Execute SECURITY DEFINER Function: '0028_anon_security_definer_function_executable.md' - Signed-In Users Can Execute SECURITY DEFINER Function: '0029_authenticated_security_definer_function_executable.md' + - Unused Replication Slot: '0030_unused_replication_slot.md' theme: name: 'material' diff --git a/splinter.sql b/splinter.sql index 3c25786..e44f0f8 100644 --- a/splinter.sql +++ b/splinter.sql @@ -1838,4 +1838,38 @@ from order by schema_name, function_name, - function_args) \ No newline at end of file + function_args) +union all +( +select + 'unused_replication_slot' as name, + 'Unused Replication Slot' as title, + case when prs.wal_status = 'lost' then 'ERROR' else 'WARN' end as level, + 'EXTERNAL' as facing, + array['PERFORMANCE'] as categories, + 'Detects replication slots that are inactive and either retaining WAL beyond max_slot_wal_keep_size (risking disk bloat) or already invalidated (unrecoverable, breaking replication for that consumer).' as description, + format( + 'Replication slot `%s` is inactive and its wal_status is `%s`', + prs.slot_name, + prs.wal_status + ) as detail, + 'https://supabase.com/docs/guides/database/database-linter?lint=0030_unused_replication_slot' as remediation, + jsonb_build_object( + -- Studio's getLintEntityString needs schema+name or entity to render anything, and slots have no schema, so entity is set to short-circuit straight to the slot name. + 'entity', prs.slot_name, + 'type', 'replication_slot', + 'slot_type', prs.slot_type, + 'wal_status', prs.wal_status, + 'plugin', prs.plugin, + -- a logical slot's drop only succeeds when run against the database it was created in. physical slots have no database. + 'database', prs.database + ) as metadata, + format('unused_replication_slot_%s_%s', prs.slot_name, prs.wal_status) as cache_key +from + pg_catalog.pg_replication_slots prs +where + not prs.active + -- 'reserved'/'extended' are still within retention limits, or a replica is reconnecting. Only 'unreserved' (limit already exceeded) and 'lost' (already invalidated) are worth flagging. + and prs.wal_status in ('unreserved', 'lost') +order by + prs.slot_name) \ No newline at end of file diff --git a/test/expected/0030_unused_replication_slot.out b/test/expected/0030_unused_replication_slot.out new file mode 100644 index 0000000..678d51b --- /dev/null +++ b/test/expected/0030_unused_replication_slot.out @@ -0,0 +1,177 @@ +-- Unlike other lint tests, this file cannot use begin/savepoint/rollback: replication slots are non-transactional (they survive a rollback) and ALTER SYSTEM is rejected inside a transaction block, so every statement here autocommits individually and cleanup is explicit instead. +set search_path = ''; +-- Converge to a clean starting state instead of trusting the tail cleanup below. bin/installcheck initdb's a fresh cluster per run, so this has no effect there, but it makes the file safe to re-run by hand (psql -f) against a persistent dev cluster without a stray splinter_test_* slot or non-default GUC from a prior manual run causing a confusing failure. +do $$ +declare + stray record; +begin + for stray in select slot_name from pg_catalog.pg_replication_slots where slot_name like 'splinter_test_%' loop + perform pg_catalog.pg_drop_replication_slot(stray.slot_name); + end loop; +end $$; +alter system reset max_wal_size; +alter system reset max_slot_wal_keep_size; +-- pins the checkpointer to explicit `checkpoint;` calls below; the default 300s timeout could otherwise fire mid-fixture on a slow runner and desync the WARN/ERROR assertions. +alter system set checkpoint_timeout = '1h'; +select pg_catalog.pg_reload_conf(); + pg_reload_conf +---------------- + t +(1 row) + +-- pg_switch_wal() is a no-op at a segment boundary, so each iteration writes one trivial WAL record first to move off it before switching again; 10 iterations assumes the default 16MB wal_segment_size. +create function pg_temp.advance_wal() returns void language plpgsql as $fn$ +begin + for i in 1..10 loop + perform pg_catalog.pg_logical_emit_message(false, 'splinter_test', 'advance'); + perform pg_catalog.pg_switch_wal(); + end loop; +end +$fn$; +-- BASELINE: 0 issues, no slots exist. None of the fixtures below exercise `active = true`: pg_regress has no easy way to hold open a real walsender connection, so the view's `active = false` predicate itself is untested here. +select * from lint."0030_unused_replication_slot"; + name | title | level | facing | categories | description | detail | remediation | metadata | cache_key +------+-------+-------+--------+------------+-------------+--------+-------------+----------+----------- +(0 rows) + +-- NEGATIVE EXAMPLE: a freshly created slot (LSN reserved, as a real replica connection would) is wal_status='reserved' while inactive, e.g. a replica that just restarted, and must NOT fire. +do $$ begin perform pg_catalog.pg_create_physical_replication_slot('splinter_test_negative_slot', true); end $$; +select name, detail, cache_key from lint."0030_unused_replication_slot"; -- expect 0 rows + name | detail | cache_key +------+--------+----------- +(0 rows) + +select pg_catalog.pg_drop_replication_slot('splinter_test_negative_slot'); + pg_drop_replication_slot +-------------------------- + +(1 row) + +-- NEGATIVE EXAMPLE (logical slot): the same reserved/inactive guarantee applies to logical slots, not just physical +do $$ begin perform pg_catalog.pg_create_logical_replication_slot('splinter_test_negative_logical_slot', 'test_decoding'); end $$; +select name, detail, cache_key from lint."0030_unused_replication_slot"; -- expect 0 rows + name | detail | cache_key +------+--------+----------- +(0 rows) + +select pg_catalog.pg_drop_replication_slot('splinter_test_negative_logical_slot'); + pg_drop_replication_slot +-------------------------- + +(1 row) + +-- NEGATIVE EXAMPLE (extended): max_wal_size exceeded but max_slot_wal_keep_size left at its default (disabled), so the slot is 'extended', not 'unreserved', and must NOT fire +do $$ begin perform pg_catalog.pg_create_physical_replication_slot('splinter_test_extended_slot', true); end $$; +alter system set max_wal_size = '2MB'; +select pg_catalog.pg_reload_conf(); + pg_reload_conf +---------------- + t +(1 row) + +do $$ begin perform pg_temp.advance_wal(); end $$; +select slot_name, wal_status from pg_catalog.pg_replication_slots where slot_name = 'splinter_test_extended_slot'; -- confirm wal_status is actually 'extended' + slot_name | wal_status +-----------------------------+------------ + splinter_test_extended_slot | extended +(1 row) + +select name, detail, cache_key from lint."0030_unused_replication_slot"; -- expect 0 rows + name | detail | cache_key +------+--------+----------- +(0 rows) + +select pg_catalog.pg_drop_replication_slot('splinter_test_extended_slot'); + pg_drop_replication_slot +-------------------------- + +(1 row) + +alter system reset max_wal_size; +select pg_catalog.pg_reload_conf(); + pg_reload_conf +---------------- + t +(1 row) + +-- drain any checkpoint still pending from the WAL burst above; otherwise it can land mid-way through the positive fixture below and invalidate the slot before the 'unreserved' assertion runs. +checkpoint; +-- POSITIVE EXAMPLE (physical): shrink max_slot_wal_keep_size and generate enough WAL past it so the slot's retained WAL exceeds the limit. +do $$ begin perform pg_catalog.pg_create_physical_replication_slot('splinter_test_positive_slot', true); end $$; +alter system set max_slot_wal_keep_size = '1MB'; +select pg_catalog.pg_reload_conf(); + pg_reload_conf +---------------- + t +(1 row) + +do $$ begin perform pg_temp.advance_wal(); end $$; +-- before any checkpoint runs, the slot has exceeded max_slot_wal_keep_size but hasn't been invalidated yet: wal_status is 'unreserved', level WARN +select name, level, detail, cache_key from lint."0030_unused_replication_slot"; -- expect 1 row, wal_status unreserved + name | level | detail | cache_key +-------------------------+-------+-----------------------------------------------------------------------------------------------+---------------------------------------------------------------- + unused_replication_slot | WARN | Replication slot `splinter_test_positive_slot` is inactive and its wal_status is `unreserved` | unused_replication_slot_splinter_test_positive_slot_unreserved +(1 row) + +-- a checkpoint is what actually invalidates an 'unreserved' slot once its required WAL is gone: wal_status becomes 'lost', level ERROR +checkpoint; +select name, level, detail, cache_key from lint."0030_unused_replication_slot"; -- expect 1 row, wal_status lost + name | level | detail | cache_key +-------------------------+-------+-----------------------------------------------------------------------------------------+---------------------------------------------------------- + unused_replication_slot | ERROR | Replication slot `splinter_test_positive_slot` is inactive and its wal_status is `lost` | unused_replication_slot_splinter_test_positive_slot_lost +(1 row) + +-- dropping is the only way to stop WAL accumulation once the consumer is confirmed gone for good +select pg_catalog.pg_drop_replication_slot('splinter_test_positive_slot'); + pg_drop_replication_slot +-------------------------- + +(1 row) + +select * from lint."0030_unused_replication_slot"; -- expect 0 rows + name | title | level | facing | categories | description | detail | remediation | metadata | cache_key +------+-------+-------+--------+------------+-------------+--------+-------------+----------+----------- +(0 rows) + +-- POSITIVE EXAMPLE (logical): same escalation as the physical case, but also proves the entity/plugin metadata Studio needs for logical slots is actually populated. +do $$ begin perform pg_catalog.pg_create_logical_replication_slot('splinter_test_positive_logical_slot', 'test_decoding'); end $$; +alter system set max_slot_wal_keep_size = '1MB'; +select pg_catalog.pg_reload_conf(); + pg_reload_conf +---------------- + t +(1 row) + +do $$ begin perform pg_temp.advance_wal(); end $$; +select name, level, metadata ->> 'entity' as entity, metadata ->> 'plugin' as plugin, metadata ->> 'database' as database, cache_key from lint."0030_unused_replication_slot"; -- expect 1 row, wal_status unreserved + name | level | entity | plugin | database | cache_key +-------------------------+-------+-------------------------------------+---------------+--------------------+------------------------------------------------------------------------ + unused_replication_slot | WARN | splinter_test_positive_logical_slot | test_decoding | contrib_regression | unused_replication_slot_splinter_test_positive_logical_slot_unreserved +(1 row) + +checkpoint; +select name, level, metadata ->> 'entity' as entity, metadata ->> 'plugin' as plugin, metadata ->> 'database' as database, cache_key from lint."0030_unused_replication_slot"; -- expect 1 row, wal_status lost + name | level | entity | plugin | database | cache_key +-------------------------+-------+-------------------------------------+---------------+--------------------+------------------------------------------------------------------ + unused_replication_slot | ERROR | splinter_test_positive_logical_slot | test_decoding | contrib_regression | unused_replication_slot_splinter_test_positive_logical_slot_lost +(1 row) + +select pg_catalog.pg_drop_replication_slot('splinter_test_positive_logical_slot'); + pg_drop_replication_slot +-------------------------- + +(1 row) + +select * from lint."0030_unused_replication_slot"; -- expect 0 rows + name | title | level | facing | categories | description | detail | remediation | metadata | cache_key +------+-------+-------+--------+------------+-------------+--------+-------------+----------+----------- +(0 rows) + +alter system reset max_slot_wal_keep_size; +alter system reset checkpoint_timeout; +select pg_catalog.pg_reload_conf(); + pg_reload_conf +---------------- + t +(1 row) + diff --git a/test/expected/queries_are_unionable.out b/test/expected/queries_are_unionable.out index 3b1005d..563d8ac 100644 --- a/test/expected/queries_are_unionable.out +++ b/test/expected/queries_are_unionable.out @@ -54,7 +54,9 @@ begin; union all select * from lint."0028_anon_security_definer_function_executable" union all - select * from lint."0029_authenticated_security_definer_function_executable"; + select * from lint."0029_authenticated_security_definer_function_executable" + union all + select * from lint."0030_unused_replication_slot"; name | title | level | facing | categories | description | detail | remediation | metadata | cache_key ------+-------+-------+--------+------------+-------------+--------+-------------+----------+----------- (0 rows) diff --git a/test/sql/0030_unused_replication_slot.sql b/test/sql/0030_unused_replication_slot.sql new file mode 100644 index 0000000..30d7be9 --- /dev/null +++ b/test/sql/0030_unused_replication_slot.sql @@ -0,0 +1,85 @@ +-- Unlike other lint tests, this file cannot use begin/savepoint/rollback: replication slots are non-transactional (they survive a rollback) and ALTER SYSTEM is rejected inside a transaction block, so every statement here autocommits individually and cleanup is explicit instead. +set search_path = ''; + +-- Converge to a clean starting state instead of trusting the tail cleanup below. bin/installcheck initdb's a fresh cluster per run, so this has no effect there, but it makes the file safe to re-run by hand (psql -f) against a persistent dev cluster without a stray splinter_test_* slot or non-default GUC from a prior manual run causing a confusing failure. +do $$ +declare + stray record; +begin + for stray in select slot_name from pg_catalog.pg_replication_slots where slot_name like 'splinter_test_%' loop + perform pg_catalog.pg_drop_replication_slot(stray.slot_name); + end loop; +end $$; +alter system reset max_wal_size; +alter system reset max_slot_wal_keep_size; +-- pins the checkpointer to explicit `checkpoint;` calls below; the default 300s timeout could otherwise fire mid-fixture on a slow runner and desync the WARN/ERROR assertions. +alter system set checkpoint_timeout = '1h'; +select pg_catalog.pg_reload_conf(); + +-- pg_switch_wal() is a no-op at a segment boundary, so each iteration writes one trivial WAL record first to move off it before switching again; 10 iterations assumes the default 16MB wal_segment_size. +create function pg_temp.advance_wal() returns void language plpgsql as $fn$ +begin + for i in 1..10 loop + perform pg_catalog.pg_logical_emit_message(false, 'splinter_test', 'advance'); + perform pg_catalog.pg_switch_wal(); + end loop; +end +$fn$; + +-- BASELINE: 0 issues, no slots exist. None of the fixtures below exercise `active = true`: pg_regress has no easy way to hold open a real walsender connection, so the view's `active = false` predicate itself is untested here. +select * from lint."0030_unused_replication_slot"; + +-- NEGATIVE EXAMPLE: a freshly created slot (LSN reserved, as a real replica connection would) is wal_status='reserved' while inactive, e.g. a replica that just restarted, and must NOT fire. +do $$ begin perform pg_catalog.pg_create_physical_replication_slot('splinter_test_negative_slot', true); end $$; +select name, detail, cache_key from lint."0030_unused_replication_slot"; -- expect 0 rows +select pg_catalog.pg_drop_replication_slot('splinter_test_negative_slot'); + +-- NEGATIVE EXAMPLE (logical slot): the same reserved/inactive guarantee applies to logical slots, not just physical +do $$ begin perform pg_catalog.pg_create_logical_replication_slot('splinter_test_negative_logical_slot', 'test_decoding'); end $$; +select name, detail, cache_key from lint."0030_unused_replication_slot"; -- expect 0 rows +select pg_catalog.pg_drop_replication_slot('splinter_test_negative_logical_slot'); + +-- NEGATIVE EXAMPLE (extended): max_wal_size exceeded but max_slot_wal_keep_size left at its default (disabled), so the slot is 'extended', not 'unreserved', and must NOT fire +do $$ begin perform pg_catalog.pg_create_physical_replication_slot('splinter_test_extended_slot', true); end $$; +alter system set max_wal_size = '2MB'; +select pg_catalog.pg_reload_conf(); +do $$ begin perform pg_temp.advance_wal(); end $$; +select slot_name, wal_status from pg_catalog.pg_replication_slots where slot_name = 'splinter_test_extended_slot'; -- confirm wal_status is actually 'extended' +select name, detail, cache_key from lint."0030_unused_replication_slot"; -- expect 0 rows +select pg_catalog.pg_drop_replication_slot('splinter_test_extended_slot'); +alter system reset max_wal_size; +select pg_catalog.pg_reload_conf(); +-- drain any checkpoint still pending from the WAL burst above; otherwise it can land mid-way through the positive fixture below and invalidate the slot before the 'unreserved' assertion runs. +checkpoint; + +-- POSITIVE EXAMPLE (physical): shrink max_slot_wal_keep_size and generate enough WAL past it so the slot's retained WAL exceeds the limit. +do $$ begin perform pg_catalog.pg_create_physical_replication_slot('splinter_test_positive_slot', true); end $$; +alter system set max_slot_wal_keep_size = '1MB'; +select pg_catalog.pg_reload_conf(); +do $$ begin perform pg_temp.advance_wal(); end $$; + +-- before any checkpoint runs, the slot has exceeded max_slot_wal_keep_size but hasn't been invalidated yet: wal_status is 'unreserved', level WARN +select name, level, detail, cache_key from lint."0030_unused_replication_slot"; -- expect 1 row, wal_status unreserved + +-- a checkpoint is what actually invalidates an 'unreserved' slot once its required WAL is gone: wal_status becomes 'lost', level ERROR +checkpoint; +select name, level, detail, cache_key from lint."0030_unused_replication_slot"; -- expect 1 row, wal_status lost + +-- dropping is the only way to stop WAL accumulation once the consumer is confirmed gone for good +select pg_catalog.pg_drop_replication_slot('splinter_test_positive_slot'); +select * from lint."0030_unused_replication_slot"; -- expect 0 rows + +-- POSITIVE EXAMPLE (logical): same escalation as the physical case, but also proves the entity/plugin metadata Studio needs for logical slots is actually populated. +do $$ begin perform pg_catalog.pg_create_logical_replication_slot('splinter_test_positive_logical_slot', 'test_decoding'); end $$; +alter system set max_slot_wal_keep_size = '1MB'; +select pg_catalog.pg_reload_conf(); +do $$ begin perform pg_temp.advance_wal(); end $$; +select name, level, metadata ->> 'entity' as entity, metadata ->> 'plugin' as plugin, metadata ->> 'database' as database, cache_key from lint."0030_unused_replication_slot"; -- expect 1 row, wal_status unreserved +checkpoint; +select name, level, metadata ->> 'entity' as entity, metadata ->> 'plugin' as plugin, metadata ->> 'database' as database, cache_key from lint."0030_unused_replication_slot"; -- expect 1 row, wal_status lost +select pg_catalog.pg_drop_replication_slot('splinter_test_positive_logical_slot'); +select * from lint."0030_unused_replication_slot"; -- expect 0 rows + +alter system reset max_slot_wal_keep_size; +alter system reset checkpoint_timeout; +select pg_catalog.pg_reload_conf(); diff --git a/test/sql/queries_are_unionable.sql b/test/sql/queries_are_unionable.sql index fe43d3d..70393d4 100644 --- a/test/sql/queries_are_unionable.sql +++ b/test/sql/queries_are_unionable.sql @@ -56,6 +56,8 @@ begin; union all select * from lint."0028_anon_security_definer_function_executable" union all - select * from lint."0029_authenticated_security_definer_function_executable"; + select * from lint."0029_authenticated_security_definer_function_executable" + union all + select * from lint."0030_unused_replication_slot"; rollback; From b7775e27232c38f9673582dd371ec9bfec950d07 Mon Sep 17 00:00:00 2001 From: Douglas J Hunley Date: Fri, 4 Sep 2026 16:07:25 -0400 Subject: [PATCH 2/2] fix: correct stale 0030 test label and merge stacked comments post-renumbering The lint's own renumbering to 0031 left a mislabeled pinning-test case name and two newly-added 2-3 line stacked comments behind. --- bin/check_lints.py | 4 +--- bin/test_check_lints.py | 5 ++--- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/bin/check_lints.py b/bin/check_lints.py index 8fe8284..50b889d 100644 --- a/bin/check_lints.py +++ b/bin/check_lints.py @@ -165,9 +165,7 @@ def check() -> list[str]: if not level_match: errors.append(f"{doc}: missing a '**Level:**' line") else: - # An empty set legitimately means "nothing to compare against" -- a doc with no - # matching lint view (e.g. 0012), or one whose own level column already errored - # above -- not a silent pass; _level_line_error skips the comparison either way. + # Empty set (no matching lint view, or its own level column already errored above) skips the comparison in _level_line_error rather than silently passing. doc_sql_levels = levels_by_number.get(doc.stem.split("_", 1)[0], set()) level_error = _level_line_error(level_match.group(1), doc_sql_levels) if level_error: diff --git a/bin/test_check_lints.py b/bin/test_check_lints.py index 577166e..cbf4bd1 100644 --- a/bin/test_check_lints.py +++ b/bin/test_check_lints.py @@ -15,7 +15,7 @@ CASES = { "bare literal": ("'INFO' as level,", {"INFO"}), - "single-line case (0030's own shape)": ( + "single-line case (0031's own shape)": ( "case when prs.wal_status = 'lost' then 'ERROR' else 'WARN' end as level,", {"ERROR", "WARN"}, ), @@ -45,8 +45,7 @@ if actual != expected: failures.append(f"{name}: expected {expected!r}, got {actual!r}") -# _level_line_error: an empty sql_levels set means "nothing to compare against", never a -# silent pass, and a doc mentioning no level word is always an error regardless. +# An empty sql_levels set means _level_line_error skips the comparison rather than silently passing; a doc mentioning no level word is still always an error. if _level_line_error("WARN", set()) is not None: failures.append( "_level_line_error('WARN', set()) should skip comparison, not error"