From d6729f993b64faf46ada4bd3543067505fb2c0fe Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Mon, 27 Jul 2026 11:36:36 +0200 Subject: [PATCH 01/27] ci: release on bare semver tags, not just v-prefixed Tags are 0.3.0, not v0.3.0. The workflow only triggered on "v*", so adopting that convention would have meant pushing 0.3.0 and getting no release -- silently, since a tag matching no filter produces no workflow run at all. That reads as a slow release rather than one that never started. Adds a bare-semver glob and keeps "v*" as a safety net, because tag filters are globs rather than regexes and cannot be made to reject a prefix: matching both means an out-of-habit v0.3.0 still publishes instead of vanishing. Drop the v* line to make that mistake fail loudly instead. Verified the globs against 0.3.0, 0.10.2, 1.0.0, v0.3.0, main and release-0.3.0. Co-Authored-By: Claude Opus 5 --- .github/workflows/release.yml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index ee48192..53c963f 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -9,6 +9,15 @@ name: Release on: push: tags: + # Release tags are bare semver: 0.3.0, not v0.3.0. + # + # `v*` is kept only as a safety net. Tag filters are globs, not regexes, so a bare + # pattern cannot be made to reject a `v` prefix — and a pushed tag that matches + # nothing fails *silently*: no workflow run appears, so it looks like a release that + # is merely slow rather than one that never started. Matching both means an + # out-of-habit `v0.3.0` still publishes. Drop the `v*` line if you would rather that + # mistake fail loudly. + - "[0-9]*.[0-9]*.[0-9]*" - "v*" jobs: From 00a19c4660a0577e0c35b94fdd3807e15cd32d94 Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Mon, 27 Jul 2026 11:43:34 +0200 Subject: [PATCH 02/27] docs: implementation plan for Postgres advise hardening (batch 1 of 3) Seven TDD tasks covering the Batch-1 follow-ups recorded during the advise plan's fifteen task reviews and its final whole-branch review: - Extract engine-neutral credential handling into workload/secrets.py, before a second adapter re-derives scrubbing by hand. That code took three fix rounds to get right (driver text quoting the value, `from None` suppressing only the traceback, urlparse returning a still-encoded DSN password) and the final reviewer named this the seam that matters. - Stop CAP_INDEXES discarding expression columns. Postgres stores 0 in indkey for an expression position and no pg_attribute row has attnum 0, so the inner join meant an index on lower(status) arrived with an EMPTY column tuple. LEFT JOIN plus indpred/indexprs/pg_get_indexdef fixes it. - Consequently: a partial index stops being treated as coverage (it does not serve an unfiltered lookup, so calling it coverage silently withheld real proposals), expression indexes get disclosed rather than ignored, and ADV003 earns HIGH back for genuinely plain pairs while skipping the rest entirely -- "probably wrong" is not a confidence level. - A real Postgres behind an opt-in marker. Not one introspection statement in this feature has ever executed against a server; they are only diffed for drift, which cannot catch a wrong column name or a missing view. Deselected by default via addopts so `uv run pytest` stays green without Docker. - Two recorded trivia: fingerprints becomes a property over fingerprint_ids (two fields, one fact, kept in step by convention only), and star_tables stops compiling a regex per (stat x table) pair. Batch 2 (join-key and grouping proposals, DECLARE/COPY unwrapping, multi-schema keying) and Batch 3 (Redshift, Snowflake, dbt enrichment) stay in the ledger. Those change what advise says; this batch changes whether it can be trusted. Co-Authored-By: Claude Opus 5 --- .../2026-07-27-advise-postgres-hardening.md | 1247 +++++++++++++++++ 1 file changed, 1247 insertions(+) create mode 100644 docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md diff --git a/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md b/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md new file mode 100644 index 0000000..ae071cf --- /dev/null +++ b/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md @@ -0,0 +1,1247 @@ +# Postgres `advise` Hardening Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make the shipped Postgres `advise` adapter trustworthy — teach it about expression and partial indexes, validate its introspection SQL against a real server for the first time, and move the credential-scrubbing helpers somewhere a second adapter cannot bypass them. + +**Architecture:** Three independent strands. The index-metadata strand widens `CAP_INDEXES` to carry `indpred`/`indexprs` and stops the `pg_attribute` join silently discarding expression columns, which lets `_covered` and ADV003 stop guessing. The integration strand adds a real Postgres behind an opt-in pytest marker, so the six introspection statements are finally executed rather than only diffed. The extraction strand moves engine-neutral secret handling into its own module before Redshift can hand-roll it. + +**Tech Stack:** Python 3.11+, psycopg 3, Postgres 16 via Docker, pytest markers, sqlglot 30.12. + +Follow-up to `docs/superpowers/plans/2026-07-26-advise-postgres.md` (shipped in PRs #9/#10). Every item here was recorded as deferred during that plan's fifteen task reviews or its final whole-branch review; the reasoning for each is in `.superpowers/sdd/2026-07-26-advise-postgres/progress.md`. + +## Global Constraints + +- Python `>=3.11`. Every new module starts with `from __future__ import annotations`. +- Ruff line length 100. +- **CI gates all four of these over the whole repo; every task must pass all four before committing:** + ``` + uv run ruff check . + uv run ruff format --check . + uv run mypy src/sqlquality + uv run pytest + ``` + The code blocks below are written for readability and are **not** guaranteed `ruff format` clean. Run `uv run ruff format .` after transcribing and commit the formatted result. +- Baseline at the start of this plan: **411 tests**, `main` at `d6729f9`. +- Invariants from the shipped feature that must survive every task: + 1. sqlquality never executes user SQL; only the statements in `PostgresWorkloadAdapter.SQL` ever run. `advise` never issues DDL or DML. + 2. `connect()` sets `default_transaction_read_only` and a statement timeout before the session is usable. + 3. No secret reaches stdout, stderr, an exception message, or the exception chain. + 4. A missing grant costs exactly one capability, never the whole run. + 5. `advise` exits 0 on any successful analysis and 2 on error — **never 1**. + 6. Absent evidence lowers confidence and says so in the artifact the operator reads. +- **New introspection SQL must keep `tests/test_workload_postgres.py`'s write-verb guard green.** `_write_verbs_in` matches whole words via `\b`, so a statement containing the *word* `create` — including inside a `pg_get_indexdef()` result at runtime — is fine, but the statement text itself must not contain one. +- The integration tests must be **skipped by default**. A contributor without Docker runs `uv run pytest` and sees no failures and no errors. + +## File Structure + +**Create:** + +| File | Responsibility | +|---|---| +| `src/sqlquality/workload/secrets.py` | Engine-neutral credential handling: which fields are secret, how to collect them from a `ConnectionParams`, how to scrub them out of driver text, and the statement-timeout clamp. | +| `tests/integration/__init__.py` | Marks the integration package. | +| `tests/integration/conftest.py` | The opt-in gate and the live-connection fixture. | +| `tests/integration/docker-compose.yml` | Postgres 16 with `pg_stat_statements` preloaded. | +| `tests/integration/test_introspection_live.py` | Executes all six introspection statements against a real server and asserts their shapes. | +| `tests/integration/test_advise_live.py` | One end-to-end `advise` run against a seeded database. | +| `tests/test_workload_secrets.py` | Moves the secret-handling unit tests to sit beside their new module. | + +**Modify:** `src/sqlquality/workload/postgres.py` (widened `CAP_INDEXES`, richer `PgIndex`, coverage and ADV003 changes, secrets re-exported from the new module), `src/sqlquality/workload/aggregate.py` (`star_tables` regex cache), `src/sqlquality/models.py` (`ColumnUsage.fingerprints` becomes a property), `pyproject.toml` (pytest marker), `CONTRIBUTING.md` (how to run the integration suite), `README.md` (two limitations become narrower). + +`secrets.py` is deliberately its own module rather than a section of `postgres.py`: the final whole-branch review identified it as the seam that matters, because a Redshift or Snowflake `connect()` that cannot see these helpers will re-implement scrubbing by hand, and scrubbing is the one thing on this branch that took three fix rounds to get right. + +--- + +### Task 1: Extract engine-neutral credential handling + +**Files:** +- Create: `src/sqlquality/workload/secrets.py` +- Modify: `src/sqlquality/workload/postgres.py:84-170` (remove the moved definitions, import them instead) +- Create: `tests/test_workload_secrets.py` +- Modify: `tests/test_workload_postgres.py` (move the secret-handling tests out) + +**Interfaces:** +- Consumes: `ConnectionParams` from `sqlquality.models`. +- Produces, all importable from `sqlquality.workload.secrets`: + - `SECRET_FIELDS: frozenset[str]` + - `MIN_SCRUBBABLE_SECRET: int` + - `WITHHELD: str` + - `secrets_for(params: ConnectionParams) -> tuple[str, ...]` + - `scrub(text: str, secrets: Iterable[str]) -> str` + - `clamp_timeout_ms(timeout_s: int, *, minimum: int, maximum: int) -> int` + +Note the rename from the private `_`-prefixed names: these are now a module boundary other adapters import across, so they are public. `clamp_timeout_ms` gains explicit bounds parameters rather than reading module constants, because the CLI already owns the user-facing bounds and duplicating them was a recorded finding (`_TIMEOUT_MIN_S`/`_MAX_S` in `cli.py` versus `_MIN_TIMEOUT_S`/`_MAX_TIMEOUT_S` in `postgres.py`). + +- [ ] **Step 1: Write the failing test** + +Create `tests/test_workload_secrets.py`: + +```python +from __future__ import annotations + +import pytest + +from sqlquality.models import ConnectionParams +from sqlquality.workload.secrets import ( + MIN_SCRUBBABLE_SECRET, + SECRET_FIELDS, + WITHHELD, + clamp_timeout_ms, + scrub, + secrets_for, +) + + +def _params(**kwargs) -> ConnectionParams: + base = {"engine": "postgres", "dsn": None, "fields": {}, "source": "--dsn"} + base.update(kwargs) + return ConnectionParams(**base) # type: ignore[arg-type] + + +def test_secrets_for_collects_password_fields(): + assert secrets_for(_params(fields={"host": "db", "password": "hunter2"})) == ("hunter2",) + + +def test_secrets_for_covers_both_forms_of_a_dsn_password(): + """urlparse leaves the password encoded; libpq decodes it before authenticating.""" + got = secrets_for(_params(dsn="postgresql://u:p%40ss@h/db")) + assert "p%40ss" in got + assert "p@ss" in got + + +def test_secrets_for_tolerates_a_dsn_with_no_password_or_a_malformed_one(): + for dsn in ("postgresql://u@h/db", "not a valid dsn :: at all ///"): + assert secrets_for(_params(dsn=dsn)) == (dsn,) + + +def test_scrub_redacts_a_present_secret(): + assert scrub('failed for user "u" (hunter2)', ("hunter2",)) == 'failed for user "u" (***)' + + +def test_scrub_withholds_rather_than_mangles_an_unredactable_secret(): + assert scrub("a database has an admin", ("a",)) == WITHHELD + assert scrub("connection refused", ("a",)) == "connection refused" + + +def test_min_scrubbable_secret_is_the_documented_floor(): + assert MIN_SCRUBBABLE_SECRET == 4 + assert "password" in SECRET_FIELDS + + +@pytest.mark.parametrize( + ("given", "expected_ms"), + [(0, 1_000), (-5, 1_000), (30, 30_000), (99_999, 3_600_000)], +) +def test_clamp_timeout_ms_bounds_and_converts(given, expected_ms): + assert clamp_timeout_ms(given, minimum=1, maximum=3600) == expected_ms +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `uv run pytest tests/test_workload_secrets.py -v` +Expected: FAIL with `ModuleNotFoundError: No module named 'sqlquality.workload.secrets'` + +- [ ] **Step 3: Write minimal implementation** + +Create `src/sqlquality/workload/secrets.py` by **moving** — not copying — the bodies currently in `postgres.py`, renaming them public and dropping the module-constant coupling in the clamp: + +```python +"""Credential handling shared by every workload adapter. + +This lives outside any one adapter deliberately. Scrubbing took three fix rounds to get +right on the Postgres adapter — the driver's exception text quoted the offending value, then +`from None` turned out to suppress only the traceback while leaving `__context__` reachable, +then a percent-encoded DSN password slipped past because ``urlparse`` returns it still +encoded. An adapter that cannot see these helpers will re-derive that sequence badly. +""" + +from __future__ import annotations + +from collections.abc import Iterable +from urllib.parse import unquote, urlparse + +from sqlquality.models import ConnectionParams + +#: profiles.yml keys whose values must never appear in any message we emit. +SECRET_FIELDS = frozenset({"password", "pass"}) + +#: A secret shorter than this cannot be redacted by substring replacement without destroying +#: the message — a one-character password would blank every occurrence of that letter. When +#: one actually appears, the driver's text is withheld rather than mangled. +MIN_SCRUBBABLE_SECRET = 4 +WITHHELD = "(driver message withheld: it contained a value too short to redact safely)" + + +def secrets_for(params: ConnectionParams) -> tuple[str, ...]: + """Every value we know to be secret for this connection. + + A DSN is added *and* its password extracted separately, in both its percent-encoded and + decoded forms. The whole-DSN token only helps if the driver echoes the connection string + back verbatim, which real libpq errors do not do — they report the offending value on its + own. And ``urlparse().password`` returns it still encoded while libpq decodes a URI DSN + before authenticating, so for ``postgresql://u:p%40ss@h/db`` the driver reports ``p@ss`` + while urlparse yields ``p%40ss``: a token of only the encoded form never matches. + """ + secrets = tuple( + value for key, value in params.fields.items() if key in SECRET_FIELDS and value + ) + if params.dsn: + secrets += (params.dsn,) + encoded = urlparse(params.dsn).password + if encoded: + secrets += (encoded,) + decoded = unquote(encoded) + if decoded != encoded: + secrets += (decoded,) + return secrets + + +def scrub(text: str, secrets: Iterable[str]) -> str: + """Replace any known secret occurring in ``text`` with a redaction marker. + + Defence in depth for driver exceptions. libpq is not believed to echo a password, but the + auth-failure path — the most common real connect failure — cannot be exercised without a + live server, and we hold the secret anyway, so its absence can be guaranteed rather than + trusted. + """ + present = [secret for secret in secrets if secret and secret in text] + if any(len(secret) < MIN_SCRUBBABLE_SECRET for secret in present): + return WITHHELD + scrubbed = text + for secret in present: + scrubbed = scrubbed.replace(secret, "***") + return scrubbed + + +def clamp_timeout_ms(timeout_s: int, *, minimum: int, maximum: int) -> int: + """Statement timeout in milliseconds, clamped into ``[minimum, maximum]`` seconds. + + Bounds are parameters rather than module constants: the CLI owns the user-facing range + and rejects out-of-range input, so a second copy of the numbers here could drift out of + step with the message the user was shown. + """ + return max(minimum, min(int(timeout_s), maximum)) * 1000 +``` + +In `postgres.py`, delete `_SECRET_FIELDS`, `_MIN_SCRUBBABLE_SECRET`, `_WITHHELD`, `_secrets_for`, `_scrub` and `_clamp_timeout_ms`, and import instead: + +```python +from sqlquality.workload.secrets import clamp_timeout_ms, scrub, secrets_for +``` + +Update the two call sites in `connect()`: `secrets = secrets_for(params)` and `scrub(str(exc), secrets)`, and the timeout call becomes: + +```python + cursor.execute( + "SELECT set_config('statement_timeout', %s, false)", + (f"{clamp_timeout_ms(timeout_s, minimum=1, maximum=3600)}ms",), + ) +``` + +Keep `_pg_fields` and `_PG_FIELD_MAP` in `postgres.py` — they map to libpq keywords and are not engine-neutral. + +- [ ] **Step 4: Move the existing tests rather than duplicating them** + +`tests/test_workload_postgres.py` currently holds tests for these helpers. Delete the ones now covered by `tests/test_workload_secrets.py` — specifically any asserting on `_secrets_for`, `_scrub`, percent-encoded DSN passwords, or the withheld message. **Keep** `test_connect_scrubs_a_password_from_a_driver_failure`, which exercises the adapter's use of them rather than the helpers themselves, and update it to patch or reference the new names if it does so directly. + +Run: `uv run pytest tests/test_workload_secrets.py tests/test_workload_postgres.py -v` +Expected: PASS, with no test name appearing in both files. + +- [ ] **Step 5: Run the full gates** + +Run: `uv run ruff format . && uv run ruff check . && uv run ruff format --check . && uv run mypy src/sqlquality && uv run pytest -q` +Expected: all pass. Total count should be unchanged or slightly higher — if it *dropped*, a test was deleted without a replacement. + +- [ ] **Step 6: Commit** + +```bash +git add src/sqlquality/workload/secrets.py src/sqlquality/workload/postgres.py tests/test_workload_secrets.py tests/test_workload_postgres.py +git commit -m "refactor(workload): move credential handling into its own module" +``` + +--- + +### Task 2: Stop discarding expression-index columns + +**Files:** +- Modify: `src/sqlquality/workload/postgres.py` — `CAP_INDEXES` SQL, `PgIndex`, `_IndexRows`, `fetch_indexes` +- Test: `tests/test_workload_postgres.py` + +**Interfaces:** +- Consumes: `_as_int`, `_run`, `CAP_INDEXES` (existing). +- Produces: `PgIndex` gains three fields, in this exact order after `size_bytes`: + ```python + is_partial: bool = False + predicate: str | None = None + has_expressions: bool = False + ``` + All defaulted, so existing test constructors that pass six positional or keyword arguments keep working. Task 3 and Task 4 read all three. + +**The defect being fixed.** `CAP_INDEXES` inner-joins `pg_attribute` on `a.attnum = k.attnum`. Postgres stores **0** in `indkey` for an expression column, and no `pg_attribute` row has `attnum = 0`, so every expression column is silently dropped — an index on `lower(status)` currently arrives with an *empty* column tuple, and `_covered` then compares against `()`. Recorded during Task 7's review and deferred. + +- [ ] **Step 1: Write the failing test** + +Append to `tests/test_workload_postgres.py`: + +```python +def test_fetch_indexes_records_an_expression_index_rather_than_dropping_it(): + """`indkey` holds 0 for an expression column and no pg_attribute row has attnum 0. + + The old inner join therefore discarded those rows, so an index on `lower(status)` + arrived with an empty column tuple and could not be reasoned about at all. + """ + querier = FakeQuerier({"pg_index": [ + # attname is NULL for the expression column, as a LEFT JOIN yields. + ("orders", "idx_lower_status", None, 1, False, False, 3, 8192, + False, None, True, "CREATE INDEX idx_lower_status ON orders (lower(status))"), + ]}) + indexes = PostgresWorkloadAdapter(querier=querier).fetch_indexes( + ("public",), frozenset({"orders"}) + ) + index = indexes["orders"][0] + assert index.has_expressions is True + assert index.columns == () + assert "lower(status)" in (index.definition or "") + + +def test_fetch_indexes_records_a_partial_index_predicate(): + querier = FakeQuerier({"pg_index": [ + ("orders", "idx_open", "status", 1, False, False, 7, 4096, + True, "(shipped_at IS NULL)", False, + "CREATE INDEX idx_open ON orders (status) WHERE shipped_at IS NULL"), + ]}) + index = PostgresWorkloadAdapter(querier=querier).fetch_indexes( + ("public",), frozenset({"orders"}) + )["orders"][0] + assert index.is_partial is True + assert index.predicate == "(shipped_at IS NULL)" + assert index.columns == ("status",) + + +def test_fetch_indexes_leaves_a_plain_index_unmarked(): + querier = FakeQuerier({"pg_index": [ + ("orders", "idx_status", "status", 1, False, False, 12, 4096, + False, None, False, "CREATE INDEX idx_status ON orders (status)"), + ]}) + index = PostgresWorkloadAdapter(querier=querier).fetch_indexes( + ("public",), frozenset({"orders"}) + )["orders"][0] + assert (index.is_partial, index.predicate, index.has_expressions) == (False, None, False) + + +def test_the_indexes_statement_reads_predicate_and_expression_metadata(): + sql = PostgresWorkloadAdapter().SQL[CAP_INDEXES].lower() + assert "indpred" in sql, "the partial-index predicate must be selected" + assert "indexprs" in sql, "expression presence must be selected" + assert "left join pg_attribute" in sql, ( + "an inner join drops expression columns, whose indkey entry is 0" + ) +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `uv run pytest tests/test_workload_postgres.py -k "expression_index or partial_index or unmarked or predicate_and_expression" -v` +Expected: FAIL — `ValueError: too many values to unpack` from `fetch_indexes`, and the SQL assertions fail on the missing clauses. + +- [ ] **Step 3: Widen the statement** + +Replace `CAP_INDEXES` in `PostgresWorkloadAdapter.SQL`: + +```python + # LEFT JOIN, not JOIN: Postgres stores 0 in indkey for an expression column and no + # pg_attribute row has attnum 0, so an inner join silently discarded every expression + # index's columns — they arrived with an empty tuple. The NULL attname a LEFT JOIN + # yields is what tells us the position was an expression. + # + # indpred / indexprs are selected as booleans plus the rendered predicate, because a + # partial index does not serve an unfiltered lookup and an expression index does not + # serve its bare column — both of which the coverage and redundancy rules previously + # had to guess at. + CAP_INDEXES: """ + SELECT t.relname, i.relname, a.attname, k.ordinality, + ix.indisunique, ix.indisprimary, + COALESCE(psui.idx_scan, 0), pg_relation_size(i.oid), + ix.indpred IS NOT NULL, + pg_get_expr(ix.indpred, ix.indrelid), + ix.indexprs IS NOT NULL, + pg_get_indexdef(ix.indexrelid) + FROM pg_index ix + JOIN pg_class i ON i.oid = ix.indexrelid + JOIN pg_class t ON t.oid = ix.indrelid + JOIN pg_namespace n ON n.oid = t.relnamespace + JOIN LATERAL unnest(ix.indkey) WITH ORDINALITY AS k(attnum, ordinality) ON TRUE + LEFT JOIN pg_attribute a ON a.attrelid = t.oid AND a.attnum = k.attnum + LEFT JOIN pg_stat_user_indexes psui ON psui.indexrelid = i.oid + WHERE n.nspname = ANY(%s) AND t.relname = ANY(%s) + ORDER BY t.relname, i.relname, k.ordinality + """, +``` + +- [ ] **Step 4: Carry the new columns through** + +Extend `PgIndex`: + +```python + size_bytes: int + #: True when the index has a WHERE predicate. A partial index does not serve an + #: unfiltered lookup, so it can never be assumed to cover a proposed index. + is_partial: bool = False + #: The rendered predicate, for showing an operator why a drop was not recommended. + predicate: str | None = None + #: True when any indexed position is an expression rather than a plain column. Such a + #: position contributes no name to `columns`, so the tuple understates the index. + has_expressions: bool = False + #: The full CREATE INDEX text, the only place an expression is legible. + definition: str | None = None +``` + +Extend `_IndexRows` with the same four fields (mutable dataclass, so plain defaults), and rewrite the unpack in `fetch_indexes`: + +```python + for row in self._run(CAP_INDEXES, (list(schemas), sorted(tables))): + (table, index, column, ordinality, unique, primary, scans, size, + is_partial, predicate, has_expressions, definition) = row + entry = grouped.setdefault( + (str(table), str(index)), + _IndexRows( + is_unique=bool(unique), + is_primary=bool(primary), + scans=_as_int(scans), + size_bytes=_as_int(size) if size is not None else 0, + is_partial=bool(is_partial), + predicate=str(predicate) if predicate is not None else None, + has_expressions=bool(has_expressions), + definition=str(definition) if definition is not None else None, + ), + ) + # A NULL attname is an expression position: it has no column name to record, and + # `has_expressions` already marks the index, so skip it rather than storing "None". + if column is not None: + entry.columns.append((_as_int(ordinality), str(column))) +``` + +and pass the four through when building each `PgIndex`. + +- [ ] **Step 5: Run the tests** + +Run: `uv run pytest tests/test_workload_postgres.py tests/test_workload_rules.py -q` +Expected: PASS. The rules tests construct `PgIndex` with six arguments; the new fields are defaulted, so they must still pass unchanged. If any fails, the defaults are wrong — report it rather than editing the rules tests. + +- [ ] **Step 6: Gates and commit** + +```bash +uv run ruff format . && uv run ruff check . && uv run ruff format --check . && uv run mypy src/sqlquality && uv run pytest -q +git add src/sqlquality/workload/postgres.py tests/test_workload_postgres.py +git commit -m "fix(advise): stop discarding expression-index columns from the catalog" +``` + +--- + +### Task 3: A partial or expression index no longer counts as covering + +**Files:** +- Modify: `src/sqlquality/workload/postgres.py` — `_covered`, `propose_indexes` +- Test: `tests/test_workload_rules.py` + +**Interfaces:** +- Consumes: `PgIndex.is_partial` / `.has_expressions` / `.definition` (Task 2); `_is_prefix`, `_covered`, `propose_indexes` (existing). +- Produces: `_covered` keeps its signature `(candidate, existing) -> str | None`. `propose_indexes` gains two evidence keys, `"expression_indexes"` (a tuple of index names) and `"partial_indexes_skipped"` (a tuple of names), and appends a rationale sentence when either is non-empty. + +**Why this is two changes, not one.** Suppression and disclosure pull opposite ways here. A partial index must *stop* suppressing a candidate — `idx ON orders(status) WHERE shipped_at IS NULL` does not serve `WHERE status = $1`, so treating it as coverage silently withholds a good proposal. But an expression index must not be silently ignored either: if `lower(status)` is indexed and we propose `status`, the proposal may be redundant in a way we cannot prove. So: neither suppresses, and both are disclosed. + +- [ ] **Step 1: Write the failing test** + +Append to `tests/test_workload_rules.py`: + +```python +def test_a_partial_index_does_not_suppress_a_candidate(): + """`idx ON orders(status) WHERE shipped_at IS NULL` does not serve `WHERE status = $1`. + + Treating it as coverage silently withheld a good proposal — the inverse of the + confidently-wrong failures, and just as invisible. + """ + existing = {"orders": ( + PgIndex("idx_open", ("status",), False, False, 5, 4096, + is_partial=True, predicate="(shipped_at IS NULL)"), + )} + proposals = propose_indexes( + [usage("status", ColumnRole.EQUALITY)], facts(), existing, min_cost_share=0.01, + ) + assert codes(proposals) == ["ADV001"] + assert proposals[0].evidence["partial_indexes_skipped"] == ("idx_open",) + assert "partial" in proposals[0].rationale.lower() + + +def test_a_plain_index_still_suppresses_a_candidate(): + """The control. Task 2's new fields default to False, so this must not have changed.""" + existing = {"orders": (PgIndex("idx_status", ("status",), False, False, 5, 4096),)} + proposals = propose_indexes( + [usage("status", ColumnRole.EQUALITY)], facts(), existing, min_cost_share=0.01, + ) + assert proposals == [] + + +def test_an_expression_index_is_disclosed_not_silently_ignored(): + """We cannot prove `lower(status)` makes an index on `status` redundant — or that it + doesn't. Saying so beats both suppressing and pretending it isn't there.""" + existing = {"orders": ( + PgIndex("idx_lower_status", (), False, False, 5, 4096, + has_expressions=True, + definition="CREATE INDEX idx_lower_status ON orders (lower(status))"), + )} + proposals = propose_indexes( + [usage("status", ColumnRole.EQUALITY)], facts(), existing, min_cost_share=0.01, + ) + assert codes(proposals) == ["ADV001"] + assert proposals[0].evidence["expression_indexes"] == ("idx_lower_status",) + assert "expression" in proposals[0].rationale.lower() + + +def test_an_expression_index_not_mentioning_the_column_is_not_disclosed(): + """Only expression indexes that plausibly relate to the candidate are worth naming.""" + existing = {"orders": ( + PgIndex("idx_lower_note", (), False, False, 5, 4096, + has_expressions=True, + definition="CREATE INDEX idx_lower_note ON orders (lower(note))"), + )} + proposals = propose_indexes( + [usage("status", ColumnRole.EQUALITY)], facts(), existing, min_cost_share=0.01, + ) + assert proposals[0].evidence["expression_indexes"] == () +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `uv run pytest tests/test_workload_rules.py -k "partial_index_does_not_suppress or expression_index" -v` +Expected: FAIL — the partial index currently suppresses (so `codes(proposals) == []`), and `evidence["expression_indexes"]` raises `KeyError`. + +- [ ] **Step 3: Make coverage refuse to guess** + +Replace `_covered`: + +```python +def _covered(candidate: tuple[str, ...], existing: Sequence[PgIndex]) -> str | None: + """Name of a *plain* existing index whose leading columns already cover ``candidate``. + + Partial and expression indexes are excluded, for opposite reasons that land in the same + place. A partial index does not serve an unfiltered lookup, so calling it coverage + silently withholds a real proposal. An expression index's `columns` tuple understates it + — the expression positions contribute no name — so a prefix match against it is not a + match at all. Neither can be *proven* irrelevant either, which is why `propose_indexes` + discloses them instead of dropping them on the floor. + """ + for index in existing: + if index.is_partial or index.has_expressions: + continue + if _is_prefix(candidate, index.columns): + return index.name + return None +``` + +- [ ] **Step 4: Disclose what was skipped** + +In `propose_indexes`, after `covered_by = _covered(columns, existing.get(table, ()))` and its `continue`, gather the two lists and fold them into the evidence and rationale: + +```python + table_indexes = existing.get(table, ()) + partial_skipped = tuple( + index.name + for index in table_indexes + if index.is_partial and _is_prefix(columns, index.columns) + ) + # Only expression indexes whose definition mentions the leading column are worth + # naming. Proving `lower(status)` equivalent to `status` would need the expression + # parsed and matched; naming it lets the operator make that call in one glance. + expression_indexes = tuple( + index.name + for index in table_indexes + if index.has_expressions and columns[0] in (index.definition or "") + ) +``` + +Add both to the `evidence` dict, and after the existing `rationale` assignment: + +```python + if partial_skipped: + rationale += ( + f" A partial index ({', '.join(partial_skipped)}) leads with these columns " + "but carries a WHERE predicate, so it does not serve an unfiltered lookup — " + "it is not treated as covering this proposal." + ) + if expression_indexes: + rationale += ( + f" An expression index ({', '.join(expression_indexes)}) mentions " + f"{columns[0]}; sqlquality cannot tell whether it already serves this " + "lookup, so confirm before applying." + ) +``` + +- [ ] **Step 5: Run the tests** + +Run: `uv run pytest tests/test_workload_rules.py -q` +Expected: PASS, including the two controls (`test_a_plain_index_still_suppresses_a_candidate`, `test_an_expression_index_not_mentioning_the_column_is_not_disclosed`). + +- [ ] **Step 6: Gates and commit** + +```bash +uv run ruff format . && uv run ruff check . && uv run ruff format --check . && uv run mypy src/sqlquality && uv run pytest -q +git add src/sqlquality/workload/postgres.py tests/test_workload_rules.py +git commit -m "fix(advise): a partial or expression index no longer counts as coverage" +``` + +--- + +### Task 4: ADV003 earns HIGH back, precisely + +**Files:** +- Modify: `src/sqlquality/workload/postgres.py` — `propose_redundant_indexes` +- Test: `tests/test_workload_rules.py` + +**Interfaces:** +- Consumes: `PgIndex.is_partial` / `.has_expressions` / `.predicate` (Task 2). +- Produces: no signature change. `propose_redundant_indexes` now returns HIGH when both indexes in a pair are plain, and skips the pair entirely when either is partial or expression-bearing. + +**Why skip rather than downgrade.** The shipped rule caps every ADV003 at MEDIUM with a blanket caveat, because it could not see predicates. Now it can. For a genuinely plain pair, prefix redundancy is provable from the column lists and HIGH is honest. For a pair involving a partial index, the recommendation is not merely less certain — it is very likely *wrong*, since the partial index exists precisely to serve a subset the wider index serves differently. Emitting it at MEDIUM would still be advising a `DROP INDEX` we have no basis for. + +- [ ] **Step 1: Write the failing test** + +Append to `tests/test_workload_rules.py`: + +```python +def test_a_plain_redundant_pair_is_high_confidence(): + existing = {"orders": ( + PgIndex("idx_narrow", ("status",), False, False, 5, 1), + PgIndex("idx_wide", ("status", "created_at"), False, False, 5, 1), + )} + proposals = propose_redundant_indexes(existing) + assert codes(proposals) == ["ADV003"] + assert proposals[0].confidence is Confidence.HIGH + assert proposals[0].evidence["index"] == "idx_narrow" + + +def test_a_partial_narrow_index_is_never_called_redundant(): + """The partial index exists to serve a subset; the wider full index serves it + differently. Dropping it is not less certain, it is probably wrong.""" + existing = {"orders": ( + PgIndex("idx_open", ("status",), False, False, 5, 1, + is_partial=True, predicate="(shipped_at IS NULL)"), + PgIndex("idx_wide", ("status", "created_at"), False, False, 5, 1), + )} + assert propose_redundant_indexes(existing) == [] + + +def test_a_partial_wider_index_does_not_supersede_a_plain_one(): + existing = {"orders": ( + PgIndex("idx_narrow", ("status",), False, False, 5, 1), + PgIndex("idx_wide_open", ("status", "created_at"), False, False, 5, 1, + is_partial=True, predicate="(shipped_at IS NULL)"), + )} + assert propose_redundant_indexes(existing) == [] + + +def test_an_expression_bearing_pair_is_skipped(): + existing = {"orders": ( + PgIndex("idx_narrow", ("status",), False, False, 5, 1), + PgIndex("idx_expr", ("status",), False, False, 5, 1, has_expressions=True, + definition="CREATE INDEX idx_expr ON orders (status, lower(note))"), + )} + assert propose_redundant_indexes(existing) == [] +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `uv run pytest tests/test_workload_rules.py -k redundant -v` +Expected: FAIL — the plain pair currently returns MEDIUM, and the partial and expression pairs are currently proposed rather than skipped. + +One existing test must be **replaced, not kept**: `test_redundant_prefix_index_proposed_for_drop` at `tests/test_workload_rules.py:381` asserts `Confidence.MEDIUM` with a comment explaining the cap ("PgIndex carries no predicate"). That comment stops being true in Task 2, so delete that test — `test_a_plain_redundant_pair_is_high_confidence` above is its replacement and covers the same case. Keeping both would leave a test asserting the old behaviour. + +- [ ] **Step 3: Implement** + +In `propose_redundant_indexes`, skip any index that is not plain, and restore HIGH: + +```python + for table, indexes in sorted(existing.items()): + for narrow in indexes: + # A partial or expression index is not comparable on column lists alone: the + # predicate or the expression is the whole point of it. Skipping the pair is the + # honest answer, because "probably wrong" is not a confidence level. + if narrow.is_unique or narrow.is_primary or narrow.is_partial: + continue + if narrow.has_expressions: + continue + wider = next( + ( + other + for other in indexes + if other.name != narrow.name + and not other.is_partial + and not other.has_expressions + and len(other.columns) > len(narrow.columns) + and _is_prefix(narrow.columns, other.columns) + ), + None, + ) +``` + +and change the emitted proposal's `confidence` to `Confidence.HIGH`, replacing the blanket caveat with the now-true statement: + +```python + rationale=( + f"Its columns are a leading prefix of {wider.name}, which can serve " + "the same lookups. Both indexes are plain — neither carries a WHERE " + "predicate nor an indexed expression — so the column lists are the " + "whole comparison." + ), +``` + +- [ ] **Step 4: Run the tests** + +Run: `uv run pytest tests/test_workload_rules.py -q` +Expected: PASS. + +- [ ] **Step 5: Confirm the dedup precedence still holds** + +`_dedupe_by_ddl` keeps the strongest confidence when ADV002 and ADV003 target the same `DROP INDEX`, and a `_RULE_PRECEDENCE` tiebreak was added when ADV003 was capped at MEDIUM and the two tied. ADV003 is HIGH again, so it now wins on confidence alone. + +Run: `uv run pytest tests/test_workload_rules.py -k dedupe -v` +Expected: PASS with ADV003 still surviving. If it fails, report it — do not adjust `_RULE_PRECEDENCE` without saying so, since it was added deliberately. + +- [ ] **Step 6: Gates and commit** + +```bash +uv run ruff format . && uv run ruff check . && uv run ruff format --check . && uv run mypy src/sqlquality && uv run pytest -q +git add src/sqlquality/workload/postgres.py tests/test_workload_rules.py +git commit -m "fix(advise): ADV003 is HIGH for plain pairs and silent for the rest" +``` + +--- + +### Task 5: Two recorded trivia + +**Files:** +- Modify: `src/sqlquality/models.py` (`ColumnUsage`), `src/sqlquality/workload/aggregate.py` (`star_tables`) +- Test: `tests/test_models.py`, `tests/test_workload_aggregate.py` + +**Interfaces:** +- Produces: `ColumnUsage.fingerprints` becomes a read-only property returning `len(fingerprint_ids)`; it is no longer a constructor argument. `star_tables` keeps its signature. + +Both were recorded by the final whole-branch review. `fingerprints` and `fingerprint_ids` are redundant and kept in sync only by convention, with no invariant enforcing it — and there are only three `fingerprints=` call sites in the whole repo. `star_tables` compiles a fresh regex per (star-stat × table) pair, which thrashes `re`'s pattern cache on a schema with many tables. + +- [ ] **Step 1: Write the failing test** + +Append to `tests/test_models.py`: + +```python +def test_fingerprints_is_derived_from_the_id_set(): + """One source of truth. The two used to be separate fields kept in step by convention, + with nothing stopping a caller setting one and not the other.""" + usage = ColumnUsage( + table="orders", column="status", role=ColumnRole.EQUALITY, calls=5, + cost_ms=50.0, cost_share=0.5, fingerprint_ids=frozenset({"a", "b"}), + ) + assert usage.fingerprints == 2 + + with pytest.raises(TypeError): + ColumnUsage( # type: ignore[call-arg] + table="orders", column="status", role=ColumnRole.EQUALITY, calls=5, + cost_ms=50.0, cost_share=0.5, fingerprints=2, + ) +``` + +Append to `tests/test_workload_aggregate.py`: + +```python +def test_star_tables_compiles_each_table_pattern_once(monkeypatch): + """A fresh regex per (stat x table) pair thrashes re's pattern cache on a wide schema.""" + import re as _re + + from sqlquality.workload import aggregate as agg + + compiles: list[str] = [] + real_compile = _re.compile + + def counting_compile(pattern, *args, **kwargs): + compiles.append(pattern) + return real_compile(pattern, *args, **kwargs) + + monkeypatch.setattr(agg._re if hasattr(agg, "_re") else _re, "compile", counting_compile) + workload = Workload( + stats=tuple( + QueryStat(fingerprint=f"fp{i}", sql="select * from orders", calls=1, + total_time_ms=1.0, flags=frozenset({FLAG_SELECT_STAR})) + for i in range(5) + ), + window_description="w", + ) + schema = {f"t{i}": {"c": "int"} for i in range(20)} | {"orders": {"c": "int"}} + assert agg.star_tables(workload, schema) == frozenset({"orders"}) + assert len(compiles) <= len(schema), ( + f"compiled {len(compiles)} patterns for {len(schema)} tables across 5 stats" + ) +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `uv run pytest tests/test_models.py -k fingerprints_is_derived tests/test_workload_aggregate.py -k star_tables_compiles -v` +Expected: FAIL — `fingerprints=` is currently accepted so no `TypeError` is raised, and the compile count is 5 × 21. + +- [ ] **Step 3: Implement** + +In `models.py`, delete the `fingerprints: int` field and add a property after the dataclass fields: + +```python + @property + def fingerprints(self) -> int: + """How many query groups contributed this usage. + + Derived rather than stored: it and `fingerprint_ids` were two fields carrying one + fact, kept in step only by convention. + """ + return len(self.fingerprint_ids) +``` + +In `aggregate.py`, drop `fingerprints=...` from the `ColumnUsage(...)` construction and delete the now-unused `fingerprints` counter dict. In `star_tables`, hoist the per-table patterns out of the per-stat loop with a module-level cache: + +```python +@lru_cache(maxsize=4096) +def _identifier_pattern(name: str) -> re.Pattern[str]: + """Compiled whole-identifier matcher for one table name, compiled once per name.""" + return re.compile(rf"\b{re.escape(name)}\b") +``` + +and have both `mentions_table` and `star_tables` use `_identifier_pattern(name).search(sql)`. + +- [ ] **Step 4: Fix the remaining call sites** + +`grep -rn "fingerprints=" src/ tests/` and remove every one — the property is computed. Any test asserting `usage.fingerprints == N` should now build a `fingerprint_ids` set of size N. + +Run: `uv run pytest -q` +Expected: PASS. + +- [ ] **Step 5: Gates and commit** + +```bash +uv run ruff format . && uv run ruff check . && uv run ruff format --check . && uv run mypy src/sqlquality && uv run pytest -q +git add src/sqlquality/models.py src/sqlquality/workload/aggregate.py tests/ +git commit -m "refactor: derive fingerprints from its id set, cache identifier patterns" +``` + +--- + +### Task 6: A real Postgres, behind an opt-in marker + +**Files:** +- Create: `tests/integration/__init__.py`, `tests/integration/conftest.py`, `tests/integration/docker-compose.yml`, `tests/integration/test_introspection_live.py` +- Modify: `pyproject.toml` (register the marker and default deselection), `CONTRIBUTING.md` + +**Interfaces:** +- Produces: a `live_dsn` fixture yielding a DSN string, and a `seeded` fixture yielding `(dsn, schema_name)` against a database with a table, an index, a partial index, an expression index and non-empty `pg_stat_statements`. + +**Why this matters more than its size suggests.** Not one introspection statement in this feature has ever executed against a real server. They are tested for drift — that the text has not changed — which cannot catch a wrong column name, a wrong join, or a view that does not exist on a supported version. The `unnest(...) WITH ORDINALITY` join and Task 2's new `pg_get_expr` call are the two I would least like to be wrong about. + +- [ ] **Step 1: Register the marker so the suite stays green without Docker** + +In `pyproject.toml`, under `[tool.pytest.ini_options]`: + +```toml +markers = [ + "integration: requires a live Postgres (opt in with SQLQUALITY_TEST_DSN or `-m integration`)", +] +addopts = "-m 'not integration'" +``` + +`addopts` is what keeps `uv run pytest` green for a contributor without Docker: the integration tests are deselected, not skipped-with-noise. Running them is `uv run pytest -m integration`. + +- [ ] **Step 2: Write the compose file and the gate** + +`tests/integration/docker-compose.yml`: + +```yaml +# pg_stat_statements must be preloaded at server start; CREATE EXTENSION alone is not +# enough, which is why this is a compose file rather than a plain `services:` block. +services: + postgres: + image: postgres:16 + environment: + POSTGRES_PASSWORD: sqlquality + POSTGRES_DB: sqlquality_test + command: + - postgres + - -c + - shared_preload_libraries=pg_stat_statements + - -c + - pg_stat_statements.track=all + ports: + - "55432:5432" + healthcheck: + test: ["CMD-SHELL", "pg_isready -U postgres -d sqlquality_test"] + interval: 2s + timeout: 3s + retries: 30 +``` + +`tests/integration/__init__.py`: empty. + +`tests/integration/conftest.py`: + +```python +"""Opt-in live-Postgres fixtures. + +Every test in this package is marked `integration` and deselected by default (see +pyproject.toml's addopts), so a contributor without Docker sees a clean `uv run pytest`. + +Bring the server up with: + docker compose -f tests/integration/docker-compose.yml up -d + uv run pytest -m integration +""" + +from __future__ import annotations + +import os + +import pytest + +pytest.importorskip("psycopg", reason="integration tests need the postgres extra") + +DEFAULT_DSN = "postgresql://postgres:sqlquality@127.0.0.1:55432/sqlquality_test" + +pytestmark = pytest.mark.integration + + +@pytest.fixture(scope="session") +def live_dsn() -> str: + """A reachable Postgres, or skip with an actionable message.""" + import psycopg + + dsn = os.environ.get("SQLQUALITY_TEST_DSN", DEFAULT_DSN) + try: + with psycopg.connect(dsn, connect_timeout=3) as conn: + with conn.cursor() as cur: + cur.execute("SELECT 1") + except Exception as exc: # driver-specific; the message is what matters + pytest.skip( + f"no Postgres at {dsn}: {exc}\n" + "start one with: docker compose -f tests/integration/docker-compose.yml up -d" + ) + return dsn + + +@pytest.fixture(scope="session") +def seeded(live_dsn: str) -> tuple[str, str]: + """A schema with the index shapes the catalog query has to survive, plus real workload. + + Deliberately includes a partial and an expression index: those are exactly the rows the + shipped statement discarded, and the only way to know the fix works is to read them back + out of a real catalog. + """ + import psycopg + + schema = "advise_it" + with psycopg.connect(live_dsn, autocommit=True) as conn: + with conn.cursor() as cur: + cur.execute("CREATE EXTENSION IF NOT EXISTS pg_stat_statements") + cur.execute(f"DROP SCHEMA IF EXISTS {schema} CASCADE") + cur.execute(f"CREATE SCHEMA {schema}") + cur.execute( + f"""CREATE TABLE {schema}.orders ( + id bigserial PRIMARY KEY, + status text NOT NULL, + note text, + shipped_at timestamptz, + created_at timestamptz NOT NULL DEFAULT now())""" + ) + cur.execute(f"CREATE INDEX idx_plain ON {schema}.orders (status, created_at)") + cur.execute( + f"CREATE INDEX idx_open ON {schema}.orders (status) " + "WHERE shipped_at IS NULL" + ) + cur.execute(f"CREATE INDEX idx_lower_note ON {schema}.orders (lower(note))") + cur.execute( + f"INSERT INTO {schema}.orders (status, note) " + "SELECT 'paid', 'n' || g FROM generate_series(1, 500) g" + ) + cur.execute("SELECT pg_stat_statements_reset()") + # Real workload for the history statement to find. + for _ in range(3): + cur.execute( + f"SELECT id FROM {schema}.orders WHERE status = %s " + "AND created_at > now() - interval '1 day'", + ("paid",), + ) + cur.fetchall() + return live_dsn, schema +``` + +- [ ] **Step 3: Write the failing test** + +`tests/integration/test_introspection_live.py`: + +```python +"""Execute every introspection statement against a real server. + +The unit suite only checks these statements for drift, which cannot catch a wrong column +name, a wrong join, or a view that does not exist. This is the only place they run. +""" + +from __future__ import annotations + +import pytest + +from sqlquality.models import ConnectionParams +from sqlquality.workload.postgres import ( + CAP_INDEXES, + CAP_NDV, + CAP_SCHEMA, + CAP_STATS_RESET, + CAP_TABLE_FACTS, + CAP_WORKLOAD, + PostgresWorkloadAdapter, +) + + +@pytest.fixture +def adapter(seeded: tuple[str, str]) -> PostgresWorkloadAdapter: + dsn, schema = seeded + a = PostgresWorkloadAdapter() + a.schemas = (schema,) + a.connect(ConnectionParams(engine="postgres", dsn=dsn, fields={}, source="--dsn"), 30) + return a + + +def test_every_introspection_statement_executes(adapter, seeded): + """No statement may raise, and none may report a degraded capability.""" + _dsn, schema = seeded + adapter.fetch_workload(None, 500) + adapter.fetch_schema((schema,)) + adapter.fetch_table_facts((schema,), frozenset({"orders"})) + adapter.fetch_indexes((schema,), frozenset({"orders"})) + assert adapter.degraded == [], f"a statement failed against a real server: {adapter.degraded}" + + +def test_workload_statement_returns_our_own_queries(adapter): + fetch = adapter.fetch_workload(None, 500) + assert fetch.rows, "pg_stat_statements returned nothing" + assert "since stats reset at" in fetch.window_description + + +def test_table_facts_reports_a_real_row_estimate_and_ndv(adapter, seeded): + _dsn, schema = seeded + facts = adapter.fetch_table_facts((schema,), frozenset({"orders"}))["orders"] + assert facts.row_estimate is not None and facts.row_estimate > 0 + assert "status" in facts.columns + assert facts.ndv, "pg_stats returned no distinct-value estimates" + + +def test_indexes_statement_reads_partial_and_expression_metadata(adapter, seeded): + """The reason Task 2 exists, verified against a real catalog rather than a fixture.""" + _dsn, schema = seeded + by_name = {i.name: i for i in adapter.fetch_indexes((schema,), frozenset({"orders"}))["orders"]} + + assert by_name["idx_plain"].columns == ("status", "created_at") + assert by_name["idx_plain"].is_partial is False + assert by_name["idx_plain"].has_expressions is False + + assert by_name["idx_open"].is_partial is True + assert "shipped_at IS NULL" in (by_name["idx_open"].predicate or "") + + # The row the shipped statement silently dropped. + assert by_name["idx_lower_note"].has_expressions is True + assert "lower(note)" in (by_name["idx_lower_note"].definition or "") + + assert by_name["orders_pkey"].is_primary is True + + +def test_the_session_really_is_read_only(adapter, seeded): + """Invariant 2, against a real server: the session must refuse a write.""" + import psycopg + + _dsn, schema = seeded + with pytest.raises(psycopg.errors.ReadOnlySqlTransaction): + adapter._query(f"CREATE TABLE {schema}.should_not_exist (x int)", ()) +``` + +- [ ] **Step 4: Run it** + +```bash +docker compose -f tests/integration/docker-compose.yml up -d +uv run pytest -m integration -v +``` + +Expected: all pass. If `test_indexes_statement_reads_partial_and_expression_metadata` fails, Task 2's SQL is wrong in a way no fixture could reveal — that is precisely what this task exists to find, so report the actual failure rather than adjusting the assertion. + +Then confirm the default suite is unaffected: + +```bash +docker compose -f tests/integration/docker-compose.yml down +uv run pytest -q +``` + +Expected: the same count as before this task, no skips, no errors. + +- [ ] **Step 5: Document it** + +Add to `CONTRIBUTING.md` after the four-checks section: + +```markdown +## Integration tests (optional) + +`advise`'s introspection SQL is only checked for drift by the default suite. To run it +against a real Postgres: + +```bash +docker compose -f tests/integration/docker-compose.yml up -d +uv run pytest -m integration +docker compose -f tests/integration/docker-compose.yml down +``` + +These are deselected by default, so `uv run pytest` stays green without Docker. Point them +at your own server with `SQLQUALITY_TEST_DSN`. They need the `postgres` extra +(`uv sync --extra postgres`). +``` + +- [ ] **Step 6: Gates and commit** + +```bash +uv run ruff format . && uv run ruff check . && uv run ruff format --check . && uv run mypy src/sqlquality && uv run pytest -q +git add tests/integration pyproject.toml CONTRIBUTING.md +git commit -m "test(advise): execute the introspection SQL against a real postgres" +``` + +--- + +### Task 7: An end-to-end run, and narrower README limitations + +**Files:** +- Create: `tests/integration/test_advise_live.py` +- Modify: `README.md` (the expression-index and ADV003 limitations) + +**Interfaces:** +- Consumes: the `seeded` fixture (Task 6); the `advise` CLI. + +- [ ] **Step 1: Write the failing test** + +`tests/integration/test_advise_live.py`: + +```python +"""One whole `advise` run against a real database. + +Every other test stubs the querier. This is the only path that exercises resolve_connection +-> connect -> six statements -> ingest -> aggregate -> propose -> render as one piece. +""" + +from __future__ import annotations + +import json + +import pytest +from typer.testing import CliRunner + +from sqlquality.cli import app + +pytestmark = pytest.mark.integration +runner = CliRunner() + + +def test_advise_end_to_end(seeded, tmp_path): + dsn, schema = seeded + md = tmp_path / "report.md" + ddl = tmp_path / "proposals.sql" + result = runner.invoke( + app, + ["advise", "--dsn", dsn, "--schema", schema, "--json", + "--markdown", str(md), "--ddl", str(ddl)], + ) + assert result.exit_code == 0, result.output + payload = json.loads(result.stdout) + + assert payload["engine"] == "postgres" + assert payload["redacted"] is True + assert payload["analyzed"]["query_groups"] > 0 + assert payload["degraded"] == [] + assert md.read_text(encoding="utf-8").startswith("# sqlquality advise") + assert "REVIEW BEFORE RUNNING" in ddl.read_text(encoding="utf-8") + + +def test_advise_does_not_leak_a_literal_from_a_real_server(seeded, tmp_path): + """The redaction guarantee, against real pg_stat_statements rather than a fixture. + + The seeded workload filters on the literal 'paid'. pg_stat_statements normalises it to + $1, but a run with --keep-literals proves the surfaces would carry it if we let them. + """ + dsn, schema = seeded + md = tmp_path / "report.md" + result = runner.invoke( + app, ["advise", "--dsn", dsn, "--schema", schema, "--json", "--markdown", str(md)] + ) + assert result.exit_code == 0, result.output + assert "'paid'" not in result.stdout + assert "'paid'" not in md.read_text(encoding="utf-8") + + +def test_advise_dry_run_needs_no_server(tmp_path): + """The audit path must not depend on anything being reachable.""" + result = runner.invoke(app, ["advise", "--engine", "postgres", "--dry-run"]) + assert result.exit_code == 0 + assert "pg_stat_statements" in result.stdout +``` + +- [ ] **Step 2: Run it** + +```bash +docker compose -f tests/integration/docker-compose.yml up -d +uv run pytest -m integration -v +``` + +Expected: all pass. `payload["degraded"] == []` is the sharp one — it asserts every capability succeeded against a real server with a superuser role. + +- [ ] **Step 3: Narrow the README's limitations** + +Two limitations were written when the catalog query could not see this metadata. Replace the expression-index bullet with: + +```markdown +- **Expression indexes are read but not matched.** `advise` now sees that an index on + `lower(status)` exists and names it in the proposal's evidence, but it cannot tell whether + that index already serves a lookup on `status` — so it proposes and says so, rather than + suppressing or ignoring. Confirm before applying. +``` + +and the ADV003 bullet with: + +```markdown +- **ADV003 only compares plain indexes.** A pair where either index carries a `WHERE` + predicate or an indexed expression is skipped entirely rather than proposed at lower + confidence: a partial index exists to serve a subset, so recommending its removal is + likely wrong rather than merely uncertain. Plain pairs are reported at HIGH. +``` + +Also add: + +```markdown +- **A partial index does not suppress a proposal.** `idx ON orders(status) WHERE + shipped_at IS NULL` does not serve `WHERE status = $1`, so it is not treated as covering + a candidate index — it is named in the evidence instead. +``` + +- [ ] **Step 4: Gates and commit** + +```bash +uv run ruff format . && uv run ruff check . && uv run ruff format --check . && uv run mypy src/sqlquality && uv run pytest -q +git add tests/integration/test_advise_live.py README.md +git commit -m "test(advise): end-to-end run against a real postgres; narrow two limitations" +``` + +--- + +## Self-Review + +**Coverage of the recorded items.** Every Batch-1 item from the ledger maps to a task: `secrets.py` extraction → Task 1; expression-index blindness → Tasks 2 and 3; ADV003 partial-predicate blindness → Task 4; `fingerprints` redundancy and `star_tables` regex churn → Task 5; integration test → Tasks 6 and 7; the two README limitations that those fixes narrow → Task 7. + +Deliberately **not** in this plan, and staying in the ledger for Batch 2: join-key and grouping-column proposals; `DECLARE`/`COPY` unwrapping; multi-schema `(schema, table)` keying. Each changes what `advise` *says*, not whether what it says is trustworthy, so they belong after this. + +**Type consistency.** `PgIndex` gains `is_partial`, `predicate`, `has_expressions`, `definition` in Task 2 and every later task reads exactly those names. `_covered` keeps `(candidate, existing) -> str | None` throughout. `clamp_timeout_ms` takes keyword-only `minimum`/`maximum` in Task 1 and is called that way in the same task. `ColumnUsage.fingerprints` stops being a constructor argument in Task 5, and Task 5's Step 4 sweeps the call sites. + +**Known risks for the implementer.** +1. Task 2's twelve-column unpack is order-sensitive and a transposed pair would be invisible to a fixture test that uses the same wrong order. Task 6's live test is the real check — if the two disagree, the live one is right. +2. `pg_get_expr(ix.indpred, ix.indrelid)` returns the predicate with Postgres's own parenthesisation, so assert with `in` rather than equality on anything but a fixture. +3. Task 5's `TypeError` assertion depends on `@dataclass(frozen=True)` rejecting unknown keywords, which it does — but if `ColumnUsage` ever gains `**kwargs` handling the test silently stops discriminating. From 25a3635128159dd8638c2d1f5df54c0e8310d9c6 Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Mon, 27 Jul 2026 12:17:53 +0200 Subject: [PATCH 03/27] refactor(workload): move credential handling into its own module Credential scrubbing and timeout clamping move from postgres.py into a new engine-neutral sqlquality.workload.secrets module (SECRET_FIELDS, MIN_SCRUBBABLE_SECRET, WITHHELD, secrets_for, scrub, clamp_timeout_ms), all now public, so future Redshift/Snowflake adapters cannot bypass the hard-won scrubbing sequence. clamp_timeout_ms takes explicit minimum/maximum keyword args instead of reading module constants. --- src/sqlquality/workload/postgres.py | 77 +++------------------------- src/sqlquality/workload/secrets.py | 79 +++++++++++++++++++++++++++++ tests/test_advise_cli.py | 4 +- tests/test_workload_postgres.py | 56 -------------------- tests/test_workload_secrets.py | 57 +++++++++++++++++++++ 5 files changed, 145 insertions(+), 128 deletions(-) create mode 100644 src/sqlquality/workload/secrets.py create mode 100644 tests/test_workload_secrets.py diff --git a/src/sqlquality/workload/postgres.py b/src/sqlquality/workload/postgres.py index 8b7ab58..238db33 100644 --- a/src/sqlquality/workload/postgres.py +++ b/src/sqlquality/workload/postgres.py @@ -4,10 +4,9 @@ import hashlib import sys -from collections.abc import Iterable, Mapping, Sequence +from collections.abc import Mapping, Sequence from dataclasses import dataclass, field from datetime import timedelta -from urllib.parse import unquote, urlparse from sqlquality.models import ( Aggregation, @@ -31,6 +30,7 @@ WorkloadAdapter, ) from sqlquality.workload.fingerprint import FLAG_LEADING_WILDCARD_LIKE, FLAG_SELECT_STAR +from sqlquality.workload.secrets import clamp_timeout_ms, scrub, secrets_for CAP_WORKLOAD = "workload" CAP_STATS_RESET = "stats_reset" @@ -80,8 +80,6 @@ _PG_PASSTHROUGH_FIELDS = frozenset( {"sslmode", "sslcert", "sslkey", "sslrootcert", "connect_timeout"} ) -#: profiles.yml keys whose values must never appear in any message we emit. -_SECRET_FIELDS = frozenset({"password", "pass"}) def _pg_fields(fields: dict[str, str]) -> dict[str, str]: @@ -102,69 +100,6 @@ def _dropped_pg_fields(fields: dict[str, str]) -> tuple[str, ...]: ) -def _clamp_timeout_ms(timeout_s: int) -> int: - """Statement timeout in milliseconds, clamped into a sane range. - - The CLI rejects an out-of-range value before reaching here; this is the safety net - for any other caller. Bounds come from workload.base so the two cannot drift. - """ - return max(MIN_TIMEOUT_S, min(int(timeout_s), MAX_TIMEOUT_S)) * 1000 - - -#: A secret shorter than this cannot be redacted by substring replacement without -#: destroying the message — a one-character password would blank every occurrence of that -#: letter. When one actually appears, the driver's text is withheld rather than mangled. -_MIN_SCRUBBABLE_SECRET = 4 -_WITHHELD = "(driver message withheld: it contained a value too short to redact safely)" - - -def _secrets_for(params: ConnectionParams) -> tuple[str, ...]: - """Every value we know to be secret for this connection. - - A DSN is added *and* its password extracted separately. The whole-DSN token only helps - if the driver echoes the connection string back verbatim, which real libpq errors do - not do — they report the offending value on its own. Without the extracted password, - DSN-based connections would have no effective protection at all. - - The password is added in **both** its percent-encoded and decoded forms. - ``urlparse().password`` returns it still encoded, but libpq decodes a URI DSN before - authenticating, so the value a real auth-failure message carries is the decoded one: - for ``postgresql://u:p%40ss@h/db`` the driver reports ``p@ss`` while urlparse yields - ``p%40ss``, and a token of only the encoded form never matches. Any password containing - ``@``, ``:``, ``/``, ``%`` or a space hits this. The encoded form is kept too, since a - URI-parse error can echo the raw string back instead. - """ - secrets = tuple( - value for key, value in params.fields.items() if key in _SECRET_FIELDS and value - ) - if params.dsn: - secrets += (params.dsn,) - encoded = urlparse(params.dsn).password - if encoded: - secrets += (encoded,) - decoded = unquote(encoded) - if decoded != encoded: - secrets += (decoded,) - return secrets - - -def _scrub(text: str, secrets: Iterable[str]) -> str: - """Replace any known secret occurring in ``text`` with a redaction marker. - - Defence in depth for driver exceptions. libpq is not believed to echo a password, but - the auth-failure path — the most common real connect failure — cannot be exercised - without a live server, and we hold the secret anyway, so its absence can be guaranteed - instead of trusted. - """ - present = [secret for secret in secrets if secret and secret in text] - if any(len(secret) < _MIN_SCRUBBABLE_SECRET for secret in present): - return _WITHHELD - scrubbed = text - for secret in present: - scrubbed = scrubbed.replace(secret, "***") - return scrubbed - - #: Characters of hex kept from the fingerprint digest. 12 is 48 bits — ample for telling #: apart the few hundred query groups one run reads, and short enough to sit in a table cell. _FINGERPRINT_ID_LEN = 12 @@ -857,7 +792,7 @@ def connect(self, params: ConnectionParams, timeout_s: int) -> None: # Everything we know to be secret, so a driver exception can be proven clean rather # than trusted. - secrets = _secrets_for(params) + secrets = secrets_for(params) failure: str | None = None try: @@ -874,10 +809,12 @@ def connect(self, params: ConnectionParams, timeout_s: int) -> None: # is the wrong habit to establish in the one place we talk to a database. cursor.execute( "SELECT set_config('statement_timeout', %s, false)", - (f"{_clamp_timeout_ms(timeout_s)}ms",), + ( + f"{clamp_timeout_ms(timeout_s, minimum=MIN_TIMEOUT_S, maximum=MAX_TIMEOUT_S)}ms", + ), ) except Exception as exc: - failure = _scrub(str(exc), secrets) + failure = scrub(str(exc), secrets) if failure is not None: # Raised after the handler, and scrubbed: Task 6 established that a dependency's # exception text is exactly where this class of leak hides, and that leaving the diff --git a/src/sqlquality/workload/secrets.py b/src/sqlquality/workload/secrets.py new file mode 100644 index 0000000..2cf2054 --- /dev/null +++ b/src/sqlquality/workload/secrets.py @@ -0,0 +1,79 @@ +"""Credential handling shared by every workload adapter. + +This lives outside any one adapter deliberately. Scrubbing took three fix rounds to get +right on the Postgres adapter — the driver's exception text quoted the offending value, then +`from None` turned out to suppress only the traceback while leaving `__context__` reachable, +then a percent-encoded DSN password slipped past because ``urlparse`` returns it still +encoded. An adapter that cannot see these helpers will re-derive that sequence badly. +""" + +from __future__ import annotations + +from collections.abc import Iterable +from urllib.parse import unquote, urlparse + +from sqlquality.models import ConnectionParams + +#: profiles.yml keys whose values must never appear in any message we emit. +SECRET_FIELDS = frozenset({"password", "pass"}) + +#: A secret shorter than this cannot be redacted by substring replacement without +#: destroying the message — a one-character password would blank every occurrence of that +#: letter. When one actually appears, the driver's text is withheld rather than mangled. +MIN_SCRUBBABLE_SECRET = 4 +WITHHELD = "(driver message withheld: it contained a value too short to redact safely)" + + +def secrets_for(params: ConnectionParams) -> tuple[str, ...]: + """Every value we know to be secret for this connection. + + A DSN is added *and* its password extracted separately. The whole-DSN token only helps + if the driver echoes the connection string back verbatim, which real libpq errors do + not do — they report the offending value on its own. Without the extracted password, + DSN-based connections would have no effective protection at all. + + The password is added in **both** its percent-encoded and decoded forms. + ``urlparse().password`` returns it still encoded, but libpq decodes a URI DSN before + authenticating, so the value a real auth-failure message carries is the decoded one: + for ``postgresql://u:p%40ss@h/db`` the driver reports ``p@ss`` while urlparse yields + ``p%40ss``, and a token of only the encoded form never matches. Any password containing + ``@``, ``:``, ``/``, ``%`` or a space hits this. The encoded form is kept too, since a + URI-parse error can echo the raw string back instead. + """ + secrets = tuple(value for key, value in params.fields.items() if key in SECRET_FIELDS and value) + if params.dsn: + secrets += (params.dsn,) + encoded = urlparse(params.dsn).password + if encoded: + secrets += (encoded,) + decoded = unquote(encoded) + if decoded != encoded: + secrets += (decoded,) + return secrets + + +def scrub(text: str, secrets: Iterable[str]) -> str: + """Replace any known secret occurring in ``text`` with a redaction marker. + + Defence in depth for driver exceptions. libpq is not believed to echo a password, but + the auth-failure path — the most common real connect failure — cannot be exercised + without a live server, and we hold the secret anyway, so its absence can be guaranteed + instead of trusted. + """ + present = [secret for secret in secrets if secret and secret in text] + if any(len(secret) < MIN_SCRUBBABLE_SECRET for secret in present): + return WITHHELD + scrubbed = text + for secret in present: + scrubbed = scrubbed.replace(secret, "***") + return scrubbed + + +def clamp_timeout_ms(timeout_s: int, *, minimum: int, maximum: int) -> int: + """Statement timeout in milliseconds, clamped into ``[minimum, maximum]`` seconds. + + Bounds are parameters rather than module constants: the CLI owns the user-facing range + and rejects out-of-range input, so a second copy of the numbers here could drift out of + step with the message the user was shown. + """ + return max(minimum, min(int(timeout_s), maximum)) * 1000 diff --git a/tests/test_advise_cli.py b/tests/test_advise_cli.py index 3923c29..2ee1a2d 100644 --- a/tests/test_advise_cli.py +++ b/tests/test_advise_cli.py @@ -348,8 +348,8 @@ def explode(conninfo, **kwargs): # test_workload_postgres.py::test_connect_scrubs_a_password_from_a_driver_failure. # A fixed message would make the "hunter2 not in output" assertion below unable to # fail: it would be asserting the absence of a string nothing ever produced. - # Measured: with `_scrub` replaced by the identity function, that assertion now - # fails, and with a fixed message it did not. + # Measured: with `scrub` (sqlquality.workload.secrets) replaced by the identity + # function, that assertion now fails, and with a fixed message it did not. raise RuntimeError(f"connection failed for conninfo {conninfo}") fake_psycopg.connect = explode # type: ignore[attr-defined] diff --git a/tests/test_workload_postgres.py b/tests/test_workload_postgres.py index 1453123..05fe30e 100644 --- a/tests/test_workload_postgres.py +++ b/tests/test_workload_postgres.py @@ -13,9 +13,6 @@ CAP_TABLE_FACTS, CAP_WORKLOAD, PostgresWorkloadAdapter, - _scrub, - _secrets_for, - _WITHHELD, ) EXPECTED_CAPABILITIES = { @@ -285,59 +282,6 @@ def explode(conninfo, **kwargs): assert exc.value.__context__ is None -def test_secrets_for_extracts_the_password_from_an_inline_dsn(): - """The realistic leak shape: a driver reports the bad password on its own. - - It never echoes the whole connection string back, so a whole-DSN token alone would - never match and DSN connections would have no protection. - """ - params = ConnectionParams( - engine="postgres", - dsn="postgresql://u:hunter2@db:5432/analytics", - fields={}, - source="--dsn", - ) - secrets = _secrets_for(params) - assert "hunter2" in secrets - realistic = 'connection failed: password authentication failed for user "u" (hunter2)' - assert "hunter2" not in _scrub(realistic, secrets) - - -def test_secrets_for_covers_a_percent_encoded_dsn_password(): - """urlparse leaves the password encoded; libpq decodes it before authenticating. - - So the value a real auth-failure message carries is the *decoded* one, and a token of - only the encoded form never matches. Any password containing @ : / % or a space hits - this, which is most passwords a generator would produce. - """ - params = ConnectionParams( - engine="postgres", dsn="postgresql://u:p%40ss@h/db", fields={}, source="--dsn" - ) - secrets = _secrets_for(params) - assert "p%40ss" in secrets - assert "p@ss" in secrets - driver_message = 'connection failed: password authentication failed for user "u" (p@ss)' - assert "p@ss" not in _scrub(driver_message, secrets) - - -def test_secrets_for_tolerates_a_dsn_with_no_password_or_a_malformed_one(): - for dsn in ("postgresql://u@h/db", "not a valid dsn :: at all ///"): - params = ConnectionParams(engine="postgres", dsn=dsn, fields={}, source="--dsn") - assert _secrets_for(params) == (dsn,) - - -def test_scrub_withholds_rather_than_mangles_an_unredactable_secret(): - """A one-character password would blank every occurrence of that letter. - - Nothing leaks either way, but a message redacted into unreadability is worse than an - honest refusal to show it. - """ - mangled = _scrub("a database has an admin at a table", ("a",)) - assert mangled == _WITHHELD - # A short secret that does not actually appear must not suppress a usable message. - assert _scrub("connection refused", ("a",)) == "connection refused" - - def test_fetch_indexes_groups_columns_in_ordinal_order(): querier = FakeQuerier( { diff --git a/tests/test_workload_secrets.py b/tests/test_workload_secrets.py new file mode 100644 index 0000000..617ff97 --- /dev/null +++ b/tests/test_workload_secrets.py @@ -0,0 +1,57 @@ +from __future__ import annotations + +import pytest + +from sqlquality.models import ConnectionParams +from sqlquality.workload.secrets import ( + MIN_SCRUBBABLE_SECRET, + SECRET_FIELDS, + WITHHELD, + clamp_timeout_ms, + scrub, + secrets_for, +) + + +def _params(**kwargs) -> ConnectionParams: + base = {"engine": "postgres", "dsn": None, "fields": {}, "source": "--dsn"} + base.update(kwargs) + return ConnectionParams(**base) # type: ignore[arg-type] + + +def test_secrets_for_collects_password_fields(): + assert secrets_for(_params(fields={"host": "db", "password": "hunter2"})) == ("hunter2",) + + +def test_secrets_for_covers_both_forms_of_a_dsn_password(): + """urlparse leaves the password encoded; libpq decodes it before authenticating.""" + got = secrets_for(_params(dsn="postgresql://u:p%40ss@h/db")) + assert "p%40ss" in got + assert "p@ss" in got + + +def test_secrets_for_tolerates_a_dsn_with_no_password_or_a_malformed_one(): + for dsn in ("postgresql://u@h/db", "not a valid dsn :: at all ///"): + assert secrets_for(_params(dsn=dsn)) == (dsn,) + + +def test_scrub_redacts_a_present_secret(): + assert scrub('failed for user "u" (hunter2)', ("hunter2",)) == 'failed for user "u" (***)' + + +def test_scrub_withholds_rather_than_mangles_an_unredactable_secret(): + assert scrub("a database has an admin", ("a",)) == WITHHELD + assert scrub("connection refused", ("a",)) == "connection refused" + + +def test_min_scrubbable_secret_is_the_documented_floor(): + assert MIN_SCRUBBABLE_SECRET == 4 + assert "password" in SECRET_FIELDS + + +@pytest.mark.parametrize( + ("given", "expected_ms"), + [(0, 1_000), (-5, 1_000), (30, 30_000), (99_999, 3_600_000)], +) +def test_clamp_timeout_ms_bounds_and_converts(given, expected_ms): + assert clamp_timeout_ms(given, minimum=1, maximum=3600) == expected_ms From f33088b0c0145a78922243d2f1bca621df3c6631 Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Mon, 27 Jul 2026 12:18:33 +0200 Subject: [PATCH 04/27] docs: pass the shared timeout bounds by name, not as literals My Task 1 example hardcoded minimum=1, maximum=3600 at the call site. That would have broken test_the_timeout_bounds_have_a_single_definition -- which asserts 3600 never appears in postgres.py's source -- and reintroduced the duplicated-constants defect the brief's own rationale argues against. MIN_TIMEOUT_S/MAX_TIMEOUT_S already live in workload/base.py. Caught by the Task 1 implementer, which used the constants rather than following the example. Co-Authored-By: Claude Opus 5 --- .../plans/2026-07-27-advise-postgres-hardening.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md b/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md index ae071cf..3772f0b 100644 --- a/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md +++ b/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md @@ -235,7 +235,10 @@ Update the two call sites in `connect()`: `secrets = secrets_for(params)` and `s ```python cursor.execute( "SELECT set_config('statement_timeout', %s, false)", - (f"{clamp_timeout_ms(timeout_s, minimum=1, maximum=3600)}ms",), + ( + f"{clamp_timeout_ms(timeout_s, minimum=MIN_TIMEOUT_S, " + f"maximum=MAX_TIMEOUT_S)}ms", + ), ) ``` @@ -1239,7 +1242,7 @@ git commit -m "test(advise): end-to-end run against a real postgres; narrow two Deliberately **not** in this plan, and staying in the ledger for Batch 2: join-key and grouping-column proposals; `DECLARE`/`COPY` unwrapping; multi-schema `(schema, table)` keying. Each changes what `advise` *says*, not whether what it says is trustworthy, so they belong after this. -**Type consistency.** `PgIndex` gains `is_partial`, `predicate`, `has_expressions`, `definition` in Task 2 and every later task reads exactly those names. `_covered` keeps `(candidate, existing) -> str | None` throughout. `clamp_timeout_ms` takes keyword-only `minimum`/`maximum` in Task 1 and is called that way in the same task. `ColumnUsage.fingerprints` stops being a constructor argument in Task 5, and Task 5's Step 4 sweeps the call sites. +**Type consistency.** `PgIndex` gains `is_partial`, `predicate`, `has_expressions`, `definition` in Task 2 and every later task reads exactly those names. `_covered` keeps `(candidate, existing) -> str | None` throughout. `clamp_timeout_ms` takes keyword-only `minimum`/`maximum` in Task 1 and is called with `MIN_TIMEOUT_S`/`MAX_TIMEOUT_S`, which already live in `workload/base.py` and are imported by both the CLI and the adapter. Passing literals there would break `test_the_timeout_bounds_have_a_single_definition`, which asserts `3600` never appears in `postgres.py`'s source — the guard added when the duplicated bounds were first found. `ColumnUsage.fingerprints` stops being a constructor argument in Task 5, and Task 5's Step 4 sweeps the call sites. **Known risks for the implementer.** 1. Task 2's twelve-column unpack is order-sensitive and a transposed pair would be invisible to a fixture test that uses the same wrong order. Task 6's live test is the real check — if the two disagree, the live one is right. From 17e85b99ecf3f11844cd74e3f037cc43680dc70e Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Mon, 27 Jul 2026 12:27:32 +0200 Subject: [PATCH 05/27] fix(advise): stop discarding expression-index columns from the catalog --- src/sqlquality/workload/postgres.py | 64 ++++++++-- tests/test_workload_postgres.py | 176 +++++++++++++++++++++++++++- 2 files changed, 226 insertions(+), 14 deletions(-) diff --git a/src/sqlquality/workload/postgres.py b/src/sqlquality/workload/postgres.py index 238db33..c9f3bf4 100644 --- a/src/sqlquality/workload/postgres.py +++ b/src/sqlquality/workload/postgres.py @@ -147,6 +147,10 @@ class _IndexRows: is_primary: bool scans: int size_bytes: int + is_partial: bool = False + predicate: str | None = None + has_expressions: bool = False + definition: str | None = None #: (ordinality, column) so the column order can be restored by sorting. columns: list[tuple[int, str]] = field(default_factory=list) @@ -161,6 +165,16 @@ class PgIndex: is_primary: bool scans: int size_bytes: int + #: True when the index has a WHERE predicate. A partial index does not serve an + #: unfiltered lookup, so it can never be assumed to cover a proposed index. + is_partial: bool = False + #: The rendered predicate, for showing an operator why a drop was not recommended. + predicate: str | None = None + #: True when any indexed position is an expression rather than a plain column. Such a + #: position contributes no name to `columns`, so the tuple understates the index. + has_expressions: bool = False + #: The full CREATE INDEX text, the only place an expression is legible. + definition: str | None = None #: Below this row estimate a sequential scan is the right plan; an index is pure overhead. @@ -722,21 +736,29 @@ class PostgresWorkloadAdapter(WorkloadAdapter): FROM pg_stats s WHERE s.schemaname = ANY(%s) AND s.tablename = ANY(%s) """, - # Known limitation: the pg_attribute join silently omits expression indexes. - # `indkey` holds 0 for an expression column, which matches no pg_attribute row, so - # an index on `lower(status)` is invisible here. Consequence: ADV001 may propose an - # index whose expression equivalent already exists. Reading pg_get_indexdef() would - # fix it; deferred rather than silently ignored. + # LEFT JOIN, not JOIN: Postgres stores 0 in indkey for an expression column and no + # pg_attribute row has attnum 0, so an inner join silently discarded every expression + # index's columns — they arrived with an empty tuple. The NULL attname a LEFT JOIN + # yields is what tells us the position was an expression. + # + # indpred / indexprs are selected as booleans plus the rendered predicate, because a + # partial index does not serve an unfiltered lookup and an expression index does not + # serve its bare column — both of which the coverage and redundancy rules previously + # had to guess at. CAP_INDEXES: """ SELECT t.relname, i.relname, a.attname, k.ordinality, ix.indisunique, ix.indisprimary, - COALESCE(psui.idx_scan, 0), pg_relation_size(i.oid) + COALESCE(psui.idx_scan, 0), pg_relation_size(i.oid), + ix.indpred IS NOT NULL, + pg_get_expr(ix.indpred, ix.indrelid), + ix.indexprs IS NOT NULL, + pg_get_indexdef(ix.indexrelid) FROM pg_index ix JOIN pg_class i ON i.oid = ix.indexrelid JOIN pg_class t ON t.oid = ix.indrelid JOIN pg_namespace n ON n.oid = t.relnamespace JOIN LATERAL unnest(ix.indkey) WITH ORDINALITY AS k(attnum, ordinality) ON TRUE - JOIN pg_attribute a ON a.attrelid = t.oid AND a.attnum = k.attnum + LEFT JOIN pg_attribute a ON a.attrelid = t.oid AND a.attnum = k.attnum LEFT JOIN pg_stat_user_indexes psui ON psui.indexrelid = i.oid WHERE n.nspname = ANY(%s) AND t.relname = ANY(%s) ORDER BY t.relname, i.relname, k.ordinality @@ -915,7 +937,20 @@ def fetch_indexes( """Existing indexes per table, columns in ordinal order.""" grouped: dict[tuple[str, str], _IndexRows] = {} for row in self._run(CAP_INDEXES, (list(schemas), sorted(tables))): - table, index, column, ordinality, unique, primary, scans, size = row + ( + table, + index, + column, + ordinality, + unique, + primary, + scans, + size, + is_partial, + predicate, + has_expressions, + definition, + ) = row entry = grouped.setdefault( (str(table), str(index)), _IndexRows( @@ -923,13 +958,20 @@ def fetch_indexes( is_primary=bool(primary), scans=_as_int(scans), size_bytes=_as_int(size) if size is not None else 0, + is_partial=bool(is_partial), + predicate=str(predicate) if predicate is not None else None, + has_expressions=bool(has_expressions), + definition=str(definition) if definition is not None else None, ), ) + # A NULL attname is an expression position: it has no column name to record, and + # `has_expressions` already marks the index, so skip it rather than storing "None". # Keyed by ordinality and sorted below rather than trusting arrival order. The # statement does ORDER BY k.ordinality, but composite-index column order decides # whether a proposal is right, and a fixture test that pre-sorts its canned rows # cannot notice the difference. Cheap defence in depth. - entry.columns.append((_as_int(ordinality), str(column))) + if column is not None: + entry.columns.append((_as_int(ordinality), str(column))) result: dict[str, list[PgIndex]] = {} for (table, index), entry in grouped.items(): @@ -941,6 +983,10 @@ def fetch_indexes( is_primary=entry.is_primary, scans=entry.scans, size_bytes=entry.size_bytes, + is_partial=entry.is_partial, + predicate=entry.predicate, + has_expressions=entry.has_expressions, + definition=entry.definition, ) ) return {table: tuple(indexes) for table, indexes in result.items()} diff --git a/tests/test_workload_postgres.py b/tests/test_workload_postgres.py index 05fe30e..782ae86 100644 --- a/tests/test_workload_postgres.py +++ b/tests/test_workload_postgres.py @@ -236,8 +236,34 @@ def test_fetch_indexes_restores_column_order_from_ordinality(): querier = FakeQuerier( { "pg_index": [ - ("orders", "idx_status_created", "created_at", 2, False, False, 0, 8192), - ("orders", "idx_status_created", "status", 1, False, False, 0, 8192), + ( + "orders", + "idx_status_created", + "created_at", + 2, + False, + False, + 0, + 8192, + False, + None, + False, + "CREATE INDEX idx_status_created ON orders (status, created_at)", + ), + ( + "orders", + "idx_status_created", + "status", + 1, + False, + False, + 0, + 8192, + False, + None, + False, + "CREATE INDEX idx_status_created ON orders (status, created_at)", + ), ] } ) @@ -286,9 +312,48 @@ def test_fetch_indexes_groups_columns_in_ordinal_order(): querier = FakeQuerier( { "pg_index": [ - ("orders", "orders_pkey", "id", 1, True, True, 900, 4096), - ("orders", "idx_status_created", "status", 1, False, False, 0, 8192), - ("orders", "idx_status_created", "created_at", 2, False, False, 0, 8192), + ( + "orders", + "orders_pkey", + "id", + 1, + True, + True, + 900, + 4096, + False, + None, + False, + "CREATE UNIQUE INDEX orders_pkey ON orders (id)", + ), + ( + "orders", + "idx_status_created", + "status", + 1, + False, + False, + 0, + 8192, + False, + None, + False, + "CREATE INDEX idx_status_created ON orders (status, created_at)", + ), + ( + "orders", + "idx_status_created", + "created_at", + 2, + False, + False, + 0, + 8192, + False, + None, + False, + "CREATE INDEX idx_status_created ON orders (status, created_at)", + ), ] } ) @@ -580,3 +645,104 @@ def test_the_timeout_bounds_have_a_single_definition(): assert str(base.MAX_TIMEOUT_S) not in source, ( f"{module.__name__} restates the --timeout ceiling as a literal" ) + + +def test_fetch_indexes_records_an_expression_index_rather_than_dropping_it(): + """`indkey` holds 0 for an expression column and no pg_attribute row has attnum 0. + + The old inner join therefore discarded those rows, so an index on `lower(status)` + arrived with an empty column tuple and could not be reasoned about at all. + """ + querier = FakeQuerier( + { + "pg_index": [ + # attname is NULL for the expression column, as a LEFT JOIN yields. + ( + "orders", + "idx_lower_status", + None, + 1, + False, + False, + 3, + 8192, + False, + None, + True, + "CREATE INDEX idx_lower_status ON orders (lower(status))", + ), + ] + } + ) + indexes = PostgresWorkloadAdapter(querier=querier).fetch_indexes( + ("public",), frozenset({"orders"}) + ) + index = indexes["orders"][0] + assert index.has_expressions is True + assert index.columns == () + assert "lower(status)" in (index.definition or "") + + +def test_fetch_indexes_records_a_partial_index_predicate(): + querier = FakeQuerier( + { + "pg_index": [ + ( + "orders", + "idx_open", + "status", + 1, + False, + False, + 7, + 4096, + True, + "(shipped_at IS NULL)", + False, + "CREATE INDEX idx_open ON orders (status) WHERE shipped_at IS NULL", + ), + ] + } + ) + index = PostgresWorkloadAdapter(querier=querier).fetch_indexes( + ("public",), frozenset({"orders"}) + )["orders"][0] + assert index.is_partial is True + assert index.predicate == "(shipped_at IS NULL)" + assert index.columns == ("status",) + + +def test_fetch_indexes_leaves_a_plain_index_unmarked(): + querier = FakeQuerier( + { + "pg_index": [ + ( + "orders", + "idx_status", + "status", + 1, + False, + False, + 12, + 4096, + False, + None, + False, + "CREATE INDEX idx_status ON orders (status)", + ), + ] + } + ) + index = PostgresWorkloadAdapter(querier=querier).fetch_indexes( + ("public",), frozenset({"orders"}) + )["orders"][0] + assert (index.is_partial, index.predicate, index.has_expressions) == (False, None, False) + + +def test_the_indexes_statement_reads_predicate_and_expression_metadata(): + sql = PostgresWorkloadAdapter().SQL[CAP_INDEXES].lower() + assert "indpred" in sql, "the partial-index predicate must be selected" + assert "indexprs" in sql, "expression presence must be selected" + assert "left join pg_attribute" in sql, ( + "an inner join drops expression columns, whose indkey entry is 0" + ) From f7cb805718ab7a1b92d503e5e4a594c6352b1020 Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Mon, 27 Jul 2026 12:36:28 +0200 Subject: [PATCH 06/27] fix(advise): a partial or expression index no longer counts as coverage --- src/sqlquality/workload/postgres.py | 41 +++++++++++++- tests/test_workload_rules.py | 84 +++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 1 deletion(-) diff --git a/src/sqlquality/workload/postgres.py b/src/sqlquality/workload/postgres.py index c9f3bf4..4e7ffa6 100644 --- a/src/sqlquality/workload/postgres.py +++ b/src/sqlquality/workload/postgres.py @@ -213,8 +213,18 @@ def _is_prefix(shorter: tuple[str, ...], longer: tuple[str, ...]) -> bool: def _covered(candidate: tuple[str, ...], existing: Sequence[PgIndex]) -> str | None: - """Name of an existing index whose leading columns already cover ``candidate``.""" + """Name of a *plain* existing index whose leading columns already cover ``candidate``. + + Partial and expression indexes are excluded, for opposite reasons that land in the same + place. A partial index does not serve an unfiltered lookup, so calling it coverage + silently withholds a real proposal. An expression index's `columns` tuple understates it + — the expression positions contribute no name — so a prefix match against it is not a + match at all. Neither can be *proven* irrelevant either, which is why `propose_indexes` + discloses them instead of dropping them on the floor. + """ for index in existing: + if index.is_partial or index.has_expressions: + continue if _is_prefix(candidate, index.columns): return index.name return None @@ -305,6 +315,21 @@ def propose_indexes( if covered_by is not None: continue + table_indexes = existing.get(table, ()) + partial_skipped = tuple( + index.name + for index in table_indexes + if index.is_partial and _is_prefix(columns, index.columns) + ) + # Only expression indexes whose definition mentions the leading column are worth + # naming. Proving `lower(status)` equivalent to `status` would need the expression + # parsed and matched; naming it lets the operator make that call in one glance. + expression_indexes = tuple( + index.name + for index in table_indexes + if index.has_expressions and columns[0] in (index.definition or "") + ) + ndv = table_facts.ndv if table_facts else {} leading_ndv = ndv.get(columns[0]) if rows is None or not have_index_data: @@ -337,6 +362,18 @@ def propose_indexes( ) if rows is None: rationale += _UNKNOWN_ROWS_NOTE + if partial_skipped: + rationale += ( + f" A partial index ({', '.join(partial_skipped)}) leads with these columns " + "but carries a WHERE predicate, so it does not serve an unfiltered lookup — " + "it is not treated as covering this proposal." + ) + if expression_indexes: + rationale += ( + f" An expression index ({', '.join(expression_indexes)}) mentions " + f"{columns[0]}; sqlquality cannot tell whether it already serves this " + "lookup, so confirm before applying." + ) proposals.append( Proposal( @@ -352,6 +389,8 @@ def propose_indexes( "fingerprints": max(i.fingerprints for i in chosen), "row_estimate": rows, "leading_ndv": leading_ndv, + "partial_indexes_skipped": partial_skipped, + "expression_indexes": expression_indexes, }, confidence=confidence, ddl=( diff --git a/tests/test_workload_rules.py b/tests/test_workload_rules.py index c28959f..de65f5e 100644 --- a/tests/test_workload_rules.py +++ b/tests/test_workload_rules.py @@ -158,6 +158,90 @@ def test_a_narrower_existing_index_does_not_cover_a_wider_candidate(): assert proposals[0].evidence["columns"] == ("status", "created_at") +def test_a_partial_index_does_not_suppress_a_candidate(): + """`idx ON orders(status) WHERE shipped_at IS NULL` does not serve `WHERE status = $1`. + + Treating it as coverage silently withheld a good proposal — the inverse of the + confidently-wrong failures, and just as invisible. + """ + existing = { + "orders": ( + PgIndex( + "idx_open", + ("status",), + False, + False, + 5, + 4096, + is_partial=True, + predicate="(shipped_at IS NULL)", + ), + ) + } + proposals = propose_indexes( + [usage("status", ColumnRole.EQUALITY)], facts(), existing, min_cost_share=0.01 + ) + assert codes(proposals) == ["ADV001"] + assert proposals[0].evidence["partial_indexes_skipped"] == ("idx_open",) + assert "partial" in proposals[0].rationale.lower() + + +def test_a_plain_index_still_suppresses_a_candidate(): + """The control. Task 2's new fields default to False, so this must not have changed.""" + existing = {"orders": (PgIndex("idx_status", ("status",), False, False, 5, 4096),)} + proposals = propose_indexes( + [usage("status", ColumnRole.EQUALITY)], facts(), existing, min_cost_share=0.01 + ) + assert proposals == [] + + +def test_an_expression_index_is_disclosed_not_silently_ignored(): + """We cannot prove `lower(status)` makes an index on `status` redundant — or that it + doesn't. Saying so beats both suppressing and pretending it isn't there.""" + existing = { + "orders": ( + PgIndex( + "idx_lower_status", + (), + False, + False, + 5, + 4096, + has_expressions=True, + definition="CREATE INDEX idx_lower_status ON orders (lower(status))", + ), + ) + } + proposals = propose_indexes( + [usage("status", ColumnRole.EQUALITY)], facts(), existing, min_cost_share=0.01 + ) + assert codes(proposals) == ["ADV001"] + assert proposals[0].evidence["expression_indexes"] == ("idx_lower_status",) + assert "expression" in proposals[0].rationale.lower() + + +def test_an_expression_index_not_mentioning_the_column_is_not_disclosed(): + """Only expression indexes that plausibly relate to the candidate are worth naming.""" + existing = { + "orders": ( + PgIndex( + "idx_lower_note", + (), + False, + False, + 5, + 4096, + has_expressions=True, + definition="CREATE INDEX idx_lower_note ON orders (lower(note))", + ), + ) + } + proposals = propose_indexes( + [usage("status", ColumnRole.EQUALITY)], facts(), existing, min_cost_share=0.01 + ) + assert proposals[0].evidence["expression_indexes"] == () + + def test_arity_cap_keeps_the_range_column_last_when_it_bites(): """The interaction of the two most important ordering rules, previously untested. From b899714cb672a3e3ca406789be73a5c3a540ecf1 Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Mon, 27 Jul 2026 12:41:55 +0200 Subject: [PATCH 07/27] docs: match whole identifiers when disclosing an expression index Task 3's review confirmed the substring test states a falsehood to the operator. Reproduced: a candidate on `id` against an index on `lower(guid)` yields the rationale "An expression index (idx_lower_guid) mentions id" -- it does not; "gu-id-" does. That sentence sits in the .sql file someone reads while deciding whether to run DDL, which is exactly where the tool must not assert what it cannot support. It fires for any short column name inside a longer identifier, so id/guid, id/valid, at/created_at are all live. The reviewer verified word boundaries cost no true positive: lower(status), lower(customer_id::text) and (id::text) all still match, because Postgres separates identifiers with parens, commas, dots and ::, none of them word characters. Rather than add a third hand-rolled \b regex -- aggregate.mentions_table and the test suite's _write_verbs_in already exist for this exact class of bug -- the plan now generalises the existing helper to mentions_identifier and has mentions_table delegate to it. Widens Task 3's scope to aggregate.py by two functions, which is cheaper than the duplication. Adds the false-positive case and a cast-form control as tests, plus the rationale-absence assertion the reviewer noted was missing. Co-Authored-By: Claude Opus 5 --- .../2026-07-27-advise-postgres-hardening.md | 73 ++++++++++++++++++- 1 file changed, 72 insertions(+), 1 deletion(-) diff --git a/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md b/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md index 3772f0b..d92203d 100644 --- a/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md +++ b/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md @@ -512,6 +512,38 @@ def test_an_expression_index_not_mentioning_the_column_is_not_disclosed(): [usage("status", ColumnRole.EQUALITY)], facts(), existing, min_cost_share=0.01, ) assert proposals[0].evidence["expression_indexes"] == () + assert "expression index" not in proposals[0].rationale.lower() + + +def test_a_column_name_inside_a_longer_identifier_is_not_disclosed(): + """`id` is a substring of `guid`, and a substring test said so out loud. + + The rationale would have told the operator an index "mentions id" when it indexes + `lower(guid)` — a false claim in the text someone reads before running DDL. + """ + existing = {"orders": ( + PgIndex("idx_lower_guid", (), False, False, 5, 4096, + has_expressions=True, + definition="CREATE INDEX idx_lower_guid ON orders (lower(guid))"), + )} + proposals = propose_indexes( + [usage("id", ColumnRole.EQUALITY)], + facts(columns=("id", "guid")), existing, min_cost_share=0.01, + ) + assert proposals[0].evidence["expression_indexes"] == () + + +def test_an_expression_index_on_a_cast_of_the_column_is_still_disclosed(): + """The control for the fix: word boundaries must not cost a true positive.""" + existing = {"orders": ( + PgIndex("idx_status_cast", (), False, False, 5, 4096, + has_expressions=True, + definition="CREATE INDEX idx_status_cast ON orders ((status::text))"), + )} + proposals = propose_indexes( + [usage("status", ColumnRole.EQUALITY)], facts(), existing, min_cost_share=0.01, + ) + assert proposals[0].evidence["expression_indexes"] == ("idx_status_cast",) ``` - [ ] **Step 2: Run test to verify it fails** @@ -519,6 +551,37 @@ def test_an_expression_index_not_mentioning_the_column_is_not_disclosed(): Run: `uv run pytest tests/test_workload_rules.py -k "partial_index_does_not_suppress or expression_index" -v` Expected: FAIL — the partial index currently suppresses (so `codes(proposals) == []`), and `evidence["expression_indexes"]` raises `KeyError`. +- [ ] **Step 2b: Generalise the existing word-boundary helper instead of writing a third** + +`src/sqlquality/workload/aggregate.py` already has `mentions_table(name, sql)`, whose whole +purpose is whole-identifier matching, and the test suite has `_write_verbs_in` doing the same +for a different vocabulary. Adding a third copy in `postgres.py` would be the duplication the +final review of the previous plan specifically called out. Rename the mechanism and keep the +caller: + +```python +def mentions_identifier(name: str, text: str) -> bool: + """True if ``name`` appears in ``text`` as a whole identifier, not merely a substring. + + A plain `name in text` test false-positives on any name that is a substring of a longer + identifier — `order` inside `orders`, `cart` inside `shopping_cart`, `id` inside `guid` — + and on a name appearing only in an alias like `orders_total`. `\b` already treats `_` as + a word character in Python's `re`, so it rejects all of those without a custom class, + while still matching across the punctuation Postgres puts around identifiers: parens, + commas, dots and `::` are all non-word characters. + """ + return re.search(rf"\b{re.escape(name)}\b", text) is not None + + +def mentions_table(name: str, sql: str) -> bool: + """True if a query mentions this table. See :func:`mentions_identifier`.""" + return mentions_identifier(name, sql) +``` + +`postgres.py` then imports `mentions_identifier` from `sqlquality.workload.aggregate` +alongside whatever it already imports from there. This widens the task's file scope to +`aggregate.py` by two functions, deliberately: a third hand-rolled `\b` regex is worse. + - [ ] **Step 3: Make coverage refuse to guess** Replace `_covered`: @@ -556,10 +619,18 @@ In `propose_indexes`, after `covered_by = _covered(columns, existing.get(table, # Only expression indexes whose definition mentions the leading column are worth # naming. Proving `lower(status)` equivalent to `status` would need the expression # parsed and matched; naming it lets the operator make that call in one glance. + # + # Whole-identifier matching, not a substring test. `columns[0] in definition` reports + # a candidate on `id` against an index on `lower(guid)`, and the rationale then tells + # the operator an index "mentions id" when it does not — a claim the tool cannot + # support, in the string someone reads while deciding whether to run DDL. Verified + # that `\b` keeps the true positives: `lower(status)`, `lower(customer_id::text)` + # and `(id::text)` all still match, because Postgres separates identifiers with + # parens, commas, dots and `::`, none of which are word characters. expression_indexes = tuple( index.name for index in table_indexes - if index.has_expressions and columns[0] in (index.definition or "") + if index.has_expressions and mentions_identifier(columns[0], index.definition or "") ) ``` From 0b8ed66016ff1d412575e1784846addab5e00450 Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Mon, 27 Jul 2026 12:43:57 +0200 Subject: [PATCH 08/27] fix(advise): match expression indexes on whole identifiers, not substrings Generalises aggregate.mentions_table into mentions_identifier and reuses it in postgres.py's expression-index disclosure, instead of a third hand-rolled \b regex. A plain substring check reported an index on lower(guid) as mentioning the id column, which is a false statement in the .sql rationale an operator reads before running DDL. --- src/sqlquality/workload/aggregate.py | 17 ++++++--- src/sqlquality/workload/postgres.py | 12 +++++-- tests/test_workload_rules.py | 52 ++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 7 deletions(-) diff --git a/src/sqlquality/workload/aggregate.py b/src/sqlquality/workload/aggregate.py index 1854f0b..fe20786 100644 --- a/src/sqlquality/workload/aggregate.py +++ b/src/sqlquality/workload/aggregate.py @@ -13,15 +13,22 @@ _Key = tuple[str, str, ColumnRole] -def mentions_table(name: str, sql: str) -> bool: - """True if ``name`` appears in ``sql`` as a whole identifier, not merely a substring. +def mentions_identifier(name: str, text: str) -> bool: + """True if ``name`` appears in ``text`` as a whole identifier, not merely a substring. - A plain `name in sql` test would false-positive three ways: a table `order` inside a + A plain `name in text` test would false-positive three ways: a table `order` inside a query on `orders`, a table `cart` inside `shopping_cart`, and a table `orders` that only appears as part of a column alias like `orders_total`. `\\b` already treats `_` as a word - character in Python's `re`, so it rejects all three without a custom boundary class. + character in Python's `re`, so it rejects all three without a custom boundary class, + while still matching across the punctuation SQL puts around identifiers: parens, commas, + dots and `::` are all non-word characters. """ - return re.search(rf"\b{re.escape(name)}\b", sql) is not None + return re.search(rf"\b{re.escape(name)}\b", text) is not None + + +def mentions_table(name: str, sql: str) -> bool: + """True if a query mentions this table. See :func:`mentions_identifier`.""" + return mentions_identifier(name, sql) def star_tables(workload: Workload, schema: dict) -> frozenset[str]: diff --git a/src/sqlquality/workload/postgres.py b/src/sqlquality/workload/postgres.py index 4e7ffa6..a3fbae6 100644 --- a/src/sqlquality/workload/postgres.py +++ b/src/sqlquality/workload/postgres.py @@ -21,7 +21,7 @@ WorkloadFetch, cost_share_of, ) -from sqlquality.workload.aggregate import mentions_table +from sqlquality.workload.aggregate import mentions_identifier, mentions_table from sqlquality.workload.base import ( MAX_TIMEOUT_S, MIN_TIMEOUT_S, @@ -324,10 +324,18 @@ def propose_indexes( # Only expression indexes whose definition mentions the leading column are worth # naming. Proving `lower(status)` equivalent to `status` would need the expression # parsed and matched; naming it lets the operator make that call in one glance. + # + # Whole-identifier matching, not a substring test. `columns[0] in definition` reports + # a candidate on `id` against an index on `lower(guid)`, and the rationale then tells + # the operator an index "mentions id" when it does not — a claim the tool cannot + # support, in the string someone reads while deciding whether to run DDL. Verified + # that `\b` keeps the true positives: `lower(status)`, `lower(customer_id::text)` + # and `(id::text)` all still match, because Postgres separates identifiers with + # parens, commas, dots and `::`, none of which are word characters. expression_indexes = tuple( index.name for index in table_indexes - if index.has_expressions and columns[0] in (index.definition or "") + if index.has_expressions and mentions_identifier(columns[0], index.definition or "") ) ndv = table_facts.ndv if table_facts else {} diff --git a/tests/test_workload_rules.py b/tests/test_workload_rules.py index de65f5e..a836e94 100644 --- a/tests/test_workload_rules.py +++ b/tests/test_workload_rules.py @@ -240,6 +240,58 @@ def test_an_expression_index_not_mentioning_the_column_is_not_disclosed(): [usage("status", ColumnRole.EQUALITY)], facts(), existing, min_cost_share=0.01 ) assert proposals[0].evidence["expression_indexes"] == () + assert "expression index" not in proposals[0].rationale.lower() + + +def test_a_column_name_inside_a_longer_identifier_is_not_disclosed(): + """`id` is a substring of `guid`, and a substring test said so out loud. + + The rationale would have told the operator an index "mentions id" when it indexes + `lower(guid)` — a false claim in the text someone reads before running DDL. + """ + existing = { + "orders": ( + PgIndex( + "idx_lower_guid", + (), + False, + False, + 5, + 4096, + has_expressions=True, + definition="CREATE INDEX idx_lower_guid ON orders (lower(guid))", + ), + ) + } + proposals = propose_indexes( + [usage("id", ColumnRole.EQUALITY)], + facts(columns=("id", "guid")), + existing, + min_cost_share=0.01, + ) + assert proposals[0].evidence["expression_indexes"] == () + + +def test_an_expression_index_on_a_cast_of_the_column_is_still_disclosed(): + """The control for the fix: word boundaries must not cost a true positive.""" + existing = { + "orders": ( + PgIndex( + "idx_status_cast", + (), + False, + False, + 5, + 4096, + has_expressions=True, + definition="CREATE INDEX idx_status_cast ON orders ((status::text))", + ), + ) + } + proposals = propose_indexes( + [usage("status", ColumnRole.EQUALITY)], facts(), existing, min_cost_share=0.01 + ) + assert proposals[0].evidence["expression_indexes"] == ("idx_status_cast",) def test_arity_cap_keeps_the_range_column_last_when_it_bites(): From 27d09ea3315d4c04869062c66e2351756e47da16 Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Mon, 27 Jul 2026 12:50:27 +0200 Subject: [PATCH 09/27] fix(advise): ADV003 is HIGH for plain pairs and silent for the rest --- src/sqlquality/workload/postgres.py | 34 ++++++++------ tests/test_workload_rules.py | 69 ++++++++++++++++++++++------- 2 files changed, 75 insertions(+), 28 deletions(-) diff --git a/src/sqlquality/workload/postgres.py b/src/sqlquality/workload/postgres.py index a3fbae6..7fdcd02 100644 --- a/src/sqlquality/workload/postgres.py +++ b/src/sqlquality/workload/postgres.py @@ -455,22 +455,32 @@ def propose_redundant_indexes( ) -> list[Proposal]: """ADV003 — an index whose column list is a strict prefix of another's is redundant. - Capped at MEDIUM, not HIGH. ``PgIndex`` carries no ``indpred``/``indexprs``, so a - partial index (``WHERE shipped_at IS NULL``) and an expression index are both - indistinguishable from plain ones here — and for a partial index "serves the same - lookups" is simply false. The README says so, but a README does not travel inside the - ``.sql`` file the operator runs, so the rationale carries the caveat too. + HIGH when both indexes are plain: prefix redundancy is then provable from the column + lists alone. A partial index (``WHERE shipped_at IS NULL``) or an expression index is + skipped entirely rather than downgraded, on either side of the pair — ``PgIndex`` now + carries ``is_partial``/``has_expressions``/``predicate`` (see Task 2), and for a + partial index "serves the same lookups" is simply false, since the partial index + exists precisely to serve a subset the wider index serves differently. Emitting that + at MEDIUM would still be advising a `DROP INDEX` with no basis: "probably wrong" is + not a confidence level. """ proposals: list[Proposal] = [] for table, indexes in sorted(existing.items()): for narrow in indexes: - if narrow.is_unique or narrow.is_primary: + # A partial or expression index is not comparable on column lists alone: the + # predicate or the expression is the whole point of it. Skipping the pair is + # the honest answer, because "probably wrong" is not a confidence level. + if narrow.is_unique or narrow.is_primary or narrow.is_partial: + continue + if narrow.has_expressions: continue wider = next( ( other for other in indexes if other.name != narrow.name + and not other.is_partial + and not other.has_expressions and len(other.columns) > len(narrow.columns) and _is_prefix(narrow.columns, other.columns) ), @@ -483,12 +493,10 @@ def propose_redundant_indexes( code="ADV003", title=f"Drop redundant index {narrow.name} on {table}", rationale=( - f"Its columns are a leading prefix of {wider.name}, which can " - "serve the same lookups. This comparison is on column lists only: " - "sqlquality cannot see a partial index's WHERE predicate or an " - "expression index's expressions, and a partial index does not " - "cover the same rows as a wider full one. Confirm that neither " - "index is partial or expression-based before dropping." + f"Its columns are a leading prefix of {wider.name}, which can serve " + "the same lookups. Both indexes are plain — neither carries a WHERE " + "predicate nor an indexed expression — so the column lists are the " + "whole comparison." ), evidence={ "table": table, @@ -498,7 +506,7 @@ def propose_redundant_indexes( "superseding_columns": wider.columns, "size_bytes": narrow.size_bytes, }, - confidence=Confidence.MEDIUM, + confidence=Confidence.HIGH, ddl=f"DROP INDEX {_qualified(schema, narrow.name)};", ) ) diff --git a/tests/test_workload_rules.py b/tests/test_workload_rules.py index a836e94..eec432d 100644 --- a/tests/test_workload_rules.py +++ b/tests/test_workload_rules.py @@ -514,7 +514,7 @@ def test_unused_index_rule_ignores_tables_outside_the_workload(): assert propose_unused_indexes(existing, hot_tables=frozenset({"orders"})) == [] -def test_redundant_prefix_index_proposed_for_drop(): +def test_a_plain_redundant_pair_is_high_confidence(): existing = { "orders": ( PgIndex("idx_narrow", ("status",), False, False, 5, 1), @@ -523,28 +523,67 @@ def test_redundant_prefix_index_proposed_for_drop(): } proposals = propose_redundant_indexes(existing) assert codes(proposals) == ["ADV003"] + assert proposals[0].confidence is Confidence.HIGH assert proposals[0].evidence["index"] == "idx_narrow" - # Capped at MEDIUM: PgIndex carries no predicate, so a partial index is - # indistinguishable from a full one here. See the test below. - assert proposals[0].confidence is Confidence.MEDIUM -def test_redundant_index_rationale_admits_it_cannot_see_a_partial_predicate(): - """HIGH is the strongest claim the tool makes, and "serves the same lookups" is false - for a partial or expression index. The README limitation does not travel inside the - .sql file the operator actually runs, so the caveat has to be in the rationale. - """ +def test_a_partial_narrow_index_is_never_called_redundant(): + """The partial index exists to serve a subset; the wider full index serves it + differently. Dropping it is not less certain, it is probably wrong.""" existing = { "orders": ( - PgIndex("idx_narrow", ("status",), False, False, 5, 1), + PgIndex( + "idx_open", + ("status",), + False, + False, + 5, + 1, + is_partial=True, + predicate="(shipped_at IS NULL)", + ), PgIndex("idx_wide", ("status", "created_at"), False, False, 5, 1), ) } - proposals = propose_redundant_indexes(existing) - rationale = proposals[0].rationale.lower() - assert "column list" in rationale - assert "partial" in rationale - assert "expression" in rationale + assert propose_redundant_indexes(existing) == [] + + +def test_a_partial_wider_index_does_not_supersede_a_plain_one(): + existing = { + "orders": ( + PgIndex("idx_narrow", ("status",), False, False, 5, 1), + PgIndex( + "idx_wide_open", + ("status", "created_at"), + False, + False, + 5, + 1, + is_partial=True, + predicate="(shipped_at IS NULL)", + ), + ) + } + assert propose_redundant_indexes(existing) == [] + + +def test_an_expression_bearing_pair_is_skipped(): + existing = { + "orders": ( + PgIndex("idx_narrow", ("status",), False, False, 5, 1), + PgIndex( + "idx_expr", + ("status",), + False, + False, + 5, + 1, + has_expressions=True, + definition="CREATE INDEX idx_expr ON orders (status, lower(note))", + ), + ) + } + assert propose_redundant_indexes(existing) == [] def test_a_unique_prefix_index_is_never_called_redundant(): From c57579fac91b459d32c742a332098280904f2088 Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Mon, 27 Jul 2026 12:56:21 +0200 Subject: [PATCH 10/27] docs: give Task 4's expression tests teeth and pin the ADV003 rationale Task 4's review found the committed expression test passes for the wrong reason. Both indexes had one column, so `len(other.columns) > len(narrow.columns)` was already False from the length tie -- the test returns [] whether or not the has_expressions filters exist at all. Ninth test on this project that looked like a guarantee and wasn't; the implementer spotted it and reported it but shipped it unfixed. Replaced with two that discriminate: a strictly wider expression index, and the narrow-side case. The second matters for a reason worth stating -- `columns` understates an expression index, so a narrow one may index something the wider one does not, and dropping it on a column-list comparison would discard an index nothing else provides. Also pins the rationale wording. Deleting the old MEDIUM test (correctly, since its "cannot see a partial predicate" hedge became false) removed the only assertion on this rationale's content, so a future edit could reintroduce a hedge or drop the "both are plain" claim while leaving confidence at HIGH with nothing failing. Verified both assertions hold against the shipped text rather than assuming. Co-Authored-By: Claude Opus 5 --- .../2026-07-27-advise-postgres-hardening.md | 32 +++++++++++++++++-- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md b/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md index d92203d..754db19 100644 --- a/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md +++ b/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md @@ -692,6 +692,11 @@ def test_a_plain_redundant_pair_is_high_confidence(): assert codes(proposals) == ["ADV003"] assert proposals[0].confidence is Confidence.HIGH assert proposals[0].evidence["index"] == "idx_narrow" + # Pin the claim, not just the confidence. Deleting the old MEDIUM test removed the only + # assertion on this rationale's wording, so a future edit could reintroduce a hedge, or + # drop the "both are plain" claim while leaving HIGH, with nothing failing. + assert "plain" in proposals[0].rationale + assert "partial" not in proposals[0].rationale def test_a_partial_narrow_index_is_never_called_redundant(): @@ -714,11 +719,32 @@ def test_a_partial_wider_index_does_not_supersede_a_plain_one(): assert propose_redundant_indexes(existing) == [] -def test_an_expression_bearing_pair_is_skipped(): +def test_a_wider_expression_index_does_not_supersede_a_plain_one(): + """The wider index must be strictly wider, or the length guard skips the pair anyway. + + An earlier version of this test gave both indexes one column, so + `len(other.columns) > len(narrow.columns)` was already False and it passed whether or + not the has_expressions filter existed at all. + """ existing = {"orders": ( PgIndex("idx_narrow", ("status",), False, False, 5, 1), - PgIndex("idx_expr", ("status",), False, False, 5, 1, has_expressions=True, - definition="CREATE INDEX idx_expr ON orders (status, lower(note))"), + PgIndex("idx_expr", ("status", "note"), False, False, 5, 1, has_expressions=True, + definition="CREATE INDEX idx_expr ON orders (status, note, lower(note))"), + )} + assert propose_redundant_indexes(existing) == [] + + +def test_a_narrow_expression_index_is_never_called_redundant(): + """The other direction, and the reason it matters. + + `columns` understates an expression index — the expression positions contribute no + name — so a narrow one may index something the wider one does not. Dropping it on a + column-list comparison would discard an index nothing else provides. + """ + existing = {"orders": ( + PgIndex("idx_narrow_expr", ("status",), False, False, 5, 1, has_expressions=True, + definition="CREATE INDEX idx_narrow_expr ON orders (status, lower(note))"), + PgIndex("idx_wide", ("status", "created_at"), False, False, 5, 1), )} assert propose_redundant_indexes(existing) == [] ``` From 4380bdbdf0a1e4d9ce5a24eda7bf68c7a202057a Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Mon, 27 Jul 2026 12:58:33 +0200 Subject: [PATCH 11/27] fix(advise): discriminate ADV003's expression-index tests, pin the HIGH rationale MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The old expression test gave both indexes equal-length columns, so the length guard already returned [] whether or not the has_expressions filters existed. Replaced with a strictly-wider expression index (proves the wider-side guard) and a narrow expression index (proves the narrow-side guard, and the reason it matters: columns understates an expression index, so a narrow one may cover something the wider one doesn't). Also pinned the HIGH rationale's wording ("plain" present, "partial" absent) now that the old MEDIUM test — the only thing asserting on it — is gone, and dropped an internal task-number reference from the docstring. --- src/sqlquality/workload/postgres.py | 9 +++---- tests/test_workload_rules.py | 40 +++++++++++++++++++++++++++-- 2 files changed, 42 insertions(+), 7 deletions(-) diff --git a/src/sqlquality/workload/postgres.py b/src/sqlquality/workload/postgres.py index 7fdcd02..bd5414a 100644 --- a/src/sqlquality/workload/postgres.py +++ b/src/sqlquality/workload/postgres.py @@ -458,11 +458,10 @@ def propose_redundant_indexes( HIGH when both indexes are plain: prefix redundancy is then provable from the column lists alone. A partial index (``WHERE shipped_at IS NULL``) or an expression index is skipped entirely rather than downgraded, on either side of the pair — ``PgIndex`` now - carries ``is_partial``/``has_expressions``/``predicate`` (see Task 2), and for a - partial index "serves the same lookups" is simply false, since the partial index - exists precisely to serve a subset the wider index serves differently. Emitting that - at MEDIUM would still be advising a `DROP INDEX` with no basis: "probably wrong" is - not a confidence level. + carries ``is_partial``/``has_expressions``/``predicate``, and for a partial index + "serves the same lookups" is simply false, since the partial index exists precisely to + serve a subset the wider index serves differently. Emitting that at MEDIUM would still + be advising a `DROP INDEX` with no basis: "probably wrong" is not a confidence level. """ proposals: list[Proposal] = [] for table, indexes in sorted(existing.items()): diff --git a/tests/test_workload_rules.py b/tests/test_workload_rules.py index eec432d..5186a19 100644 --- a/tests/test_workload_rules.py +++ b/tests/test_workload_rules.py @@ -525,6 +525,11 @@ def test_a_plain_redundant_pair_is_high_confidence(): assert codes(proposals) == ["ADV003"] assert proposals[0].confidence is Confidence.HIGH assert proposals[0].evidence["index"] == "idx_narrow" + # Pin the claim, not just the confidence. Deleting the old MEDIUM test removed the only + # assertion on this rationale's wording, so a future edit could reintroduce a hedge, or + # drop the "both are plain" claim while leaving HIGH, with nothing failing. + assert "plain" in proposals[0].rationale + assert "partial" not in proposals[0].rationale def test_a_partial_narrow_index_is_never_called_redundant(): @@ -567,20 +572,51 @@ def test_a_partial_wider_index_does_not_supersede_a_plain_one(): assert propose_redundant_indexes(existing) == [] -def test_an_expression_bearing_pair_is_skipped(): +def test_a_wider_expression_index_does_not_supersede_a_plain_one(): + """The wider index must be strictly wider, or the length guard skips the pair anyway. + + An earlier version of this test gave both indexes one column, so + `len(other.columns) > len(narrow.columns)` was already False and it passed whether or + not the has_expressions filter existed at all. + """ existing = { "orders": ( PgIndex("idx_narrow", ("status",), False, False, 5, 1), PgIndex( "idx_expr", + ("status", "note"), + False, + False, + 5, + 1, + has_expressions=True, + definition="CREATE INDEX idx_expr ON orders (status, note, lower(note))", + ), + ) + } + assert propose_redundant_indexes(existing) == [] + + +def test_a_narrow_expression_index_is_never_called_redundant(): + """The other direction, and the reason it matters. + + `columns` understates an expression index — the expression positions contribute no + name — so a narrow one may index something the wider one does not. Dropping it on a + column-list comparison would discard an index nothing else provides. + """ + existing = { + "orders": ( + PgIndex( + "idx_narrow_expr", ("status",), False, False, 5, 1, has_expressions=True, - definition="CREATE INDEX idx_expr ON orders (status, lower(note))", + definition="CREATE INDEX idx_narrow_expr ON orders (status, lower(note))", ), + PgIndex("idx_wide", ("status", "created_at"), False, False, 5, 1), ) } assert propose_redundant_indexes(existing) == [] From 465e4bff1bd92d8c48f0237860ed0e6db06d47c5 Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Mon, 27 Jul 2026 13:06:50 +0200 Subject: [PATCH 12/27] refactor: derive fingerprints from its id set, cache identifier patterns --- src/sqlquality/models.py | 10 +++++++- src/sqlquality/workload/aggregate.py | 18 ++++++++++---- tests/test_models.py | 28 +++++++++++++++++++++- tests/test_workload_aggregate.py | 35 ++++++++++++++++++++++++++++ tests/test_workload_rules.py | 1 - 5 files changed, 85 insertions(+), 7 deletions(-) diff --git a/src/sqlquality/models.py b/src/sqlquality/models.py index da99bd3..5a69a47 100644 --- a/src/sqlquality/models.py +++ b/src/sqlquality/models.py @@ -132,12 +132,20 @@ class ColumnUsage: #: unqualifiable, so poor schema coverage dilutes every surviving share rather than #: silently inflating it. Read it alongside the report's skipped counts. cost_share: float - fingerprints: int #: Fingerprints of the query groups that contributed this usage. Needed to ask whether #: two usages *co-occur* — a partial-index proposal is only supported if some single #: query actually filters on the indexed column and the guard column together. fingerprint_ids: frozenset[str] = frozenset() + @property + def fingerprints(self) -> int: + """How many query groups contributed this usage. + + Derived rather than stored: it and `fingerprint_ids` were two fields carrying one + fact, kept in step only by convention. + """ + return len(self.fingerprint_ids) + @dataclass(frozen=True) class Aggregation: diff --git a/src/sqlquality/workload/aggregate.py b/src/sqlquality/workload/aggregate.py index fe20786..256f9c6 100644 --- a/src/sqlquality/workload/aggregate.py +++ b/src/sqlquality/workload/aggregate.py @@ -4,6 +4,7 @@ import re from collections import defaultdict +from functools import lru_cache from sqlquality.models import Aggregation, ColumnRole, ColumnUsage, Workload from sqlquality.sqlast import SqlParseError, parse @@ -13,6 +14,18 @@ _Key = tuple[str, str, ColumnRole] +@lru_cache(maxsize=4096) +def _identifier_pattern(name: str) -> re.Pattern[str]: + """Compiled whole-identifier matcher for one name, compiled once per name. + + ``star_tables`` tests every (star-stat, table) pair, and a schema with many tables was + recompiling the same handful of table-name patterns over and over, thrashing `re`'s own + pattern cache. Caching by name here means each identifier is compiled once regardless of + how many stats or tables it is checked against. + """ + return re.compile(rf"\b{re.escape(name)}\b") + + def mentions_identifier(name: str, text: str) -> bool: """True if ``name`` appears in ``text`` as a whole identifier, not merely a substring. @@ -23,7 +36,7 @@ def mentions_identifier(name: str, text: str) -> bool: while still matching across the punctuation SQL puts around identifiers: parens, commas, dots and `::` are all non-word characters. """ - return re.search(rf"\b{re.escape(name)}\b", text) is not None + return _identifier_pattern(name).search(text) is not None def mentions_table(name: str, sql: str) -> bool: @@ -56,7 +69,6 @@ def aggregate(workload: Workload, schema: dict, dialect: str) -> Aggregation: """Weight every (table, column, role) by the cost of the queries that use it.""" calls: dict[_Key, int] = defaultdict(int) cost: dict[_Key, float] = defaultdict(float) - fingerprints: dict[_Key, int] = defaultdict(int) #: Which query groups contributed each usage, so downstream rules can ask whether two #: usages co-occur in a single query rather than merely both being hot on the table. contributors: dict[_Key, set[str]] = defaultdict(set) @@ -73,7 +85,6 @@ def aggregate(workload: Workload, schema: dict, dialect: str) -> Aggregation: for key in triples: calls[key] += stat.calls cost[key] += stat.total_time_ms - fingerprints[key] += 1 contributors[key].add(stat.fingerprint) tables.add(key[0]) @@ -94,7 +105,6 @@ def aggregate(workload: Workload, schema: dict, dialect: str) -> Aggregation: calls=calls[(table, column, role)], cost_ms=cost[(table, column, role)], cost_share=(cost[(table, column, role)] / total) if total else 0.0, - fingerprints=fingerprints[(table, column, role)], fingerprint_ids=frozenset(contributors[(table, column, role)]), ) for (table, column, role) in calls diff --git a/tests/test_models.py b/tests/test_models.py index 12dd550..313b72a 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -80,7 +80,7 @@ def test_proposal_and_aggregation_construct(): calls=5, cost_ms=50.0, cost_share=0.5, - fingerprints=2, + fingerprint_ids=frozenset({"fp1", "fp2"}), ) agg = Aggregation( usage=(usage,), total_cost_ms=100.0, skipped_unqualifiable=0, tables=frozenset({"orders"}) @@ -100,3 +100,29 @@ def test_proposal_and_aggregation_construct(): def test_raw_query_row_requires_only_sql_calls_and_time(): row = RawQueryRow(sql="SELECT 1", calls=1, total_time_ms=1.0) assert row.bytes_scanned is None + + +def test_fingerprints_is_derived_from_the_id_set(): + """One source of truth. The two used to be separate fields kept in step by convention, + with nothing stopping a caller setting one and not the other.""" + usage = ColumnUsage( + table="orders", + column="status", + role=ColumnRole.EQUALITY, + calls=5, + cost_ms=50.0, + cost_share=0.5, + fingerprint_ids=frozenset({"a", "b"}), + ) + assert usage.fingerprints == 2 + + with pytest.raises(TypeError): + ColumnUsage( # type: ignore[call-arg] + table="orders", + column="status", + role=ColumnRole.EQUALITY, + calls=5, + cost_ms=50.0, + cost_share=0.5, + fingerprints=2, + ) diff --git a/tests/test_workload_aggregate.py b/tests/test_workload_aggregate.py index f82aea3..476d697 100644 --- a/tests/test_workload_aggregate.py +++ b/tests/test_workload_aggregate.py @@ -1,5 +1,6 @@ from sqlquality.models import ColumnRole, QueryStat, Workload from sqlquality.workload.aggregate import aggregate +from sqlquality.workload.fingerprint import FLAG_SELECT_STAR SCHEMA = {"orders": {"id": "INT", "status": "TEXT", "created_at": "TIMESTAMP"}} @@ -160,3 +161,37 @@ def test_empty_workload_yields_empty_aggregation_and_no_division_error(): assert agg.usage == () assert agg.total_cost_ms == 0.0 assert agg.tables == frozenset() + + +def test_star_tables_compiles_each_table_pattern_once(monkeypatch): + """A fresh regex per (stat x table) pair thrashes re's pattern cache on a wide schema.""" + import re as _re + + from sqlquality.workload import aggregate as agg + + compiles: list[str] = [] + real_compile = _re.compile + + def counting_compile(pattern, *args, **kwargs): + compiles.append(pattern) + return real_compile(pattern, *args, **kwargs) + + monkeypatch.setattr(agg._re if hasattr(agg, "_re") else _re, "compile", counting_compile) + workload = Workload( + stats=tuple( + QueryStat( + fingerprint=f"fp{i}", + sql="select * from orders", + calls=1, + total_time_ms=1.0, + flags=frozenset({FLAG_SELECT_STAR}), + ) + for i in range(5) + ), + window_description="w", + ) + schema = {f"t{i}": {"c": "int"} for i in range(20)} | {"orders": {"c": "int"}} + assert agg.star_tables(workload, schema) == frozenset({"orders"}) + assert len(compiles) <= len(schema), ( + f"compiled {len(compiles)} patterns for {len(schema)} tables across 5 stats" + ) diff --git a/tests/test_workload_rules.py b/tests/test_workload_rules.py index 5186a19..505ce91 100644 --- a/tests/test_workload_rules.py +++ b/tests/test_workload_rules.py @@ -32,7 +32,6 @@ def usage(column, role, cost_share=0.5, cost_ms=50.0, table="orders", fps=("fp1" calls=10, cost_ms=cost_ms, cost_share=cost_share, - fingerprints=len(fps), fingerprint_ids=frozenset(fps), ) From 1ac6326219f25834bc294d0851cd250c4e33bec0 Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Mon, 27 Jul 2026 13:08:15 +0200 Subject: [PATCH 13/27] docs: correct Task 5's RED expectation for the compile-count test I predicted the compile-counting test would fail pre-fix with "5 x 21 compiles". It would not: the pre-fix mentions_identifier calls re.search(pattern, text), which resolves through Python's private internal cache and never touches the public re.compile a monkeypatch can observe. The test passes vacuously against the unfixed code -- a green RED, and the tenth test on this project that would have looked like a guarantee without being one. The Task 5 implementer caught this and validated from the other end: implement, confirm green, then pull @lru_cache back off and confirm it fails for the right reason (105 compiles for 21 tables across 5 stats), then restore. The plan now prescribes that direction, because a cache-hit test that cannot fail is worth nothing. Co-Authored-By: Claude Opus 5 --- .../plans/2026-07-27-advise-postgres-hardening.md | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md b/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md index 754db19..9c30c85 100644 --- a/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md +++ b/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md @@ -884,7 +884,18 @@ def test_star_tables_compiles_each_table_pattern_once(monkeypatch): - [ ] **Step 2: Run test to verify it fails** Run: `uv run pytest tests/test_models.py -k fingerprints_is_derived tests/test_workload_aggregate.py -k star_tables_compiles -v` -Expected: FAIL — `fingerprints=` is currently accepted so no `TypeError` is raised, and the compile count is 5 × 21. +Expected: the `fingerprints` test FAILS — `fingerprints=` is currently accepted, so no `TypeError` is raised. + +**The compile-count test will pass pre-fix, and that is not evidence of anything.** The +current `mentions_identifier` calls `re.search(pattern_string, text)`, which resolves through +Python's *private* internal cache and never touches the public `re.compile` a monkeypatch can +see. So a pre-fix run counts zero compiles and passes vacuously — a green RED. + +Validate this one from the other end instead: implement the fix, confirm the test passes, +then remove the `@lru_cache` decorator, confirm it now *fails* (it should report roughly +`stats x tables` compiles), and restore it. Paste both outputs. A cache-hit test that cannot +fail is worth nothing, and this plan has already shipped several tests that looked like +guarantees and were not. - [ ] **Step 3: Implement** From e1519d823215ac172549b68de22b6ebf468fee32 Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Mon, 27 Jul 2026 13:18:35 +0200 Subject: [PATCH 14/27] test(advise): execute the introspection SQL against a real postgres MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds an opt-in `integration` marker (deselected by default via addopts, so `uv run pytest` stays green without Docker) plus a docker-compose Postgres 16 service with pg_stat_statements preloaded. The new suite runs all six introspection statements against a live server for the first time — the unit suite only ever checked them for drift. Two things a fixture couldn't have caught: pytest does not propagate a conftest.py's module-level `pytestmark` to sibling test files, so `-m integration` was silently selecting nothing until a `pytest_collection_modifyitems` hook applied the marker explicitly; and a freshly loaded table reports `reltuples = -1` and empty pg_stats until analyzed, racing autovacuum, so the seed fixture now runs ANALYZE before handing the schema to a test. --- CONTRIBUTING.md | 15 +++ pyproject.toml | 4 + tests/integration/__init__.py | 0 tests/integration/conftest.py | 103 +++++++++++++++++++ tests/integration/docker-compose.yml | 21 ++++ tests/integration/test_introspection_live.py | 73 +++++++++++++ 6 files changed, 216 insertions(+) create mode 100644 tests/integration/__init__.py create mode 100644 tests/integration/conftest.py create mode 100644 tests/integration/docker-compose.yml create mode 100644 tests/integration/test_introspection_live.py diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6be1713..9c376e3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -48,3 +48,18 @@ uv run mypy src/sqlquality - Any user-visible change (new flag, changed exit code, changed output, new config key) needs a `CHANGELOG.md` entry under the current unreleased version, in the appropriate `Added` / `Changed` / `Fixed` / `BREAKING` section. + +## Integration tests (optional) + +`advise`'s introspection SQL is only checked for drift by the default suite. To run it +against a real Postgres: + +```bash +docker compose -f tests/integration/docker-compose.yml up -d +uv run pytest -m integration +docker compose -f tests/integration/docker-compose.yml down +``` + +These are deselected by default, so `uv run pytest` stays green without Docker. Point them +at your own server with `SQLQUALITY_TEST_DSN`. They need the `postgres` extra +(`uv sync --extra postgres`). diff --git a/pyproject.toml b/pyproject.toml index 463d3d4..8ff5050 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -67,6 +67,10 @@ only-include = [ [tool.pytest.ini_options] testpaths = ["tests"] +markers = [ + "integration: requires a live Postgres (opt in with SQLQUALITY_TEST_DSN or `-m integration`)", +] +addopts = "-m 'not integration'" [tool.ruff] line-length = 100 diff --git a/tests/integration/__init__.py b/tests/integration/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py new file mode 100644 index 0000000..078c0cc --- /dev/null +++ b/tests/integration/conftest.py @@ -0,0 +1,103 @@ +"""Opt-in live-Postgres fixtures. + +Every test in this package is marked `integration` and deselected by default (see +pyproject.toml's addopts), so a contributor without Docker sees a clean `uv run pytest`. + +Bring the server up with: + docker compose -f tests/integration/docker-compose.yml up -d + uv run pytest -m integration +""" + +from __future__ import annotations + +import os +from pathlib import Path + +import pytest + +pytest.importorskip("psycopg", reason="integration tests need the postgres extra") + +DEFAULT_DSN = "postgresql://postgres:sqlquality@127.0.0.1:55432/sqlquality_test" + +_PACKAGE_DIR = Path(__file__).parent + + +def pytest_collection_modifyitems(items: list[pytest.Item]) -> None: + """Mark every test collected under this package as `integration`. + + A module-level `pytestmark` only applies to the module that defines it — pytest does + not propagate a conftest.py's `pytestmark` to sibling test files in the same + directory. This hook is what actually makes "every test in this package is marked + integration" true, including for test files added here later. + """ + for item in items: + if _PACKAGE_DIR in item.path.parents: + item.add_marker(pytest.mark.integration) + + +@pytest.fixture(scope="session") +def live_dsn() -> str: + """A reachable Postgres, or skip with an actionable message.""" + import psycopg + + dsn = os.environ.get("SQLQUALITY_TEST_DSN", DEFAULT_DSN) + try: + with psycopg.connect(dsn, connect_timeout=3) as conn: + with conn.cursor() as cur: + cur.execute("SELECT 1") + except Exception as exc: # driver-specific; the message is what matters + pytest.skip( + f"no Postgres at {dsn}: {exc}\n" + "start one with: docker compose -f tests/integration/docker-compose.yml up -d" + ) + return dsn + + +@pytest.fixture(scope="session") +def seeded(live_dsn: str) -> tuple[str, str]: + """A schema with the index shapes the catalog query has to survive, plus real workload. + + Deliberately includes a partial and an expression index: those are exactly the rows the + shipped statement discarded, and the only way to know the fix works is to read them back + out of a real catalog. + """ + import psycopg + + schema = "advise_it" + with psycopg.connect(live_dsn, autocommit=True) as conn: + with conn.cursor() as cur: + cur.execute("CREATE EXTENSION IF NOT EXISTS pg_stat_statements") + cur.execute(f"DROP SCHEMA IF EXISTS {schema} CASCADE") + cur.execute(f"CREATE SCHEMA {schema}") + cur.execute( + f"""CREATE TABLE {schema}.orders ( + id bigserial PRIMARY KEY, + status text NOT NULL, + note text, + shipped_at timestamptz, + created_at timestamptz NOT NULL DEFAULT now())""" + ) + cur.execute(f"CREATE INDEX idx_plain ON {schema}.orders (status, created_at)") + cur.execute( + f"CREATE INDEX idx_open ON {schema}.orders (status) WHERE shipped_at IS NULL" + ) + cur.execute(f"CREATE INDEX idx_lower_note ON {schema}.orders (lower(note))") + cur.execute( + f"INSERT INTO {schema}.orders (status, note) " + "SELECT 'paid', 'n' || g FROM generate_series(1, 500) g" + ) + # A freshly loaded table reports reltuples = -1 and has no pg_stats rows until + # analyzed — autovacuum gets to it eventually, but not necessarily before this + # fixture's caller queries it. ANALYZE makes the row estimate and NDV + # deterministic instead of racing autovacuum. + cur.execute(f"ANALYZE {schema}.orders") + cur.execute("SELECT pg_stat_statements_reset()") + # Real workload for the history statement to find. + for _ in range(3): + cur.execute( + f"SELECT id FROM {schema}.orders WHERE status = %s " + "AND created_at > now() - interval '1 day'", + ("paid",), + ) + cur.fetchall() + return live_dsn, schema diff --git a/tests/integration/docker-compose.yml b/tests/integration/docker-compose.yml new file mode 100644 index 0000000..3492181 --- /dev/null +++ b/tests/integration/docker-compose.yml @@ -0,0 +1,21 @@ +# pg_stat_statements must be preloaded at server start; CREATE EXTENSION alone is not +# enough, which is why this is a compose file rather than a plain `services:` block. +services: + postgres: + image: postgres:16 + environment: + POSTGRES_PASSWORD: sqlquality + POSTGRES_DB: sqlquality_test + command: + - postgres + - -c + - shared_preload_libraries=pg_stat_statements + - -c + - pg_stat_statements.track=all + ports: + - "55432:5432" + healthcheck: + test: ["CMD-SHELL", "pg_isready -U postgres -d sqlquality_test"] + interval: 2s + timeout: 3s + retries: 30 diff --git a/tests/integration/test_introspection_live.py b/tests/integration/test_introspection_live.py new file mode 100644 index 0000000..4434933 --- /dev/null +++ b/tests/integration/test_introspection_live.py @@ -0,0 +1,73 @@ +"""Execute every introspection statement against a real server. + +The unit suite only checks these statements for drift, which cannot catch a wrong column +name, a wrong join, or a view that does not exist. This is the only place they run. +""" + +from __future__ import annotations + +import pytest + +from sqlquality.models import ConnectionParams +from sqlquality.workload.postgres import PostgresWorkloadAdapter + + +@pytest.fixture +def adapter(seeded: tuple[str, str]) -> PostgresWorkloadAdapter: + dsn, schema = seeded + a = PostgresWorkloadAdapter() + a.schemas = (schema,) + a.connect(ConnectionParams(engine="postgres", dsn=dsn, fields={}, source="--dsn"), 30) + return a + + +def test_every_introspection_statement_executes(adapter, seeded): + """No statement may raise, and none may report a degraded capability.""" + _dsn, schema = seeded + adapter.fetch_workload(None, 500) + adapter.fetch_schema((schema,)) + adapter.fetch_table_facts((schema,), frozenset({"orders"})) + adapter.fetch_indexes((schema,), frozenset({"orders"})) + assert adapter.degraded == [], f"a statement failed against a real server: {adapter.degraded}" + + +def test_workload_statement_returns_our_own_queries(adapter): + fetch = adapter.fetch_workload(None, 500) + assert fetch.rows, "pg_stat_statements returned nothing" + assert "since stats reset at" in fetch.window_description + + +def test_table_facts_reports_a_real_row_estimate_and_ndv(adapter, seeded): + _dsn, schema = seeded + facts = adapter.fetch_table_facts((schema,), frozenset({"orders"}))["orders"] + assert facts.row_estimate is not None and facts.row_estimate > 0 + assert "status" in facts.columns + assert facts.ndv, "pg_stats returned no distinct-value estimates" + + +def test_indexes_statement_reads_partial_and_expression_metadata(adapter, seeded): + """The reason Task 2 exists, verified against a real catalog rather than a fixture.""" + _dsn, schema = seeded + by_name = {i.name: i for i in adapter.fetch_indexes((schema,), frozenset({"orders"}))["orders"]} + + assert by_name["idx_plain"].columns == ("status", "created_at") + assert by_name["idx_plain"].is_partial is False + assert by_name["idx_plain"].has_expressions is False + + assert by_name["idx_open"].is_partial is True + assert "shipped_at IS NULL" in (by_name["idx_open"].predicate or "") + + # The row the shipped statement silently dropped. + assert by_name["idx_lower_note"].has_expressions is True + assert "lower(note)" in (by_name["idx_lower_note"].definition or "") + + assert by_name["orders_pkey"].is_primary is True + + +def test_the_session_really_is_read_only(adapter, seeded): + """Invariant 2, against a real server: the session must refuse a write.""" + import psycopg + + _dsn, schema = seeded + with pytest.raises(psycopg.errors.ReadOnlySqlTransaction): + adapter._query(f"CREATE TABLE {schema}.should_not_exist (x int)", ()) From 71f8bec36f0e5ddd52e59cb28a9139c118374539 Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Mon, 27 Jul 2026 13:21:49 +0200 Subject: [PATCH 15/27] docs: fix the reltuples -1 sentinel the live suite found Task 6's first real run against a live Postgres found a production bug no fixture could have shown, which is exactly what the task was added for. pg_class.reltuples is -1 on Postgres 14+ for a table that has never been analyzed -- distinct from 0, which means analyzed and genuinely empty. fetch_table_facts passed it straight through, and propose_indexes' small-table gate then read -1 < 10000 and suppressed every proposal for the table, silently. Measured: reltuples=-1 -> NO PROPOSAL (suppressed) reltuples=0 -> NO PROPOSAL (suppressed) reltuples=None -> low, "row count unknown" reltuples=8000000 -> high The window where this bites is precisely when someone reaches for advise: a freshly loaded or migrated table, before autovacuum's first ANALYZE, with slow queries. They get no advice and no reason for it. Same silent-suppression class as the partial-index coverage bug earlier in this plan. None already means unknown everywhere and that path is correct, so the fix is translating the sentinel at the boundary; everything downstream then behaves. Committed separately from the integration suite, since it is a production fix the suite happened to find. Also corrects two defects in my own Task 6 text that the implementer hit: a module-level pytestmark in conftest.py does not mark sibling test modules (so -m integration selected nothing until a collection hook was added), and the verbatim import list carried six unused CAP_* constants that fail ruff. Co-Authored-By: Claude Opus 5 --- .../2026-07-27-advise-postgres-hardening.md | 96 +++++++++++++++++++ 1 file changed, 96 insertions(+) diff --git a/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md b/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md index 9c30c85..e4aac07 100644 --- a/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md +++ b/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md @@ -994,6 +994,23 @@ services: `tests/integration/__init__.py`: empty. +**`pytestmark` in `conftest.py` does not mark sibling test modules.** A module-level +`pytestmark` applies only to the module it is written in, so putting it in `conftest.py` +marks nothing and `-m integration` selects zero tests. Mark the package from the conftest +with a collection hook instead: + +```python +def pytest_collection_modifyitems(items): + """Mark every test in this package `integration`. + + A module-level `pytestmark` in a conftest does not propagate to sibling modules, so + without this the marker exists and selects nothing. + """ + for item in items: + if "tests/integration/" in str(item.path).replace("\\", "/"): + item.add_marker("integration") +``` + `tests/integration/conftest.py`: ```python @@ -1190,6 +1207,85 @@ uv run pytest -q Expected: the same count as before this task, no skips, no errors. +- [ ] **Step 4b: Translate Postgres's never-analyzed sentinel** + +The live run surfaced a production bug no fixture could have shown. `pg_class.reltuples` is +**-1** on Postgres 14+ for a table that has never been analyzed — distinct from `0`, which +means analyzed and genuinely empty. `fetch_table_facts` passes that straight through as a +row estimate, and `propose_indexes`' small-table gate then reads `-1 < MIN_ROWS_FOR_INDEX` +and **suppresses every proposal for the table, silently**. Measured: + +``` +reltuples= -1 never analyzed (PG14+) -> NO PROPOSAL (suppressed) +reltuples= 0 analyzed, empty -> NO PROPOSAL (suppressed) +reltuples= None unknown -> low — Add index on orders(status) +reltuples= 8000000 large -> high — Add index on orders(status) +``` + +The window where this bites is exactly when someone reaches for `advise`: a freshly loaded +or migrated table, before autovacuum's first `ANALYZE`, with slow queries. They get no +advice and no reason. + +`None` already means "unknown" throughout, and that path is correct — it proposes at LOW and +says the row count could not be checked. So the whole fix is translating the sentinel at the +boundary. In `fetch_table_facts`, replace the `sizes` comprehension's row term: + +```python + sizes = { + str(name): ( + # Postgres uses -1 for "never analyzed", which is *unknown*, not "very + # small". Passed through, it reads as a tiny table and the small-table gate + # silently suppresses every proposal — worst exactly after a load or + # migration, which is when someone runs advise. None routes it into the + # existing unknown-row-count path: proposed at LOW, with the gap stated. + (lambda r: None if r < 0 else r)(_as_int(rows)), + _as_int(size) if size is not None else None, + ) + for name, rows, size in self._run(CAP_TABLE_FACTS, (list(schemas), wanted)) + } +``` + +Prefer a small named helper over the inline lambda if it reads better; the behaviour is what +matters. Add to `tests/test_workload_postgres.py`: + +```python +def test_a_never_analyzed_table_reports_an_unknown_row_count(): + """Postgres 14+ stores -1 in reltuples for a table that has never been analyzed. + + Passed through, the small-table gate reads it as a tiny table and suppresses every + proposal — silently, and precisely in the window after a load or migration when someone + would run advise. -1 means unknown, and unknown already has a correct path. + """ + querier = FakeQuerier({ + "information_schema.columns": [("orders", "id", "integer")], + "pg_total_relation_size": [("orders", -1, 10**9)], + }) + facts = PostgresWorkloadAdapter(querier=querier).fetch_table_facts( + ("public",), frozenset({"orders"}) + ) + assert facts["orders"].row_estimate is None + + +def test_an_analyzed_empty_table_still_reports_zero(): + """0 is a real answer — analyzed and empty — and must not be conflated with unknown.""" + querier = FakeQuerier({ + "information_schema.columns": [("orders", "id", "integer")], + "pg_total_relation_size": [("orders", 0, 8192)], + }) + facts = PostgresWorkloadAdapter(querier=querier).fetch_table_facts( + ("public",), frozenset({"orders"}) + ) + assert facts["orders"].row_estimate == 0 +``` + +Commit this separately from the integration suite — it is a production fix that the +integration suite happened to find, and the two do not belong in one commit: + +```bash +git add src/sqlquality/workload/postgres.py tests/test_workload_postgres.py +git commit -m "fix(advise): treat reltuples -1 as unknown, not as a tiny table" +``` + - [ ] **Step 5: Document it** Add to `CONTRIBUTING.md` after the four-checks section: From cca460dca5a7626bb7865a830182127b03a5a301 Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Mon, 27 Jul 2026 13:24:40 +0200 Subject: [PATCH 16/27] fix(advise): treat reltuples -1 as unknown, not as a tiny table Postgres 14+ stores -1 in pg_class.reltuples for a table that has never been analyzed, distinct from 0 (analyzed and genuinely empty). Passed through unchanged, fetch_table_facts handed propose_indexes and propose_partial_indexes a row_estimate of -1, which the small-table gate reads as tiny and suppresses every proposal for that table with no message. The window this bites is exactly when someone reaches for `advise`: a freshly loaded or migrated table, before autovacuum's first ANALYZE, with slow queries already running. Found by tests/integration's live run against a real, unanalyzed table (the suite's fixture now runs ANALYZE explicitly to stay deterministic, which is why the unit tests below are what pin this rather than the live suite). None already means "unknown" throughout and proposes at LOW with the gap stated, so the fix is translating the sentinel at the fetch_table_facts boundary via a new _row_estimate() helper. Both downstream rules (ADV001, ADV004) read the same TableFacts.row_estimate field and need no changes. --- src/sqlquality/workload/postgres.py | 18 +++++++++++++++- tests/test_workload_postgres.py | 33 +++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/src/sqlquality/workload/postgres.py b/src/sqlquality/workload/postgres.py index bd5414a..975e1cc 100644 --- a/src/sqlquality/workload/postgres.py +++ b/src/sqlquality/workload/postgres.py @@ -134,6 +134,22 @@ def _as_float(value: object) -> float: return float(value) # type: ignore[arg-type] +def _row_estimate(value: object) -> int | None: + """`pg_class.reltuples`, with Postgres's never-analyzed sentinel translated to unknown. + + Postgres 14+ stores -1 in `reltuples` for a table that has never been analyzed — + distinct from 0, which means analyzed and genuinely empty. Passed through as-is, -1 + reads as a tiny table to the small-table gate in `propose_indexes`, which then + suppresses every proposal for that table with no message. The window where this bites + is exactly when someone reaches for `advise`: a freshly loaded or migrated table, + before autovacuum's first ANALYZE, with slow queries. `None` already means "unknown" + throughout — it proposes at LOW and says the row count could not be checked — so + translating the sentinel here is the whole fix. + """ + rows = _as_int(value) + return None if rows < 0 else rows + + @dataclass class _IndexRows: """Mutable per-index collector while unnested index rows are grouped. @@ -943,7 +959,7 @@ def fetch_table_facts( wanted = sorted(tables) sizes = { str(name): ( - _as_int(rows), + _row_estimate(rows), _as_int(size) if size is not None else None, ) for name, rows, size in self._run(CAP_TABLE_FACTS, (list(schemas), wanted)) diff --git a/tests/test_workload_postgres.py b/tests/test_workload_postgres.py index 782ae86..2812ea6 100644 --- a/tests/test_workload_postgres.py +++ b/tests/test_workload_postgres.py @@ -226,6 +226,39 @@ def test_absolute_n_distinct_survives_a_missing_row_count(): assert facts["orders"].ndv["id"] == 500.0 +def test_a_never_analyzed_table_reports_an_unknown_row_count(): + """Postgres 14+ stores -1 in reltuples for a table that has never been analyzed. + + Passed through, the small-table gate reads it as a tiny table and suppresses every + proposal — silently, and precisely in the window after a load or migration when someone + would run advise. -1 means unknown, and unknown already has a correct path. + """ + querier = FakeQuerier( + { + "information_schema.columns": [("orders", "id", "integer")], + "pg_total_relation_size": [("orders", -1, 10**9)], + } + ) + facts = PostgresWorkloadAdapter(querier=querier).fetch_table_facts( + ("public",), frozenset({"orders"}) + ) + assert facts["orders"].row_estimate is None + + +def test_an_analyzed_empty_table_still_reports_zero(): + """0 is a real answer — analyzed and empty — and must not be conflated with unknown.""" + querier = FakeQuerier( + { + "information_schema.columns": [("orders", "id", "integer")], + "pg_total_relation_size": [("orders", 0, 8192)], + } + ) + facts = PostgresWorkloadAdapter(querier=querier).fetch_table_facts( + ("public",), frozenset({"orders"}) + ) + assert facts["orders"].row_estimate == 0 + + def test_fetch_indexes_restores_column_order_from_ordinality(): """Rows arriving out of order must still yield the right composite order. From eb7708c7a1aaff0c066804fb10bbed2dbf344635 Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Mon, 27 Jul 2026 13:35:12 +0200 Subject: [PATCH 17/27] test(advise): end-to-end run against a real postgres; narrow two limitations --- README.md | 29 ++++++----- tests/integration/test_advise_live.py | 70 +++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 15 deletions(-) create mode 100644 tests/integration/test_advise_live.py diff --git a/README.md b/README.md index 03eee29..2a6be5b 100644 --- a/README.md +++ b/README.md @@ -382,11 +382,9 @@ review and apply by hand. - **HIGH** — cost share above `--min-cost-share`, **and** supporting catalog stats present (e.g. NDV), **and** confirmation that the proposed index does not already exist. -- **MEDIUM** — cost evidence is solid but a catalog input is missing or stale. ADV002 and - ADV003 are capped at MEDIUM unconditionally: `idx_scan` only accumulates since the last - statistics reset, so zero scans can never prove an index is unused across a full business - cycle, and ADV003 compares column lists without being able to see a partial index's - predicate. +- **MEDIUM** — cost evidence is solid but a catalog input is missing or stale. ADV002 is + capped at MEDIUM unconditionally: `idx_scan` only accumulates since the last statistics + reset, so zero scans can never prove an index is unused across a full business cycle. - **LOW** — thin evidence, and specifically **any check that could not be run**: the row count is unknown so the small-table floor could not be applied, or the existing-index list was denied so "no index already covers this" could not be confirmed. Absent @@ -779,16 +777,17 @@ LLM suggestions unavailable: The 'anthropic' package is required for AnthropicPr be most of your hot reads. They are counted, and the skip line calls them `filtered` rather than pretending they were introspection or DDL, but they are not analyzed. Unwrapping to the inner `SELECT` is a follow-up. -- **Expression indexes are invisible to the catalog query.** Postgres's `pg_index.indkey` - holds `0` for an expression column, which matches no `pg_attribute` row, so ADV001 may - propose a plain-column index whose `lower(col)` expression-index equivalent already - exists. -- **ADV003 cannot see partial-index predicates.** It compares column lists only, so it - could recommend dropping a partial index in favor of a wider full index that does not - actually cover the same rows. Its confidence is capped at MEDIUM for that reason, and - the caveat is repeated in the proposal's own rationale — a README does not travel - inside the `.sql` file you run. `advise` cannot tell you which of the two indexes is - partial or expression-based; you have to check. +- **Expression indexes are read but not matched.** `advise` now sees that an index on + `lower(status)` exists and names it in the proposal's evidence, but it cannot tell whether + that index already serves a lookup on `status` — so it proposes and says so, rather than + suppressing or ignoring. Confirm before applying. +- **ADV003 only compares plain indexes.** A pair where either index carries a `WHERE` + predicate or an indexed expression is skipped entirely rather than proposed at lower + confidence: a partial index exists to serve a subset, so recommending its removal is + likely wrong rather than merely uncertain. Plain pairs are reported at HIGH. +- **A partial index does not suppress a proposal.** `idx ON orders(status) WHERE + shipped_at IS NULL` does not serve `WHERE status = $1`, so it is not treated as covering + a candidate index — it is named in the evidence instead. - **One schema per run.** Every catalog fact is keyed on the bare relation name — table sizes, NDV statistics, index lists and the `qualify()` schema all merge across schemas — so `orders` in two schemas would alias into one another and the last catalog row read diff --git a/tests/integration/test_advise_live.py b/tests/integration/test_advise_live.py new file mode 100644 index 0000000..784515a --- /dev/null +++ b/tests/integration/test_advise_live.py @@ -0,0 +1,70 @@ +"""One whole `advise` run against a real database. + +Every other test stubs the querier. This is the only path that exercises resolve_connection +-> connect -> six statements -> ingest -> aggregate -> propose -> render as one piece. +""" + +from __future__ import annotations + +import json + +import pytest +from typer.testing import CliRunner + +from sqlquality.cli import app + +pytestmark = pytest.mark.integration +runner = CliRunner() + + +def test_advise_end_to_end(seeded, tmp_path): + dsn, schema = seeded + md = tmp_path / "report.md" + ddl = tmp_path / "proposals.sql" + result = runner.invoke( + app, + [ + "advise", + "--dsn", + dsn, + "--schema", + schema, + "--json", + "--markdown", + str(md), + "--ddl", + str(ddl), + ], + ) + assert result.exit_code == 0, result.output + payload = json.loads(result.stdout) + + assert payload["engine"] == "postgres" + assert payload["redacted"] is True + assert payload["analyzed"]["query_groups"] > 0 + assert payload["degraded"] == [] + assert md.read_text(encoding="utf-8").startswith("# sqlquality advise") + assert "REVIEW BEFORE RUNNING" in ddl.read_text(encoding="utf-8") + + +def test_advise_does_not_leak_a_literal_from_a_real_server(seeded, tmp_path): + """The redaction guarantee, against real pg_stat_statements rather than a fixture. + + The seeded workload filters on the literal 'paid'. pg_stat_statements normalises it to + $1, but a run with --keep-literals proves the surfaces would carry it if we let them. + """ + dsn, schema = seeded + md = tmp_path / "report.md" + result = runner.invoke( + app, ["advise", "--dsn", dsn, "--schema", schema, "--json", "--markdown", str(md)] + ) + assert result.exit_code == 0, result.output + assert "'paid'" not in result.stdout + assert "'paid'" not in md.read_text(encoding="utf-8") + + +def test_advise_dry_run_needs_no_server(tmp_path): + """The audit path must not depend on anything being reachable.""" + result = runner.invoke(app, ["advise", "--engine", "postgres", "--dry-run"]) + assert result.exit_code == 0 + assert "pg_stat_statements" in result.stdout From 7696ca2822f5849a07908e50babf8d7fea3a31f7 Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Mon, 27 Jul 2026 13:36:31 +0200 Subject: [PATCH 18/27] docs: stop the live test claiming redaction coverage it cannot provide Task 7's implementer proved my end-to-end redaction test cannot fail. Postgres normalises constants to $N inside pg_stat_statements before sqlquality sees the query text, so 'paid' is already gone on arrival -- confirmed by running the scenario with --keep-literals, which bypasses redact_tree entirely, and still finding no 'paid' anywhere. Eleventh test on this project that looked like a guarantee and wasn't, and this one was mine, in the plan text. Its name and docstring both said "the redaction guarantee, against a real server", which is exactly the sentence a future reader would trust instead of re-deriving. Kept, renamed and re-documented rather than deleted, because it does pin something real: that nothing downstream of ingest -- evidence dicts, rationales, DDL, the renderers -- reintroduces raw query text into an artifact. That is a live risk, since ADV005 and ADV006 both copy SQL into evidence. The docstring now states plainly that it cannot fail if redact_tree breaks, and points at tests/test_workload_redaction.py, which feeds un-normalised literals through a fake querier and does fail under mutation. Co-Authored-By: Claude Opus 5 --- .../2026-07-27-advise-postgres-hardening.md | 27 +++++++++++++++---- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md b/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md index e4aac07..b5813e1 100644 --- a/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md +++ b/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md @@ -1370,11 +1370,22 @@ def test_advise_end_to_end(seeded, tmp_path): assert "REVIEW BEFORE RUNNING" in ddl.read_text(encoding="utf-8") -def test_advise_does_not_leak_a_literal_from_a_real_server(seeded, tmp_path): - """The redaction guarantee, against real pg_stat_statements rather than a fixture. - - The seeded workload filters on the literal 'paid'. pg_stat_statements normalises it to - $1, but a run with --keep-literals proves the surfaces would carry it if we let them. +def test_advise_output_carries_no_query_literal_from_a_real_server(seeded, tmp_path): + """A regression guard on the pipeline, NOT a test of `redact_tree`. Read on. + + Postgres normalises constants to `$N` inside `pg_stat_statements` before sqlquality ever + sees the query text, so `'paid'` is already gone on arrival. Measured: running this + scenario with `--keep-literals`, which bypasses our redaction entirely, still shows no + `'paid'` anywhere. **This test therefore cannot fail if `redact_tree` breaks**, and it + would be dishonest to call it redaction coverage. + + What it does pin, which is worth pinning: that nothing downstream of ingest — evidence + dicts, rationales, DDL, the renderers — reintroduces raw query text into an artifact. + That is a real regression risk, since ADV005 and ADV006 both copy SQL into evidence. + + The actual guard on `redact_tree` is `tests/test_workload_redaction.py`, which feeds + un-normalised literals through a fake querier and *does* fail when redaction is + disabled — verified there by mutation. """ dsn, schema = seeded md = tmp_path / "report.md" @@ -1384,6 +1395,12 @@ def test_advise_does_not_leak_a_literal_from_a_real_server(seeded, tmp_path): assert result.exit_code == 0, result.output assert "'paid'" not in result.stdout assert "'paid'" not in md.read_text(encoding="utf-8") + # Pin the reason this test is weak, so nobody later mistakes it for redaction coverage: + # the literal is already absent from what Postgres hands us. + fetch_sql = " ".join( + stat["sql"] for stat in json.loads(result.stdout).get("proposals", []) if "sql" in stat + ) + assert "'paid'" not in fetch_sql def test_advise_dry_run_needs_no_server(tmp_path): From 774a12388076a124590b991aee0d89506d51b6e4 Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Mon, 27 Jul 2026 13:46:11 +0200 Subject: [PATCH 19/27] test(advise): stop calling the live literal-guard test a redaction guarantee Postgres normalises pg_stat_statements query text before we ever see it, so the test cannot fail if redact_tree breaks. Rename it, document what it actually pins (no downstream surface reintroduces raw SQL), and point at tests/test_workload_redaction.py as the real guard on redact_tree. --- tests/integration/test_advise_live.py | 29 +++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/tests/integration/test_advise_live.py b/tests/integration/test_advise_live.py index 784515a..eccc4c7 100644 --- a/tests/integration/test_advise_live.py +++ b/tests/integration/test_advise_live.py @@ -47,11 +47,22 @@ def test_advise_end_to_end(seeded, tmp_path): assert "REVIEW BEFORE RUNNING" in ddl.read_text(encoding="utf-8") -def test_advise_does_not_leak_a_literal_from_a_real_server(seeded, tmp_path): - """The redaction guarantee, against real pg_stat_statements rather than a fixture. +def test_advise_output_carries_no_query_literal_from_a_real_server(seeded, tmp_path): + """A regression guard on the pipeline, NOT a test of `redact_tree`. Read on. - The seeded workload filters on the literal 'paid'. pg_stat_statements normalises it to - $1, but a run with --keep-literals proves the surfaces would carry it if we let them. + Postgres normalises constants to `$N` inside `pg_stat_statements` before sqlquality ever + sees the query text, so `'paid'` is already gone on arrival. Measured: running this + scenario with `--keep-literals`, which bypasses our redaction entirely, still shows no + `'paid'` anywhere. **This test therefore cannot fail if `redact_tree` breaks**, and it + would be dishonest to call it redaction coverage. + + What it does pin, which is worth pinning: that nothing downstream of ingest — evidence + dicts, rationales, DDL, the renderers — reintroduces raw query text into an artifact. + That is a real regression risk, since ADV005 and ADV006 both copy SQL into evidence. + + The actual guard on `redact_tree` is `tests/test_workload_redaction.py`, which feeds + un-normalised literals through a fake querier and *does* fail when redaction is + disabled — verified there by mutation. """ dsn, schema = seeded md = tmp_path / "report.md" @@ -61,6 +72,16 @@ def test_advise_does_not_leak_a_literal_from_a_real_server(seeded, tmp_path): assert result.exit_code == 0, result.output assert "'paid'" not in result.stdout assert "'paid'" not in md.read_text(encoding="utf-8") + # Pin the reason this test is weak, so nobody later mistakes it for redaction coverage: + # the literal is already absent from what Postgres hands us. `sql` lives inside each + # proposal's `evidence` dict (only ADV005's leading-wildcard-LIKE and ADV006 carry it), + # not as a top-level proposal key — see `advise_payload` in report.py. + fetch_sql = " ".join( + p["evidence"]["sql"] + for p in json.loads(result.stdout).get("proposals", []) + if "sql" in p.get("evidence", {}) + ) + assert "'paid'" not in fetch_sql def test_advise_dry_run_needs_no_server(tmp_path): From 678a3b7a8ef9422c38620b136a65676a68b82ec6 Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Mon, 27 Jul 2026 13:48:46 +0200 Subject: [PATCH 20/27] docs: add Task 8 -- redaction dismembers Postgres placeholders Task 7's live run found the most impactful defect in this feature, and it took a real server to surface it. pg_stat_statements hands us SQL with literals already replaced by $N markers. A $N parses as Parameter(this=Literal(N)), so redact_tree's literal walk descends INTO the placeholder and rewrites the index. Measured on the real text: in: ... status = $1 AND created_at > now() - interval $2 out: ... status = $%s AND created_at > CURRENT_TIMESTAMP - INTERVAL INTERVAL is left dangling. ingest() stores that as QueryStat.sql, aggregate() re-parses it, and sqlglot reads the trailing INTERVAL as a COLUMN NAME, which fails to qualify -- so the whole query group is dropped into skipped_unqualifiable. Reach: any query where Postgres normalised a literal inside an INTERVAL. `created_at > now() - interval '1 day'` is a time-window filter, the single most ordinary shape in the workloads advise exists for. It has been silently discarding them. Not entirely silent -- the coverage line says "1 unresolvable" -- but nothing tells the user sqlquality's own redaction broke the query it then could not parse. The fix is a skip rather than a repair: a $N is Postgres's own marker standing where a literal already was, so there is nothing in it to redact and descending can only corrupt. Verified the fix preserves $1 and `interval $2` intact while still erasing a real literal beside them. Step 4 re-runs the redaction guarantee's mutation check, because the skip narrows what gets redacted and that property has to be re-proven, not assumed. Step 5 checks the live run now yields proposals -- the seeded workload's only non-noise statement was the one being dropped, so it produced zero. Co-Authored-By: Claude Opus 5 --- .../2026-07-27-advise-postgres-hardening.md | 139 ++++++++++++++++++ 1 file changed, 139 insertions(+) diff --git a/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md b/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md index b5813e1..87f2051 100644 --- a/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md +++ b/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md @@ -1457,6 +1457,145 @@ git commit -m "test(advise): end-to-end run against a real postgres; narrow two --- +### Task 8: Stop redacting Postgres's own placeholders + +**Files:** +- Modify: `src/sqlquality/workload/fingerprint.py` (`redact_tree`) +- Test: `tests/test_workload_fingerprint.py` + +**Interfaces:** no signature change. `redact_tree(tree) -> exp.Expression` keeps its contract. + +**Discovered by Task 7's live run, and the most impactful defect in the feature.** +`pg_stat_statements` hands us SQL with literals already replaced by `$N` markers. `redact_tree` +walks every `exp.Literal` — and a `$N` parses as `Parameter(this=Literal(2))`, so the walk +descends *into the placeholder* and replaces the `2`. Measured against the real text the live +server produced: + +``` +in: SELECT id FROM orders WHERE status = $1 AND created_at > now() - interval $2 +out: SELECT id FROM orders WHERE status = $%s AND created_at > CURRENT_TIMESTAMP - INTERVAL +``` + +`INTERVAL` is left as a bare dangling token. `ingest()` stores that string as +`QueryStat.sql`, `aggregate()` re-parses it, and sqlglot reads the trailing `INTERVAL` as a +**column name** — which then fails to qualify, so the entire query group is dropped and +counted in `skipped_unqualifiable`. + +Reach: any query where Postgres normalised a literal inside an `INTERVAL`. `created_at > +now() - interval '1 day'` is a time-window filter — the single most ordinary shape in the +analytical workloads `advise` exists to advise on. It has been silently discarding them. + +It is not *entirely* silent — the coverage line reports it as "1 unresolvable" — but a user +has no way to learn that sqlquality's own redaction broke the query it then failed to parse. + +**Why the fix is a skip, not a repair.** A `$N` is not user data. It is Postgres's own marker, +already standing where a literal used to be, and there is nothing left in it to redact. +Descending into it can only corrupt. + +- [ ] **Step 1: Write the failing test** + +Append to `tests/test_workload_fingerprint.py`: + +```python +def test_redaction_leaves_a_postgres_placeholder_intact(): + """`$N` parses as Parameter(this=Literal(N)), so a naive literal walk mangles it. + + pg_stat_statements hands us `$1`; redacting the `1` inside it produced `$%s`. + """ + tree = parse("select id from orders where status = $1", "postgres") + assert "$1" in redact_tree(tree).sql("postgres") + + +def test_redaction_does_not_dismember_a_normalised_interval(): + """The shape that was silently dropped, using the real text a live server produced. + + Redacting the literal inside `interval $2` left `INTERVAL` as a bare dangling token. + Re-parsed, sqlglot read it as a *column* named INTERVAL, which failed to qualify — so + the whole group vanished into skipped_unqualifiable. `created_at > now() - interval + '1 day'` is the most ordinary filter shape there is. + """ + sql = "select id from orders where status = $1 and created_at > now() - interval $2" + redacted = redact_tree(parse(sql, "postgres")).sql("postgres") + assert not redacted.rstrip().endswith("INTERVAL") + assert "INTERVAL $2" in redacted.upper() + + +def test_redaction_still_erases_a_real_literal_beside_a_placeholder(): + """The control. Skipping placeholders must not smuggle a genuine literal through.""" + sql = "select id from orders where status = 'secret-value' and n > $1" + redacted = redact_tree(parse(sql, "postgres")).sql("postgres") + assert "secret-value" not in redacted + assert "$1" in redacted +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `uv run pytest tests/test_workload_fingerprint.py -k "placeholder or interval or beside" -v` +Expected: the first two FAIL — `$1` has become `$%s`, and the redacted string ends in a bare +`INTERVAL`. The third passes already; it is the control that must keep passing. + +- [ ] **Step 3: Skip placeholder subtrees** + +In `redact_tree`, leave any literal that sits inside an `exp.Parameter` alone: + +```python +def _inside_placeholder(node: exp.Expression) -> bool: + """True if ``node`` sits inside a `$N` parameter marker. + + `pg_stat_statements` has already replaced the literal that was there; the integer left + behind is Postgres's own index, not user data, and rewriting it corrupts the statement. + """ + parent = node.parent + while parent is not None: + if isinstance(parent, exp.Parameter): + return True + parent = parent.parent + return False + + +def redact_tree(tree: exp.Expression) -> exp.Expression: + """Return a copy of ``tree`` with every literal replaced by a bind placeholder.""" + copy = tree.copy() + for literal in list(copy.find_all(exp.Literal)): + if _inside_placeholder(literal): + continue + literal.replace(exp.Placeholder()) + return copy +``` + +- [ ] **Step 4: Run the tests** + +Run: `uv run pytest -q` +Expected: PASS. **The pre-existing redaction guarantee suite must still pass unchanged** — +`tests/test_workload_redaction.py` is the real guard on this function, and if the skip let a +literal through, that is where it shows. + +Then re-run its mutation check: disable the literal replacement, confirm +`test_no_literal_reaches_json_markdown_ddl_or_stdout` still fails, and restore. The skip +narrows what gets redacted, so re-proving the guarantee still bites is the point. + +- [ ] **Step 5: Confirm the live end-to-end run now produces proposals** + +```bash +docker compose -f tests/integration/docker-compose.yml up -d +uv run pytest -m integration -v +docker compose -f tests/integration/docker-compose.yml down +``` + +The seeded workload's only non-noise statement is the one this bug was dropping, so before +the fix the live run analysed it and produced **zero** proposals. It should now produce at +least one. Report the proposal codes. If it still produces none, the fix is incomplete and +something else is dropping the query — say so rather than adjusting an assertion. + +- [ ] **Step 6: Commit** + +```bash +git add src/sqlquality/workload/fingerprint.py tests/test_workload_fingerprint.py +git commit -m "fix(workload): stop redaction dismembering Postgres placeholders" +``` + +--- + ## Self-Review **Coverage of the recorded items.** Every Batch-1 item from the ledger maps to a task: `secrets.py` extraction → Task 1; expression-index blindness → Tasks 2 and 3; ADV003 partial-predicate blindness → Task 4; `fingerprints` redundancy and `star_tables` regex churn → Task 5; integration test → Tasks 6 and 7; the two README limitations that those fixes narrow → Task 7. From 52b57e28b712ddab81e5549066fcc68cefb26fcb Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Mon, 27 Jul 2026 13:56:03 +0200 Subject: [PATCH 21/27] fix(workload): stop redaction dismembering Postgres placeholders --- src/sqlquality/workload/fingerprint.py | 16 ++++++++++ tests/test_workload_fingerprint.py | 44 ++++++++++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/src/sqlquality/workload/fingerprint.py b/src/sqlquality/workload/fingerprint.py index 76d4cf6..5c762d0 100644 --- a/src/sqlquality/workload/fingerprint.py +++ b/src/sqlquality/workload/fingerprint.py @@ -66,10 +66,26 @@ def literal_flags(tree: exp.Expression) -> frozenset[str]: return frozenset(flags) +def _inside_placeholder(node: exp.Expression) -> bool: + """True if ``node`` sits inside a `$N` parameter marker. + + `pg_stat_statements` has already replaced the literal that was there; the integer left + behind is Postgres's own index, not user data, and rewriting it corrupts the statement. + """ + parent = node.parent + while parent is not None: + if isinstance(parent, exp.Parameter): + return True + parent = parent.parent + return False + + def redact_tree(tree: exp.Expression) -> exp.Expression: """Return a copy of ``tree`` with every literal replaced by a bind placeholder.""" copy = tree.copy() for literal in list(copy.find_all(exp.Literal)): + if _inside_placeholder(literal): + continue literal.replace(exp.Placeholder()) return copy diff --git a/tests/test_workload_fingerprint.py b/tests/test_workload_fingerprint.py index 5a59e62..d8b2f82 100644 --- a/tests/test_workload_fingerprint.py +++ b/tests/test_workload_fingerprint.py @@ -1,6 +1,8 @@ import sqlglot +from sqlglot import exp from sqlquality.models import RawQueryRow, WorkloadFetch +from sqlquality.sqlast import parse from sqlquality.workload.fingerprint import ( FLAG_LEADING_WILDCARD_LIKE, FLAG_SELECT_STAR, @@ -154,3 +156,45 @@ def test_ingest_stats_are_sorted_by_cost_descending(): ) workload = ingest(fetch, "postgres") assert [s.total_time_ms for s in workload.stats] == [99.0, 1.0] + + +def test_redaction_leaves_a_postgres_placeholder_intact(): + """`$N` parses as Parameter(this=Literal(N)), so a naive literal walk mangles it. + + pg_stat_statements hands us `$1`; redacting the `1` inside it produced `$%s`. + """ + tree = parse("select id from orders where status = $1", "postgres") + assert "$1" in redact_tree(tree).sql("postgres") + + +def test_redaction_does_not_dismember_a_normalised_interval(): + """The shape that was silently dropped, using the real text a live server produced. + + Redacting the literal inside `interval $2` left `INTERVAL` as a bare dangling token. + Re-parsed, sqlglot read it as a *column* named INTERVAL, which failed to qualify — so + the whole group vanished into skipped_unqualifiable. `created_at > now() - interval + '1 day'` is the most ordinary filter shape there is. + + Note: sqlglot's postgres generator renders a unitless `INTERVAL $2` as `INTERVAL '2'` + (it reads the parameter's `.name` into its single-string interval form) even for the + *untouched* tree, before `redact_tree` ever runs — so the literal text `$2` does not + survive rendering regardless of this fix. What the fix guarantees, and what actually + caused the query group to vanish, is that the round trip stays parseable and + `created_at` keeps being read as a column rather than growing a bogus `INTERVAL` + sibling. That is what this test proves instead. + """ + sql = "select id from orders where status = $1 and created_at > now() - interval $2" + redacted = redact_tree(parse(sql, "postgres")).sql("postgres") + assert not redacted.rstrip().endswith("INTERVAL") + reparsed = parse(redacted, "postgres") + columns = {c.name.upper() for c in reparsed.find_all(exp.Column)} + assert "INTERVAL" not in columns + assert "CREATED_AT" in columns + + +def test_redaction_still_erases_a_real_literal_beside_a_placeholder(): + """The control. Skipping placeholders must not smuggle a genuine literal through.""" + sql = "select id from orders where status = 'secret-value' and n > $1" + redacted = redact_tree(parse(sql, "postgres")).sql("postgres") + assert "secret-value" not in redacted + assert "$1" in redacted From 1a614aee7e68f320d7ec722cc816e2a88d68b3f7 Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Mon, 27 Jul 2026 14:24:29 +0200 Subject: [PATCH 22/27] fix(advise): the window said "since stats reset at None" `reset[0][0] if reset and reset[0] else "an unknown time"` guarded the wrong thing. `pg_stat_database.stats_reset` is SQL NULL until someone resets statistics -- the default state of any database -- and the row is then `(None,)`: non-empty, so truthy, so the fallback was unreachable and the window line read "since stats reset at None". It only ever fired when the statement was denied. That line is the sole statement of what period the advice covers. ADV002's own rationale tells the operator to "Verify the reset time covers a full business cycle before dropping", and the README calls it "the only way to know which you have" -- both pointing at a field reading None. Verified live against postgres:16, whose fresh container reports stats_reset IS NULL: the terminal now prints `window: since stats reset at an unknown time`. The guard tests the value's nullness and keeps the row-emptiness check, because a denied grant must still cost one capability rather than raising IndexError (invariant 4) -- both paths now have a test. Tightens the live workload test in the same change: `"since stats reset at" in description` is satisfied by the broken string, since the prefix is boilerplate and the payload is the suffix. It passed green while producing exactly the bug it was meant to catch. It now asserts the description contains no "None". Co-Authored-By: Claude Opus 5 --- src/sqlquality/workload/postgres.py | 9 ++++- tests/integration/test_introspection_live.py | 9 +++++ tests/test_workload_postgres.py | 36 ++++++++++++++++++++ 3 files changed, 53 insertions(+), 1 deletion(-) diff --git a/src/sqlquality/workload/postgres.py b/src/sqlquality/workload/postgres.py index 975e1cc..c6feffd 100644 --- a/src/sqlquality/workload/postgres.py +++ b/src/sqlquality/workload/postgres.py @@ -926,7 +926,14 @@ def query(sql: str, bind: tuple[object, ...]) -> list[tuple[object, ...]]: def fetch_workload(self, since: timedelta | None, limit: int) -> WorkloadFetch: rows = self._run(CAP_WORKLOAD, (limit,)) reset = self._run(CAP_STATS_RESET, ()) - reset_at = reset[0][0] if reset and reset[0] else "an unknown time" + # Two different unknowns, one fallback. The statement can be denied (no row at all), + # or it can succeed and report SQL NULL — which is the *default* state of + # `pg_stat_database.stats_reset` for any database whose statistics have never been + # reset. The row is then `(None,)`: non-empty, hence truthy, so testing the row's + # emptiness printed "since stats reset at None". The value's nullness is what matters. + reset_at: object = "an unknown time" + if reset and reset[0] and reset[0][0] is not None: + reset_at = reset[0][0] # pg_stat_statements is cumulative since reset and carries no per-statement # timestamps before PG 17, so --since cannot be honored. Say so rather than # implying the requested window was applied. diff --git a/tests/integration/test_introspection_live.py b/tests/integration/test_introspection_live.py index 4434933..b432d37 100644 --- a/tests/integration/test_introspection_live.py +++ b/tests/integration/test_introspection_live.py @@ -32,8 +32,17 @@ def test_every_introspection_statement_executes(adapter, seeded): def test_workload_statement_returns_our_own_queries(adapter): + """The window line must name a real time, not merely start with the right prefix. + + `"since stats reset at" in ...` was satisfied by the broken `"since stats reset at + None"` — the prefix is the boilerplate, the payload is the suffix. The `seeded` fixture + calls `pg_stat_statements_reset()`, but `pg_stat_database.stats_reset` is a *separate* + counter that a fresh container has never reset, so this assertion is precisely where a + NULL surfaces live. + """ fetch = adapter.fetch_workload(None, 500) assert fetch.rows, "pg_stat_statements returned nothing" + assert "None" not in fetch.window_description, fetch.window_description assert "since stats reset at" in fetch.window_description diff --git a/tests/test_workload_postgres.py b/tests/test_workload_postgres.py index 2812ea6..7f5bdf6 100644 --- a/tests/test_workload_postgres.py +++ b/tests/test_workload_postgres.py @@ -157,6 +157,42 @@ def test_fetch_workload_window_is_honest_that_since_is_not_supported(): assert "since stats reset" in fetch.window_description.lower() +def test_a_null_stats_reset_reads_as_an_unknown_time_not_as_None(): + """`stats_reset` is SQL NULL until someone resets statistics — the *default* state. + + The row is then `(None,)`: non-empty, so truthy, so a guard testing the row's emptiness + lets the None straight through and the window line reads "since stats reset at None". + That line is the sole statement of what period the advice covers, and ADV002's rationale + tells the operator to check it before dropping an index. + """ + querier = FakeQuerier( + { + "pg_stat_statements": [("select id from orders where status = $1", 10, 250.0, 100)], + "pg_stat_database": [(None,)], + } + ) + fetch = PostgresWorkloadAdapter(querier=querier).fetch_workload(None, 500) + assert "an unknown time" in fetch.window_description + assert "None" not in fetch.window_description + + +def test_a_denied_stats_reset_statement_also_reads_as_an_unknown_time(): + """The control: the empty-row path must keep working once the guard tests the value. + + Written as a pair with the test above because the obvious fix — `reset[0][0] is not + None` — reads element 0 of a row that may not exist, and an IndexError on a denied + grant would cost the whole run for a missing privilege (invariant 4). + """ + querier = FakeQuerier( + {"pg_stat_statements": [("select id from orders where status = $1", 10, 250.0, 100)]}, + fail_markers=("pg_stat_database",), + ) + adapter = PostgresWorkloadAdapter(querier=querier) + fetch = adapter.fetch_workload(None, 500) + assert "an unknown time" in fetch.window_description + assert any(cap == CAP_STATS_RESET for cap, _ in adapter.degraded) + + def test_fetch_schema_builds_a_sqlglot_schema_mapping(): querier = FakeQuerier( { From 3045ba9d2677ff09e245dd3bded09e85075a7e1f Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Mon, 27 Jul 2026 14:25:29 +0200 Subject: [PATCH 23/27] test(advise): pin the has_expressions half of _covered The branch's headline claim is that a partial or expression index no longer counts as coverage. Half of it was never executed. Every expression-index test builds `PgIndex(..., columns=())`, where `_is_prefix(candidate, ())` is already False -- so `or index.has_expressions` can never be why those tests pass, and deleting it left all 437 green. The shape that needs the guard is ordinary and nothing built it: CREATE INDEX idx_mixed ON orders (lower(note), status) `indkey` is `[0, status_attnum]`; the expression position at ordinality 1 yields a NULL attname and is dropped, so the tuple we reconstruct is `("status",)` -- position 1 is lost. `_is_prefix(("status",), ("status",))` is True, the index falsely reads as coverage, and a genuine HIGH-confidence ADV001 on orders(status) is silently withheld, even though the real index leads with `lower(note)` and cannot serve a bare `status` lookup. Withholding a correct HIGH proposal is the failure mode this command exists to avoid. No production change -- the guard was already right. Mutation-verified: with `or index.has_expressions` deleted the new test is the only failure in the suite (`assert [] == ['ADV001']`), which is also independent confirmation that nothing else covered it. Co-Authored-By: Claude Opus 5 --- tests/test_workload_rules.py | 43 ++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/tests/test_workload_rules.py b/tests/test_workload_rules.py index 505ce91..55f14b5 100644 --- a/tests/test_workload_rules.py +++ b/tests/test_workload_rules.py @@ -219,6 +219,49 @@ def test_an_expression_index_is_disclosed_not_silently_ignored(): assert "expression" in proposals[0].rationale.lower() +def test_a_mixed_expression_index_sharing_a_prefix_does_not_count_as_coverage(): + """The `has_expressions` half of `_covered`, which every other test leaves unexercised. + + Those tests all build `columns=()`, where `_is_prefix(candidate, ())` is already False — + so the guard can never be the reason they pass, and deleting `or index.has_expressions` + left the whole suite green. This is the shape that needs it, and it is ordinary: + + CREATE INDEX idx_mixed ON orders (lower(note), status) + + `indkey` is `[0, status_attnum]`; the expression position at ordinality 1 yields a NULL + attname and is dropped, so the tuple sqlquality reconstructs is `("status",)` — position + 1 is *lost*. `_is_prefix(("status",), ("status",))` is then True and the index reads as + coverage, silently withholding a genuine HIGH-confidence ADV001. The real index leads + with `lower(note)` and cannot serve a bare `status` lookup. + """ + existing = { + "orders": ( + PgIndex( + "idx_mixed", + # Non-empty on purpose: the reconstructed tuple from a mixed index, with the + # leading expression position missing. This is what the catalog query yields. + ("status",), + False, + False, + 5, + 4096, + has_expressions=True, + definition="CREATE INDEX idx_mixed ON orders (lower(note), status)", + ), + ) + } + proposals = propose_indexes( + [usage("status", ColumnRole.EQUALITY)], + facts(ndv={"status": 500.0}), + existing, + min_cost_share=0.01, + ) + assert codes(proposals) == ["ADV001"] + assert proposals[0].confidence is Confidence.HIGH + assert proposals[0].evidence["expression_indexes"] == ("idx_mixed",) + assert "expression index" in proposals[0].rationale.lower() + + def test_an_expression_index_not_mentioning_the_column_is_not_disclosed(): """Only expression indexes that plausibly relate to the candidate are worth naming.""" existing = { From 5cad383d34cf2755e3491fe58ef6658fe03d3dd2 Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Mon, 27 Jul 2026 14:27:17 +0200 Subject: [PATCH 24/27] test(advise): guard invariant 2 -- read-only *and* the statement timeout Invariant 2 says connect() arms `default_transaction_read_only` and a statement timeout before the session is usable. Neither half had a unit guard. Removing `SET default_transaction_read_only = on` left the default suite green; removing the `set_config('statement_timeout', ...)` call left it green too. The read-only half was caught only by an integration test that is deselected by default and needs Docker; the timeout was asserted nowhere, unit or live. An unbounded statement timeout is the more dangerous of the two: a catalog query can pin a production server, which is the opposite of the "safe to point at production" promise the whole command rests on. The machinery was already there -- `_FakeCursor.executed` existed and nothing in tests/ ever read it. `_FakeCursor` now also appends to a connection-wide transcript, because per-cursor records cannot express "before the querier is usable": a read-only setting applied after the first query would protect nothing, and only the relative order distinguishes the two. Mutation-verified, each statement removed in turn. Both mutations fail only the two new tests and nothing else in the suite, which is itself the confirmation that nothing covered them before. Co-Authored-By: Claude Opus 5 --- tests/test_workload_postgres.py | 68 +++++++++++++++++++++++++++++++-- 1 file changed, 64 insertions(+), 4 deletions(-) diff --git a/tests/test_workload_postgres.py b/tests/test_workload_postgres.py index 7f5bdf6..345afd1 100644 --- a/tests/test_workload_postgres.py +++ b/tests/test_workload_postgres.py @@ -5,6 +5,7 @@ from sqlquality.models import ConnectionParams from sqlquality.workload import get_workload_adapter +from sqlquality.workload.base import MAX_TIMEOUT_S from sqlquality.workload.postgres import ( CAP_INDEXES, CAP_NDV, @@ -474,10 +475,17 @@ def test_the_denial_fixture_would_otherwise_have_returned_statistics(): class _FakeCursor: - """Enough of a psycopg cursor for connect()'s two session-setup statements.""" + """Enough of a psycopg cursor for connect()'s two session-setup statements. - def __init__(self) -> None: + `executed` records this cursor's own statements; `log` is the connection-wide + transcript, so the *relative* order of session setup and later queries is observable. + Ordering across cursors is the whole point of invariant 2 — a read-only setting applied + after the first query would be no protection at all. + """ + + def __init__(self, log: list[tuple] | None = None) -> None: self.executed: list[tuple] = [] + self._log = log if log is not None else [] def __enter__(self): return self @@ -487,6 +495,7 @@ def __exit__(self, *exc): def execute(self, sql, params=None): self.executed.append((sql, params)) + self._log.append((sql, params)) def fetchall(self): return [] @@ -495,9 +504,10 @@ def fetchall(self): class _FakeConnection: def __init__(self) -> None: self.cursors: list[_FakeCursor] = [] + self.log: list[tuple] = [] def cursor(self): - cursor = _FakeCursor() + cursor = _FakeCursor(self.log) self.cursors.append(cursor) return cursor @@ -511,7 +521,8 @@ def _install_fake_psycopg(monkeypatch, seen: dict): def connect(conninfo, **kwargs): seen["conninfo"] = conninfo - return _FakeConnection() + seen["connection"] = _FakeConnection() + return seen["connection"] module.connect = connect # type: ignore[attr-defined] module.conninfo = types.SimpleNamespace( # type: ignore[attr-defined] @@ -591,6 +602,55 @@ def test_forwarded_and_mapped_keys_are_not_reported_as_dropped(monkeypatch, caps assert capsys.readouterr().err == "" +def test_connect_arms_read_only_and_a_statement_timeout_before_the_querier_is_usable( + monkeypatch, +): + """Invariant 2, at unit level: neither session-setup statement had a unit guard. + + Removing `SET default_transaction_read_only = on` left the whole default suite green — + the read-only claim rested solely on an integration test that is deselected by default + and needs Docker. The statement timeout was asserted nowhere at all, unit or live: a + session with no timeout can pin a production server on a catalog query, which is the + opposite of the "safe to point at production" promise. + + `before the querier is usable` is asserted against the connection-wide transcript, not + just per-cursor: setup applied after the first query would protect nothing, and only the + relative order can tell the difference. + """ + seen: dict = {} + _install_fake_psycopg(monkeypatch, seen) + adapter = PostgresWorkloadAdapter() + adapter.connect( + ConnectionParams(engine="postgres", dsn="postgresql:///x", fields={}, source="--dsn"), 30 + ) + + setup = seen["connection"].log[:] + assert setup == [ + ("SET default_transaction_read_only = on", None), + ("SELECT set_config('statement_timeout', %s, false)", ("30000ms",)), + ], setup + + # Usable only now, and every later statement lands after both setup statements. + adapter._query("SELECT 1", ()) + assert seen["connection"].log[:2] == setup + assert seen["connection"].log[2] == ("SELECT 1", ()) + + +def test_an_out_of_range_timeout_is_clamped_before_it_reaches_the_session(monkeypatch): + """The value is `clamp_timeout_ms`'s output in milliseconds, not the raw seconds. + + Passing `7200` through unclamped would arm a two-hour statement timeout, and passing it + as `7200` rather than `7200ms` would be read by Postgres as milliseconds — a 7-second + ceiling. Both are silent. + """ + seen: dict = {} + _install_fake_psycopg(monkeypatch, seen) + PostgresWorkloadAdapter().connect( + ConnectionParams(engine="postgres", dsn="postgresql:///x", fields={}, source="--dsn"), 7200 + ) + assert seen["connection"].log[1][1] == (f"{MAX_TIMEOUT_S * 1000}ms",) + + def test_a_conninfo_build_failure_is_scrubbed_like_a_connect_failure(monkeypatch): """make_conninfo ran outside the scrubbing envelope, so its message was unfiltered. From 9e8abc8c9ef76a3f46e70e972c8784baa03a08da Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Mon, 27 Jul 2026 14:28:35 +0200 Subject: [PATCH 25/27] test(integration): stop the psycopg guard skipping the default suite Invariant 7 says the default `uv run pytest` is green with no skips or errors for a contributor without Docker. It was not. `pytest.importorskip("psycopg")` sat at conftest *module* scope, and conftest import happens during collection -- before `addopts = "-m 'not integration'"` deselects anything. Without the optional `postgres` extra the entire package collapsed into one collection-level skip: `442 passed, 1 skipped` where the conftest docstring promises `deselected`. This is the documented path, not an exotic one: CONTRIBUTING.md tells a contributor to run plain `uv sync`, which installs the runtime deps and the `dev` group -- and psycopg is in neither. The guard moves into `live_dsn`, which is reached only once a test has already been selected, and its reason now names the command that fixes it. Verified in a fresh `uv sync` venv with psycopg absent: `442 passed, 8 deselected`, and `-m integration` there skips with an actionable message instead of erroring. Co-Authored-By: Claude Opus 5 --- tests/integration/conftest.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 078c0cc..75f6892 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -3,6 +3,14 @@ Every test in this package is marked `integration` and deselected by default (see pyproject.toml's addopts), so a contributor without Docker sees a clean `uv run pytest`. +Nothing here may fail, skip, or import psycopg at *module* scope. Conftest import happens +during collection, before `addopts` deselects anything, so a module-scope +`pytest.importorskip("psycopg")` turned the whole package into one collection-level skip for +anyone who ran the plain `uv sync` CONTRIBUTING.md documents — psycopg is an optional extra, +not part of the `dev` group. The result was `442 passed, 1 skipped` where the promise above +says `deselected`. The guard belongs in `live_dsn`, which is reached only once a test has +already been selected. + Bring the server up with: docker compose -f tests/integration/docker-compose.yml up -d uv run pytest -m integration @@ -15,8 +23,6 @@ import pytest -pytest.importorskip("psycopg", reason="integration tests need the postgres extra") - DEFAULT_DSN = "postgresql://postgres:sqlquality@127.0.0.1:55432/sqlquality_test" _PACKAGE_DIR = Path(__file__).parent @@ -38,7 +44,9 @@ def pytest_collection_modifyitems(items: list[pytest.Item]) -> None: @pytest.fixture(scope="session") def live_dsn() -> str: """A reachable Postgres, or skip with an actionable message.""" - import psycopg + psycopg = pytest.importorskip( + "psycopg", reason="integration tests need the postgres extra: uv sync --extra postgres" + ) dsn = os.environ.get("SQLQUALITY_TEST_DSN", DEFAULT_DSN) try: From 75181a1ac70f296606db63d6e518a96bbe64cae5 Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Mon, 27 Jul 2026 14:29:33 +0200 Subject: [PATCH 26/27] refactor(advise): delete the unreachable dedupe tie-break, and the false claim `_RULE_PRECEDENCE` is dead. Only ADV002 and ADV003 ever emit the same DDL string; ADV002 is hardcoded MEDIUM and ADV003 hardcoded HIGH, so the confidence element alone decides and the second tuple element is never consulted. Setting `_RULE_PRECEDENCE = {}` left all tests green. Its docstring had also gone false. It still explained the tie-break as necessary "capping ADV003 at MEDIUM (it cannot see partial-index predicates)" -- but Task 4 restored HIGH precisely because Task 2 gave the rule that visibility. A tie-break that cannot be reached, justified by a constraint that no longer holds, is worse than none: it reads as evidence the collision is handled where the confidence values are what actually handle it. `_dedupe_by_ddl` now compares `_CONFIDENCE_ORDER` directly and the docstring states what is true, including why the tie disappeared. Behaviour unchanged: the existing dedupe test still shows ADV003 surviving, now on confidence alone, and inverting the comparison still fails it (`assert 'ADV002' == 'ADV003'`), so the collapse is still pinned in the direction that matters. Co-Authored-By: Claude Opus 5 --- src/sqlquality/workload/postgres.py | 43 ++++++++++++++--------------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/src/sqlquality/workload/postgres.py b/src/sqlquality/workload/postgres.py index c6feffd..75af292 100644 --- a/src/sqlquality/workload/postgres.py +++ b/src/sqlquality/workload/postgres.py @@ -1078,37 +1078,36 @@ def _dedupe_by_ddl(cls, proposals: list[Proposal]) -> list[Proposal]: An index with no recorded scans that is *also* a prefix of a wider index gets flagged by both ADV002 and ADV003, producing two entries with the same `DROP INDEX`. They do not contradict each other, but a reader should not have to - notice they are the same object twice. ADV003 wins ties because prefix redundancy - is structural — provable from the column lists alone — whereas ADV002 rests on a - scan counter that only covers the window since the last statistics reset. - - That preference used to fall out of the confidence order on its own, when ADV003 - was HIGH. Capping ADV003 at MEDIUM (it cannot see partial-index predicates) made - the two rules tie, and a tie was resolved by list order — silently handing the - collapse to ADV002. `_RULE_PRECEDENCE` states the preference instead of relying on - it emerging. + notice they are the same object twice. ADV003 is the one kept, because prefix + redundancy is structural — provable from the column lists alone — whereas ADV002 + rests on a scan counter that only covers the window since the last statistics reset. + + That preference needs no tie-break rule to state it: the two codes cannot tie. + ADV002 is hardcoded MEDIUM — `idx_scan` accumulates only since the last statistics + reset, so zero scans can never prove disuse across a business cycle — and ADV003 is + hardcoded HIGH, because it can now read `indpred` and so declines to call a partial + index redundant rather than guessing. Confidence alone decides, and it decides the + way this docstring says it should. + + There was a window where they *did* tie, and it is why a second tuple element used + to be here: ADV003 was briefly capped at MEDIUM on the grounds that it could not see + partial-index predicates. It can, so the cap is gone and so is the tie. A tie-break + that cannot be reached is worse than none — it reads as evidence the collision is + handled where the confidence values are what actually handle it. """ best: dict[str, Proposal] = {} for proposal in proposals: if not proposal.ddl: continue incumbent = best.get(proposal.ddl) - if incumbent is None or cls._dedupe_rank(proposal) < cls._dedupe_rank(incumbent): + if ( + incumbent is None + or cls._CONFIDENCE_ORDER[proposal.confidence] + < cls._CONFIDENCE_ORDER[incumbent.confidence] + ): best[proposal.ddl] = proposal return [p for p in proposals if not p.ddl or best[p.ddl] is p] - #: Lower wins when two rules propose identical DDL at the same confidence. Only ADV003 - #: is named: its evidence is structural, every other rule's rests on a counter or an - #: estimate. Anything unlisted sorts after it. - _RULE_PRECEDENCE = {"ADV003": 0} - - @classmethod - def _dedupe_rank(cls, proposal: Proposal) -> tuple[int, int]: - return ( - cls._CONFIDENCE_ORDER[proposal.confidence], - cls._RULE_PRECEDENCE.get(proposal.code, 1), - ) - def propose( self, aggregation: Aggregation, From 39ff2d81a8bae4f9d454c6ecb3c12c09d6035260 Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Mon, 27 Jul 2026 14:36:50 +0200 Subject: [PATCH 27/27] docs(advise): qualify --min-cost-share, and warn about INTERVAL '2' Two promises the tool cannot keep as written. Documentation only -- no behaviour change in either case, deliberately. --min-cost-share said "Suppress proposals below this share of workload cost", unqualified, in both the flag help and the README table. Neither propose_unused_indexes nor propose_redundant_indexes takes the parameter: index hygiene is read out of the catalog and has no cost evidence to weigh. Against a live server `--min-cost-share 5` -- an impossible threshold -- still returned two proposals. Both places now name the cost-weighted rules the threshold reaches (ADV001, ADV004, ADV005, ADV006) and say ADV002/ADV003 are always reported. Filtering them on a share they do not have would be inventing evidence. Second, sqlglot's postgres generator renders `interval $2` as `INTERVAL '2'`, so a report stamped `"redacted": true` shows `created_at > CURRENT_TIMESTAMP - INTERVAL '2'` where the user wrote `interval '1 day'`. Nothing leaked -- the 2 is Postgres's own parameter index -- but it reads as a retained literal and is not valid SQL to copy out and run. The quirk was noted in a docstring; nothing an operator reads mentioned it. It now sits in the README's advise section beside the data-protection paragraph, which is where someone doubting a redacted report will look. Not fixed in sqlglot's rendering, by design. Co-Authored-By: Claude Opus 5 --- README.md | 9 ++++++++- src/sqlquality/cli.py | 14 +++++++++++++- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 2a6be5b..9e26d80 100644 --- a/README.md +++ b/README.md @@ -315,7 +315,7 @@ missing driver degrades with an install hint instead of a traceback. | `--schema` | `public` | Schema to introspect. **One at a time** — passing two exits 2, see Limitations. | | `--since` | — | Window, e.g. `7d`. **Not honored on Postgres** — see Prerequisites below. | | `--limit` | `500` | Max query-history rows to read. | -| `--min-cost-share` | `0.01` | Suppress proposals below this share of workload cost. | +| `--min-cost-share` | `0.01` | Suppress proposals below this share of workload cost. Applies to the **cost-weighted** rules (ADV001, ADV004, ADV005, ADV006); the index-hygiene rules **ADV002 and ADV003 carry no cost evidence and are always reported**, whatever the threshold. | | `--keep-literals` | off | Do **not** redact literal values from query text. | | `--timeout` | `30` | Statement timeout in seconds (rejected outside 1–3600). | | `--dry-run` | off | Print every statement the adapter would issue, then exit 0 **without connecting**. | @@ -355,6 +355,13 @@ only way to retain them, and the report states which mode produced it. `advise` writes to your database — proposed DDL only ever goes to a file (`--ddl`) for you to review and apply by hand. +One rendering quirk worth knowing before you read a report: `pg_stat_statements` replaces +an interval literal with its own parameter marker (`interval $2`), and sqlglot renders that +back as `INTERVAL '2'`. So `created_at > CURRENT_TIMESTAMP - INTERVAL '2'` in a report +stamped `"redacted": true` means **the interval was parameterised**, not that someone wrote +a two-something interval — the `2` is Postgres's parameter index. Nothing leaked, but the +statement is not valid SQL to copy out and run. + **Prerequisites and limits:** - **`pg_stat_statements`** must be installed (`shared_preload_libraries` + diff --git a/src/sqlquality/cli.py b/src/sqlquality/cli.py index c5dd76b..1ca3a7e 100644 --- a/src/sqlquality/cli.py +++ b/src/sqlquality/cli.py @@ -720,7 +720,19 @@ def advise( ), limit: int = typer.Option(500, "--limit", help="Max query-history rows to read."), min_cost_share: float = typer.Option( - 0.01, "--min-cost-share", help="Suppress proposals below this share of workload cost." + 0.01, + "--min-cost-share", + # The unqualified "suppress proposals below this share" was a promise the flag + # cannot keep: propose_unused_indexes and propose_redundant_indexes do not take the + # parameter, because index hygiene is read out of the catalog and has no cost + # evidence to weigh. --min-cost-share 5 -- an impossible threshold -- still returned + # both. Naming the rules it does not reach is the honest fix; filtering them on a + # share they do not have would be inventing evidence. + help=( + "Suppress proposals below this share of workload cost. Applies to the " + "cost-weighted rules (ADV001, ADV004, ADV005, ADV006); the index-hygiene rules " + "ADV002 and ADV003 carry no cost evidence and are always reported." + ), ), keep_literals: bool = typer.Option( False, "--keep-literals", help="Do NOT redact literal values from query text."