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: 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/README.md b/README.md index 03eee29..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` + @@ -382,11 +389,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 +784,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/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..87f2051 --- /dev/null +++ b/docs/superpowers/plans/2026-07-27-advise-postgres-hardening.md @@ -0,0 +1,1610 @@ +# 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=MIN_TIMEOUT_S, " + f"maximum=MAX_TIMEOUT_S)}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"] == () + 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** + +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`: + +```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. + # + # 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 mentions_identifier(columns[0], 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" + # 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(): + """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_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_narrow_expr ON orders (status, lower(note))"), + PgIndex("idx_wide", ("status", "created_at"), False, False, 5, 1), + )} + 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: 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** + +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. + +**`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 +"""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 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: + +```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_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" + 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") + # 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): + """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" +``` + +--- + +### 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. + +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 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. +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. 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/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." 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 1854f0b..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,15 +14,34 @@ _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. +@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") + - A plain `name in sql` test would false-positive three ways: a table `order` inside a +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 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 _identifier_pattern(name).search(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]: @@ -49,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) @@ -66,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]) @@ -87,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/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/src/sqlquality/workload/postgres.py b/src/sqlquality/workload/postgres.py index 8b7ab58..75af292 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, @@ -22,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, @@ -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 @@ -199,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. @@ -212,6 +163,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) @@ -226,6 +181,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. @@ -264,8 +229,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 @@ -356,6 +331,29 @@ 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. + # + # 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 mentions_identifier(columns[0], 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: @@ -388,6 +386,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( @@ -403,6 +413,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=( @@ -459,22 +471,31 @@ 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``, 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) ), @@ -487,12 +508,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, @@ -502,7 +521,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)};", ) ) @@ -787,21 +806,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 @@ -857,7 +884,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 +901,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 @@ -897,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. @@ -930,7 +966,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)) @@ -978,7 +1014,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( @@ -986,13 +1035,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(): @@ -1004,6 +1060,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()} @@ -1018,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, 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/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..75f6892 --- /dev/null +++ b/tests/integration/conftest.py @@ -0,0 +1,111 @@ +"""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`. + +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 +""" + +from __future__ import annotations + +import os +from pathlib import Path + +import pytest + +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.""" + 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: + 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_advise_live.py b/tests/integration/test_advise_live.py new file mode 100644 index 0000000..eccc4c7 --- /dev/null +++ b/tests/integration/test_advise_live.py @@ -0,0 +1,91 @@ +"""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_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" + 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") + # 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): + """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 diff --git a/tests/integration/test_introspection_live.py b/tests/integration/test_introspection_live.py new file mode 100644 index 0000000..b432d37 --- /dev/null +++ b/tests/integration/test_introspection_live.py @@ -0,0 +1,82 @@ +"""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): + """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 + + +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)", ()) 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_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_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 diff --git a/tests/test_workload_postgres.py b/tests/test_workload_postgres.py index 1453123..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, @@ -13,9 +14,6 @@ CAP_TABLE_FACTS, CAP_WORKLOAD, PostgresWorkloadAdapter, - _scrub, - _secrets_for, - _WITHHELD, ) EXPECTED_CAPABILITIES = { @@ -160,6 +158,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( { @@ -229,6 +263,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. @@ -239,8 +306,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)", + ), ] } ) @@ -285,66 +378,52 @@ 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( { "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)", + ), ] } ) @@ -396,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 @@ -409,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 [] @@ -417,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 @@ -433,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] @@ -513,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. @@ -636,3 +774,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" + ) diff --git a/tests/test_workload_rules.py b/tests/test_workload_rules.py index c28959f..55f14b5 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), ) @@ -158,6 +157,185 @@ 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_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 = { + "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"] == () + 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(): """The interaction of the two most important ordering rules, previously untested. @@ -378,7 +556,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), @@ -387,28 +565,103 @@ 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 + # 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_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_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_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_narrow_expr ON orders (status, lower(note))", + ), 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_unique_prefix_index_is_never_called_redundant(): 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