feat(advise): harden the Postgres adapter, and put a real Postgres in the loop - #11
Merged
Conversation
Tags are 0.3.0, not v0.3.0. The workflow only triggered on "v*", so adopting that convention would have meant pushing 0.3.0 and getting no release -- silently, since a tag matching no filter produces no workflow run at all. That reads as a slow release rather than one that never started. Adds a bare-semver glob and keeps "v*" as a safety net, because tag filters are globs rather than regexes and cannot be made to reject a prefix: matching both means an out-of-habit v0.3.0 still publishes instead of vanishing. Drop the v* line to make that mistake fail loudly instead. Verified the globs against 0.3.0, 0.10.2, 1.0.0, v0.3.0, main and release-0.3.0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Seven TDD tasks covering the Batch-1 follow-ups recorded during the advise plan's fifteen task reviews and its final whole-branch review: - Extract engine-neutral credential handling into workload/secrets.py, before a second adapter re-derives scrubbing by hand. That code took three fix rounds to get right (driver text quoting the value, `from None` suppressing only the traceback, urlparse returning a still-encoded DSN password) and the final reviewer named this the seam that matters. - Stop CAP_INDEXES discarding expression columns. Postgres stores 0 in indkey for an expression position and no pg_attribute row has attnum 0, so the inner join meant an index on lower(status) arrived with an EMPTY column tuple. LEFT JOIN plus indpred/indexprs/pg_get_indexdef fixes it. - Consequently: a partial index stops being treated as coverage (it does not serve an unfiltered lookup, so calling it coverage silently withheld real proposals), expression indexes get disclosed rather than ignored, and ADV003 earns HIGH back for genuinely plain pairs while skipping the rest entirely -- "probably wrong" is not a confidence level. - A real Postgres behind an opt-in marker. Not one introspection statement in this feature has ever executed against a server; they are only diffed for drift, which cannot catch a wrong column name or a missing view. Deselected by default via addopts so `uv run pytest` stays green without Docker. - Two recorded trivia: fingerprints becomes a property over fingerprint_ids (two fields, one fact, kept in step by convention only), and star_tables stops compiling a regex per (stat x table) pair. Batch 2 (join-key and grouping proposals, DECLARE/COPY unwrapping, multi-schema keying) and Batch 3 (Redshift, Snowflake, dbt enrichment) stay in the ledger. Those change what advise says; this batch changes whether it can be trusted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Credential scrubbing and timeout clamping move from postgres.py into a new engine-neutral sqlquality.workload.secrets module (SECRET_FIELDS, MIN_SCRUBBABLE_SECRET, WITHHELD, secrets_for, scrub, clamp_timeout_ms), all now public, so future Redshift/Snowflake adapters cannot bypass the hard-won scrubbing sequence. clamp_timeout_ms takes explicit minimum/maximum keyword args instead of reading module constants.
My Task 1 example hardcoded minimum=1, maximum=3600 at the call site. That would have broken test_the_timeout_bounds_have_a_single_definition -- which asserts 3600 never appears in postgres.py's source -- and reintroduced the duplicated-constants defect the brief's own rationale argues against. MIN_TIMEOUT_S/MAX_TIMEOUT_S already live in workload/base.py. Caught by the Task 1 implementer, which used the constants rather than following the example. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Task 3's review confirmed the substring test states a falsehood to the operator. Reproduced: a candidate on `id` against an index on `lower(guid)` yields the rationale "An expression index (idx_lower_guid) mentions id" -- it does not; "gu-id-" does. That sentence sits in the .sql file someone reads while deciding whether to run DDL, which is exactly where the tool must not assert what it cannot support. It fires for any short column name inside a longer identifier, so id/guid, id/valid, at/created_at are all live. The reviewer verified word boundaries cost no true positive: lower(status), lower(customer_id::text) and (id::text) all still match, because Postgres separates identifiers with parens, commas, dots and ::, none of them word characters. Rather than add a third hand-rolled \b regex -- aggregate.mentions_table and the test suite's _write_verbs_in already exist for this exact class of bug -- the plan now generalises the existing helper to mentions_identifier and has mentions_table delegate to it. Widens Task 3's scope to aggregate.py by two functions, which is cheaper than the duplication. Adds the false-positive case and a cast-form control as tests, plus the rationale-absence assertion the reviewer noted was missing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rings Generalises aggregate.mentions_table into mentions_identifier and reuses it in postgres.py's expression-index disclosure, instead of a third hand-rolled \b regex. A plain substring check reported an index on lower(guid) as mentioning the id column, which is a false statement in the .sql rationale an operator reads before running DDL.
Task 4's review found the committed expression test passes for the wrong reason. Both indexes had one column, so `len(other.columns) > len(narrow.columns)` was already False from the length tie -- the test returns [] whether or not the has_expressions filters exist at all. Ninth test on this project that looked like a guarantee and wasn't; the implementer spotted it and reported it but shipped it unfixed. Replaced with two that discriminate: a strictly wider expression index, and the narrow-side case. The second matters for a reason worth stating -- `columns` understates an expression index, so a narrow one may index something the wider one does not, and dropping it on a column-list comparison would discard an index nothing else provides. Also pins the rationale wording. Deleting the old MEDIUM test (correctly, since its "cannot see a partial predicate" hedge became false) removed the only assertion on this rationale's content, so a future edit could reintroduce a hedge or drop the "both are plain" claim while leaving confidence at HIGH with nothing failing. Verified both assertions hold against the shipped text rather than assuming. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…GH rationale
The old expression test gave both indexes equal-length columns, so the length
guard already returned [] whether or not the has_expressions filters existed.
Replaced with a strictly-wider expression index (proves the wider-side guard)
and a narrow expression index (proves the narrow-side guard, and the reason it
matters: columns understates an expression index, so a narrow one may cover
something the wider one doesn't). Also pinned the HIGH rationale's wording
("plain" present, "partial" absent) now that the old MEDIUM test — the only
thing asserting on it — is gone, and dropped an internal task-number reference
from the docstring.
I predicted the compile-counting test would fail pre-fix with "5 x 21 compiles". It would not: the pre-fix mentions_identifier calls re.search(pattern, text), which resolves through Python's private internal cache and never touches the public re.compile a monkeypatch can observe. The test passes vacuously against the unfixed code -- a green RED, and the tenth test on this project that would have looked like a guarantee without being one. The Task 5 implementer caught this and validated from the other end: implement, confirm green, then pull @lru_cache back off and confirm it fails for the right reason (105 compiles for 21 tables across 5 stats), then restore. The plan now prescribes that direction, because a cache-hit test that cannot fail is worth nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds an opt-in `integration` marker (deselected by default via addopts, so `uv run pytest` stays green without Docker) plus a docker-compose Postgres 16 service with pg_stat_statements preloaded. The new suite runs all six introspection statements against a live server for the first time — the unit suite only ever checked them for drift. Two things a fixture couldn't have caught: pytest does not propagate a conftest.py's module-level `pytestmark` to sibling test files, so `-m integration` was silently selecting nothing until a `pytest_collection_modifyitems` hook applied the marker explicitly; and a freshly loaded table reports `reltuples = -1` and empty pg_stats until analyzed, racing autovacuum, so the seed fixture now runs ANALYZE before handing the schema to a test.
Task 6's first real run against a live Postgres found a production bug no fixture could have shown, which is exactly what the task was added for. pg_class.reltuples is -1 on Postgres 14+ for a table that has never been analyzed -- distinct from 0, which means analyzed and genuinely empty. fetch_table_facts passed it straight through, and propose_indexes' small-table gate then read -1 < 10000 and suppressed every proposal for the table, silently. Measured: reltuples=-1 -> NO PROPOSAL (suppressed) reltuples=0 -> NO PROPOSAL (suppressed) reltuples=None -> low, "row count unknown" reltuples=8000000 -> high The window where this bites is precisely when someone reaches for advise: a freshly loaded or migrated table, before autovacuum's first ANALYZE, with slow queries. They get no advice and no reason for it. Same silent-suppression class as the partial-index coverage bug earlier in this plan. None already means unknown everywhere and that path is correct, so the fix is translating the sentinel at the boundary; everything downstream then behaves. Committed separately from the integration suite, since it is a production fix the suite happened to find. Also corrects two defects in my own Task 6 text that the implementer hit: a module-level pytestmark in conftest.py does not mark sibling test modules (so -m integration selected nothing until a collection hook was added), and the verbatim import list carried six unused CAP_* constants that fail ruff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Postgres 14+ stores -1 in pg_class.reltuples for a table that has never been analyzed, distinct from 0 (analyzed and genuinely empty). Passed through unchanged, fetch_table_facts handed propose_indexes and propose_partial_indexes a row_estimate of -1, which the small-table gate reads as tiny and suppresses every proposal for that table with no message. The window this bites is exactly when someone reaches for `advise`: a freshly loaded or migrated table, before autovacuum's first ANALYZE, with slow queries already running. Found by tests/integration's live run against a real, unanalyzed table (the suite's fixture now runs ANALYZE explicitly to stay deterministic, which is why the unit tests below are what pin this rather than the live suite). None already means "unknown" throughout and proposes at LOW with the gap stated, so the fix is translating the sentinel at the fetch_table_facts boundary via a new _row_estimate() helper. Both downstream rules (ADV001, ADV004) read the same TableFacts.row_estimate field and need no changes.
Task 7's implementer proved my end-to-end redaction test cannot fail. Postgres normalises constants to $N inside pg_stat_statements before sqlquality sees the query text, so 'paid' is already gone on arrival -- confirmed by running the scenario with --keep-literals, which bypasses redact_tree entirely, and still finding no 'paid' anywhere. Eleventh test on this project that looked like a guarantee and wasn't, and this one was mine, in the plan text. Its name and docstring both said "the redaction guarantee, against a real server", which is exactly the sentence a future reader would trust instead of re-deriving. Kept, renamed and re-documented rather than deleted, because it does pin something real: that nothing downstream of ingest -- evidence dicts, rationales, DDL, the renderers -- reintroduces raw query text into an artifact. That is a live risk, since ADV005 and ADV006 both copy SQL into evidence. The docstring now states plainly that it cannot fail if redact_tree breaks, and points at tests/test_workload_redaction.py, which feeds un-normalised literals through a fake querier and does fail under mutation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…arantee Postgres normalises pg_stat_statements query text before we ever see it, so the test cannot fail if redact_tree breaks. Rename it, document what it actually pins (no downstream surface reintroduces raw SQL), and point at tests/test_workload_redaction.py as the real guard on redact_tree.
Task 7's live run found the most impactful defect in this feature, and it took a real server to surface it. pg_stat_statements hands us SQL with literals already replaced by $N markers. A $N parses as Parameter(this=Literal(N)), so redact_tree's literal walk descends INTO the placeholder and rewrites the index. Measured on the real text: in: ... status = $1 AND created_at > now() - interval $2 out: ... status = $%s AND created_at > CURRENT_TIMESTAMP - INTERVAL INTERVAL is left dangling. ingest() stores that as QueryStat.sql, aggregate() re-parses it, and sqlglot reads the trailing INTERVAL as a COLUMN NAME, which fails to qualify -- so the whole query group is dropped into skipped_unqualifiable. Reach: any query where Postgres normalised a literal inside an INTERVAL. `created_at > now() - interval '1 day'` is a time-window filter, the single most ordinary shape in the workloads advise exists for. It has been silently discarding them. Not entirely silent -- the coverage line says "1 unresolvable" -- but nothing tells the user sqlquality's own redaction broke the query it then could not parse. The fix is a skip rather than a repair: a $N is Postgres's own marker standing where a literal already was, so there is nothing in it to redact and descending can only corrupt. Verified the fix preserves $1 and `interval $2` intact while still erasing a real literal beside them. Step 4 re-runs the redaction guarantee's mutation check, because the skip narrows what gets redacted and that property has to be re-proven, not assumed. Step 5 checks the live run now yields proposals -- the seeded workload's only non-noise statement was the one being dropped, so it produced zero. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`reset[0][0] if reset and reset[0] else "an unknown time"` guarded the wrong thing. `pg_stat_database.stats_reset` is SQL NULL until someone resets statistics -- the default state of any database -- and the row is then `(None,)`: non-empty, so truthy, so the fallback was unreachable and the window line read "since stats reset at None". It only ever fired when the statement was denied. That line is the sole statement of what period the advice covers. ADV002's own rationale tells the operator to "Verify the reset time covers a full business cycle before dropping", and the README calls it "the only way to know which you have" -- both pointing at a field reading None. Verified live against postgres:16, whose fresh container reports stats_reset IS NULL: the terminal now prints `window: since stats reset at an unknown time`. The guard tests the value's nullness and keeps the row-emptiness check, because a denied grant must still cost one capability rather than raising IndexError (invariant 4) -- both paths now have a test. Tightens the live workload test in the same change: `"since stats reset at" in description` is satisfied by the broken string, since the prefix is boilerplate and the payload is the suffix. It passed green while producing exactly the bug it was meant to catch. It now asserts the description contains no "None". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The branch's headline claim is that a partial or expression index no longer
counts as coverage. Half of it was never executed. Every expression-index test
builds `PgIndex(..., columns=())`, where `_is_prefix(candidate, ())` is already
False -- so `or index.has_expressions` can never be why those tests pass, and
deleting it left all 437 green.
The shape that needs the guard is ordinary and nothing built it:
CREATE INDEX idx_mixed ON orders (lower(note), status)
`indkey` is `[0, status_attnum]`; the expression position at ordinality 1 yields
a NULL attname and is dropped, so the tuple we reconstruct is `("status",)` --
position 1 is lost. `_is_prefix(("status",), ("status",))` is True, the index
falsely reads as coverage, and a genuine HIGH-confidence ADV001 on
orders(status) is silently withheld, even though the real index leads with
`lower(note)` and cannot serve a bare `status` lookup. Withholding a correct
HIGH proposal is the failure mode this command exists to avoid.
No production change -- the guard was already right. Mutation-verified: with
`or index.has_expressions` deleted the new test is the only failure in the
suite (`assert [] == ['ADV001']`), which is also independent confirmation that
nothing else covered it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Invariant 2 says connect() arms `default_transaction_read_only` and a statement
timeout before the session is usable. Neither half had a unit guard. Removing
`SET default_transaction_read_only = on` left the default suite green; removing
the `set_config('statement_timeout', ...)` call left it green too. The read-only
half was caught only by an integration test that is deselected by default and
needs Docker; the timeout was asserted nowhere, unit or live.
An unbounded statement timeout is the more dangerous of the two: a catalog query
can pin a production server, which is the opposite of the "safe to point at
production" promise the whole command rests on.
The machinery was already there -- `_FakeCursor.executed` existed and nothing in
tests/ ever read it. `_FakeCursor` now also appends to a connection-wide
transcript, because per-cursor records cannot express "before the querier is
usable": a read-only setting applied after the first query would protect
nothing, and only the relative order distinguishes the two.
Mutation-verified, each statement removed in turn. Both mutations fail only the
two new tests and nothing else in the suite, which is itself the confirmation
that nothing covered them before.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Invariant 7 says the default `uv run pytest` is green with no skips or errors for
a contributor without Docker. It was not. `pytest.importorskip("psycopg")` sat at
conftest *module* scope, and conftest import happens during collection -- before
`addopts = "-m 'not integration'"` deselects anything. Without the optional
`postgres` extra the entire package collapsed into one collection-level skip:
`442 passed, 1 skipped` where the conftest docstring promises `deselected`.
This is the documented path, not an exotic one: CONTRIBUTING.md tells a
contributor to run plain `uv sync`, which installs the runtime deps and the `dev`
group -- and psycopg is in neither.
The guard moves into `live_dsn`, which is reached only once a test has already
been selected, and its reason now names the command that fixes it. Verified in a
fresh `uv sync` venv with psycopg absent: `442 passed, 8 deselected`, and
`-m integration` there skips with an actionable message instead of erroring.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lse claim
`_RULE_PRECEDENCE` is dead. Only ADV002 and ADV003 ever emit the same DDL string;
ADV002 is hardcoded MEDIUM and ADV003 hardcoded HIGH, so the confidence element
alone decides and the second tuple element is never consulted. Setting
`_RULE_PRECEDENCE = {}` left all tests green.
Its docstring had also gone false. It still explained the tie-break as necessary
"capping ADV003 at MEDIUM (it cannot see partial-index predicates)" -- but Task 4
restored HIGH precisely because Task 2 gave the rule that visibility. A tie-break
that cannot be reached, justified by a constraint that no longer holds, is worse
than none: it reads as evidence the collision is handled where the confidence
values are what actually handle it.
`_dedupe_by_ddl` now compares `_CONFIDENCE_ORDER` directly and the docstring
states what is true, including why the tie disappeared. Behaviour unchanged: the
existing dedupe test still shows ADV003 surviving, now on confidence alone, and
inverting the comparison still fails it (`assert 'ADV002' == 'ADV003'`), so the
collapse is still pinned in the direction that matters.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two promises the tool cannot keep as written. Documentation only -- no behaviour change in either case, deliberately. --min-cost-share said "Suppress proposals below this share of workload cost", unqualified, in both the flag help and the README table. Neither propose_unused_indexes nor propose_redundant_indexes takes the parameter: index hygiene is read out of the catalog and has no cost evidence to weigh. Against a live server `--min-cost-share 5` -- an impossible threshold -- still returned two proposals. Both places now name the cost-weighted rules the threshold reaches (ADV001, ADV004, ADV005, ADV006) and say ADV002/ADV003 are always reported. Filtering them on a share they do not have would be inventing evidence. Second, sqlglot's postgres generator renders `interval $2` as `INTERVAL '2'`, so a report stamped `"redacted": true` shows `created_at > CURRENT_TIMESTAMP - INTERVAL '2'` where the user wrote `interval '1 day'`. Nothing leaked -- the 2 is Postgres's own parameter index -- but it reads as a retained literal and is not valid SQL to copy out and run. The quirk was noted in a docstring; nothing an operator reads mentioned it. It now sits in the README's advise section beside the data-protection paragraph, which is where someone doubting a redacted report will look. Not fixed in sqlglot's rendering, by design. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Batch 1 of 3 follow-ups to
sqlquality advise(Postgres). Hardens the adapter againstreal-server behaviour that the unit suite could not see, and puts a live Postgres in the
loop so the next batch is not flying blind.
What changed
Index awareness (Tasks 1–4).
advisenow readspg_get_indexdefand understandsexpression and partial indexes instead of quietly mis-reading them:
correctly reported as already covered.
WHERE …) or an expression index no longer counts as coverage foran unqualified predicate — it covers a subset of rows, not the workload.
lower(note)stopsmatching a column named
not.support it, with the rationale stating the gap.
Live Postgres integration suite (Tasks 6–7).
tests/integration/brings uppostgres:16 with
pg_stat_statementspreloaded, seeds a workload, and runs the realintrospection SQL plus an end-to-end
advise. Markedintegrationand deselected bydefault —
uv run pytestneeds neither Docker nor psycopg.Two production bugs were found only by that suite:
pg_class.reltuples = -1(a table that was neverANALYZEd — the default for a freshtable) was read as "tiny table", which suppressed every proposal silently, and also
produced a negative NDV. Now translated to
None(unknown) at the boundary.redact_treedescended into$Nplaceholders (Parameter(this=Literal(N))), turninginterval $2into a danglingINTERVAL, which reparsed as a column named INTERVAL,failed to qualify, and silently dropped the whole query group. Literals inside
exp.Parameterare now left alone.workload/secrets.py(Task 5). Credential handling extracted into one module with asingle
secrets_for/scrubseam, so the "no credential reaches output" invariant has oneplace to hold rather than three.
Trivia (Task 8 and the fix wave). The report's coverage window read
since stats reset at Nonefor any database whose stats were never reset — i.e. thedefault — while ADV002's own rationale tells the operator to go check that window.
--min-cost-sharedocumentation qualified; the unreachable dedupe tie-break and its false"ADV003 wins ties" claim deleted.
Review
Final whole-branch review (opus) found nine issues; all nine fixed in one wave, none
disputed. Notably it found a thirteenth hollow test in this line of work: the
has_expressionshalf of_covered— half of the branch's headline claim — was completelyuntested. Every expression test passed with
columns=(), where the guard could never bethe reason, so deleting the guard left all 437 tests green. The shape it protects
(
CREATE INDEX ON orders (lower(note), status)) would otherwise falsely cover a candidateon
statusand silently withhold a genuine HIGH ADV001.The scoped re-review confirmed all nine fixes discriminate under independent mutation from
purged bytecode caches, and verified all seven invariants live — including invariant 4
against a role lacking
pg_read_all_stats(run continued, exit 0) and invariant 3 againsta wrong password and an unreachable host (no credential in output).
Residuals — none blocking
zero-width row, not a denied grant, which short-circuits earlier). Report accuracy only;
shipped code is strictly safer than the finding's text.
MAX_TIMEOUT_Sproductionreads, so widening the constant slips past it. Pinned elsewhere by
test_out_of_range_timeout_exit_2_before_connecting._FakeCursor.executedis still written and never read.Recommended follow-up (not in this PR):
ci.ymlrunsuv sync --all-extras, so CI cannever catch a regression of the psycopg-guard fix — the invariant that
uv run pytestpasses with no extras and no Docker has no permanent guard. A job that runs the default
suite after a plain
uv syncand fails on any skip would close that.Verification
442 passed, 8 deselected(deselected, not skipped — confirmed in a fresh venv with noextras installed)
8 passedfor the integration suite against live postgres:16ruff check,ruff format --check,mypy src/sqlqualityall clean🤖 Generated with Claude Code