feat(advise): schema-qualified keying, join/group rules, and wrapped reads - #12
Merged
Conversation
Introduce Relation(schema, table) and resolve it in the extract layer via the introspected schema map, not Table.db alone -- qualify() leaves db empty for the normal bare-table case, which previously would have keyed every real workload under schema="". Also catch SchemaError (not an OptimizeError subclass) alongside OptimizeError so an ambiguous bare name raises UnqualifiableQuery instead of crashing the run. Tasks 2-4 carry Relation into aggregate, catalog facts, rules and the CLI; until then test_workload_aggregate.py, test_workload_rules.py, test_advise_cli.py and test_models.py fail on the changed ColumnUsage field, as expected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ation.__str__
Code review of the Relation task found four gaps between what the tests pin
and what the implementation actually guarantees:
- _collect_dml's `len(tables) == 1` guard was unpinned; a multi-table
UPDATE ... FROM with a bare column would silently misattribute it to
tables[0] with no test catching the regression.
- test_explicitly_qualified_table_uses_the_schema_it_names survived deleting
resolve_relation's `if table.db:` branch outright, because its fixture
table lived in only one schema. Rewritten against a name held by two
schemas so only the explicit-db branch can produce the right answer.
- Relation.__str__ had no test at all.
- resolve_relation trusted an explicit table.db without checking it was
actually introspected. Probed sqlglot 30.12 directly: qualify() validates
this for SELECT-scope columns but not for UPDATE/DELETE targets or their
bare columns, so `UPDATE other.orders SET status = 'x' WHERE id = 1`
against a schema map missing "other" sails through untouched and would
manufacture a phantom Relation("other", "orders"). Added the missing
membership check plus regression tests.
All four new/changed tests were confirmed to go red under the mutation the
review specified before being restored.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
aggregate() now keys usage by Relation instead of bare table name, so two introspected schemas holding the same table name (e.g. sales.orders and staging.orders) produce two entries rather than a merged, aliased one. extract_usage's ambiguous-bare-name case (sqlglot's SchemaError, matched loosely on "ambiguous mapping" since there is no error code) is now raised as AmbiguousRelation, a UnqualifiableQuery subclass, and counted in a new Aggregation.skipped_ambiguous rather than folded into skipped_unqualifiable or left to crash the run — the remedy for each is different: qualify the query / run once per schema, versus widen the introspected schema. star_tables is rewritten against the nested schema map to return relations too, declining a name held by two schemas rather than guessing.
…up rules, unwrapping)
…guous DML targets Review findings on the relation-keyed aggregate rollup: - star_tables text-matched the schema's table names against raw SQL, which cannot see a schema qualifier and diverged from resolve_relation in both directions: `select * from nosuch.items` resolved through a bare-name collision with an unrelated schema (the exact phantom resolve_relation's table.db guard exists to refuse), while `select * from sales.orders` naming one side of a same-table-name collision was dropped as if unqualified. Rewritten to parse each starred statement and resolve its exp.Table nodes through resolve_relation directly, so the two agree by construction. - The usage sort key's `u.relation` component was untested; two relations tied on cost/column/role now have a pinned, canonical order. - _collect_dml's sole-target resolution silently dropped an ambiguous bare DML target (qualify() does not validate UPDATE/DELETE targets, so no SchemaError was ever raised for it) with no counter incremented — the one case that made a statement look analysed when it was not. It now raises AmbiguousRelation, same as the SELECT-path ambiguity. - Added the missing denominator-semantics test for the ambiguous path, and replaced hardcoded "select_star" literals in tests with FLAG_SELECT_STAR.
CAP_SCHEMA, CAP_TABLE_FACTS, CAP_NDV and CAP_INDEXES filtered on schema but never returned it, so a row from sales.orders and one from staging.orders were indistinguishable and the last one silently won. Each statement now selects its schema as the first column, and fetch_schema/fetch_table_facts/ fetch_indexes are reworked to key by the schema-qualified Relation Tasks 1-2 introduced. TableFacts.name becomes TableFacts.relation to match. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- test_every_relation_returning_statement_selects_its_schema was hollow: it grepped the whole statement, so the schema-in-WHERE-clause substring passed even with the schema stripped from the SELECT list. Now checks the select list specifically (text between SELECT and the first FROM). - fetch_schema's abstract docstring in base.py still described the flat shape this task removed; updated to the nested shape and why. - propose()'s facts param in base.py was still dict[str, TableFacts], which was the actual cause of the one cli.py mypy error; now dict[Relation, TableFacts], clearing it. - Corrected the over-fetch comment: the schema filter does still exclude unintrospected schemas: what over-fetches is a same-named table in a different *requested* schema not itself in `relations`. - ruff format --check was failing on four hunks in the touched files; reformatted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Each propose_* rule now keys on Relation and derives its DDL's schema from relation.schema instead of a single run-wide schema= keyword, so proposals for same-named tables in different schemas no longer collide or get stamped with the wrong schema.
…rsing Addresses Task 4 review findings: - ADV002/ADV003/ADV004 evidence carrying schema/table was untested for those three rules; added assertions confirmed RED under the mutations the review specified. - ADV006 named a wide table a starred statement never referenced whenever two schemas shared the table's bare name, since it matched on raw SQL text. Now resolves each statement's exp.Table nodes (parsed with the adapter's own dialect) against the wide set, the same policy star_tables already applies, falling back to bare-name text matching only when a statement fails to parse. - Added an end-to-end propose() test using a non-public relation.
Relation-keyed catalog facts (Tasks 1-4) removed the reason --schema was capped at one: _validate_schemas now dedupes and preserves order instead of exiting 2. skipped_ambiguous is surfaced in the coverage line, a new _ambiguity_warning naming the remedy, the JSON payload and the markdown report. Fixed report.py's "tables" key, which put a non-JSON-serializable Relation into the --json payload after the whole analysis had already run. Also: aggregate() now counts a bare `SELECT *` over a table two introspected schemas both hold as skipped_ambiguous too (previously uncounted anywhere, since a starless qualify() never raises for it), and _wide_relations_touched's unreachable, buggy parse-failure fallback is deleted rather than left untested (ingest() never lets an unparseable statement reach workload.stats in the first place). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
_analyzed_count ignored skipped_ambiguous, so an ambiguous statement counted as both "analyzed" in the coverage line and "unexplained" in _coverage_warning's share simultaneously. Worst case this silently suppressed the low-coverage warning exactly when ambiguity alone crossed the threshold (25/100 real share vs. an inflated-to-exactly-20% share). Also widen the bare-ambiguous-table gate in aggregate(): it required FLAG_SELECT_STAR, so `select count(*) from orders` and `select 1 from orders` over a colliding schema produced zero usage without ever being counted by either skip counter. Dropped the flag conjunct; any statement that resolves to zero usage because of a genuinely ambiguous bare table now counts, star or not. Added a CLI-level test proving _ambiguity_warning is actually wired into advise()'s command body, not just unit-testable in isolation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…dence ladder Review round 2 on ADV007: it could emit HIGH while falsely claiming "No existing index leads with it" next to a partial or expression index that does lead with the join column -- ADV001 already discloses both, ADV007 had neither. Add the same evidence/rationale disclosures, verbatim ADV001 wording. Also close three unpinned-guarantee gaps: the propose() wiring test only checked a 2-of-7 superset (deleting ADV007's whole block stayed green), the MEDIUM (unknown-NDV) branch had no test forbidding HIGH, and the cost_share < min_cost_share guard had no test forbidding "if False". All four now have red-then-green tests; README:800/808 corrected to note the disclosures apply to both index-creating rules.
Adds propose_grouping_indexes, wired into propose() after propose_join_keys. One composite index (not one per column, unlike ADV007) ordered by cost descending since redaction does not preserve GROUP BY column position; capped at MEDIUM/LOW, deliberately no HIGH branch, since whether Postgres uses the index for grouping depends on the planner's GroupAggregate vs HashAggregate choice, which the catalog cannot see. Discloses partial and expression indexes leading with the grouping columns, matching ADV001/ADV007. 14 new tests pin every confidence rung, the co-occurrence guard, the cost_share threshold, max_arity, and both disclosures; the composed-rules test now pins all eight rule codes. Updated --min-cost-share help text and the README rule table/limitations to reflect ADV008.
The extension rule checked each candidate column against only the seed's fingerprints, so a transitive chain -- a grouped with b in one query, a grouped with c in another -- welded (a, b, c) into one composite that no query in the workload actually groups by, while evidence still reported cost and fingerprint counts that read as support. Fixed by tracking a running fingerprint intersection and requiring every new column to share it, so the composite only ever grows to a size some single query supports. evidence now carries "co_occurring_fingerprints" (the honest joint count, same name/meaning as ADV004's) alongside the existing per-column "fingerprints", which never implied joint support on its own. Added tests for the transitive arrangement, the honest joint-support count, and the seed-shares-nothing case; proved both new tests RED against the reintroduced pairwise-with-seed bug.
report.py renders evidence as generic sorted k=v pairs with no per-rule text, so ADV008 emitting both "fingerprints" (per-column max) and "co_occurring_fingerprints" (the honest joint overlap) side by side left nothing in the artifact telling a reader which one actually supports a multi-column proposal -- someone could still read fingerprints: 2 as two query groups backing a composite only one query group groups by. Dropped "fingerprints" from ADV008's evidence, matching ADV004, which already omits it for the same reason: both propose an index justified by columns appearing together, so only the joint overlap supports the claim. ADV001/ADV007 keep it -- each proposes for one candidate, so a per-column count is the whole truth there. Documented the split in the docstring. Extended the joint-support test to assert "fingerprints" is absent, and confirmed it goes RED with the key restored.
_dedupe_by_ddl only reconciled proposals that render byte-identical DDL, which was enough while ADV002/ADV003 were the only pair that could tie. ADV007 and ADV008 (Tasks 6-7) broke that: ADV001 and ADV008 can now tie at equal confidence on identical DDL, and ADV001/ADV007/ADV008 can each reach a plain index from different evidence, so a strict-prefix pair (e.g. ADV001's (customer_id, created_at) alongside ADV007's (customer_id)) could ship in the same report -- advising a CREATE today and, via ADV003, a DROP on the very next run. - _CODE_PREFERENCE breaks equal-confidence DDL ties deterministically instead of by list order; the stale docstring claiming ties were unreachable is corrected. - _collapse_index_prefixes folds a proposal's columns into any wider proposal they are a strict prefix of, within one relation, excluding ADV004's partial indexes (a WHERE predicate makes it a different object). - _disclose_column_set_overlaps annotates same-column-set-different-order pairs (e.g. (status, region) vs (region, status)) instead of silently shipping both with no acknowledgement they overlap -- neither is a prefix of the other, so no collapse or future ADV003 pass would ever catch it. - _fold_discarded makes both collapses non-lossy: a discarded proposal's full rationale and confidence are appended to the survivor's, so a caveat only the losing rule stated (e.g. a low-NDV selectivity warning) never silently disappears. All three additions are order-independent, verified with permuted-input tests rather than by reading the code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…epeating itself Review findings on the proposal-collapse rework: - _disclose_column_set_overlaps never sorted its notes, so with two or more same-set partners the concatenated rationale depended on input order (six permutations gave six texts). Latent today -- ADV001 and ADV008 emit at most one composite per relation and ADV007 is single-column -- but order independence is the whole point of the tie-break this task restored, so it is sorted by the same canonical key _collapse_index_prefixes already uses. - Three guards no test noticed, each of which the suite stayed green without: the cross-relation guards in _collapse_index_prefixes and _disclose_column_set_overlaps, and the empty-columns guard in _index_creation_columns. A guard nothing pins is one refactor from gone. - test_prefix_collision_collapses_regardless_of_which_rule_appended_first was vacuous: its fixture absorbed a single proposal, so the sorted(absorbed, ...) it appeared to pin was a no-op and reversing it changed nothing. It now absorbs two. - ADV001 emitted LOW for a low leading NDV with a rationale byte-identical to its HIGH and MEDIUM text, while ADV007 explained itself for the identical reason. Batch 2 created that asymmetry by giving ADV007 the caveat, so ADV001 now states it in the same words -- an operator should not have to notice the explanation depends on which rule proposed the index. Confidence values are unchanged; only the text. - Folding a discarded proposal's rationale in verbatim repeated whole sentences, because all three index-creating rules share verbatim disclosure wording: a real three-way collision produced a 1384-character paragraph containing the same 40-word sentence three times. Folding is now sentence-wise, skipping only exact repeats already present, in first-occurrence order. Nothing unique is lost, and a discarded proposal that adds no new sentence still has its disagreeing confidence recorded. Verified: all seven reverting mutations turn exactly their own test RED from purged caches; determinism re-checked after the change, with all 24 permutations of a four-element colliding set and 400 shuffles of an eleven-element set (prefix chain, second relation, ADV004 partial, ddl=None, identical-DROP pair) each producing one distinct output. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ing them DECLARE ... CURSOR FOR SELECT ... and COPY (SELECT ...) TO ... are ordinary reads with real predicates, but both start with a keyword is_noise drops -- and every psycopg2 server-side cursor (cursor(name=...), what Django and SQLAlchemy emit for large result sets) produces a DECLARE. Add unwrap() to recover the inner query via text surgery (sqlglot cannot parse DECLARE at all) and run it in ingest before is_noise, so a cursor declaration is judged on the query it declares rather than on its wrapper. A DECLARE/COPY wrapping our own introspection is still filtered, since is_noise now runs against the unwrapped text. Updates the test_advise_cli.py coverage-line test and the _coverage_line docstring, which documented this as a known limitation.
Review of the DECLARE/COPY unwrap found the cursor half recovers predicates
but not cost: Postgres attributes a cursor's scan work to the FETCH
statements that follow, which stay filtered, so a DECLARE enters the
workload at near-zero cost and the default --min-cost-share can suppress it
outright. Rewrite unwrap's docstring, the README limitation, and the
markdown report's skip line to say so, and to stop claiming the noise
filter matches "by statement prefix" now that DECLARE/COPY are unwrapped
before it runs.
Also fix a real double-count: under pg_stat_statements.track = all, one
COPY (...) TO produces both a verbatim top-level row and a normalised
nested-query row for the same execution, and they redact to different
fingerprints. Filter CAP_WORKLOAD to s.toplevel, which requires PostgreSQL
14+ (toplevel doesn't exist before then) — raise the documented floor
accordingly rather than silently breaking the 13+ contract the hint used
to advertise, since 13 is already past its EOL.
Round out unwrap()'s test coverage: pin the COPY prefix anchor against a
later '(', the TO/FROM write exclusion, and .strip() (load-bearing, not
cosmetic — greedy .* under DOTALL lets trailing whitespace into the query
and that changes fingerprint grouping). Drop three parametrize members that
looked like coverage but couldn't discriminate any plausible mutation of
the regexes they sat next to.
…nstead Live PG16 testing under track = all with real PL/pgSQL functions showed the toplevel filter is worse than the bug it fixed: toplevel = false is the only way Postgres ever exposes a function body's nested SQL, so filtering to toplevel silently emptied a genuinely hot, function-wrapped query's evidence entirely and let a colder query win a high-confidence proposal in its place — confidently wrong, with no disclosure (degraded stayed empty, coverage looked complete). No s.query text pattern distinguishes a COPY's nested duplicate from a function's nested statement; both attempted predicates either kept the duplicate or discarded valid COPY reads under the default track = top. Revert AND s.toplevel and the 14+ floor bump (hint, SQL comment, README prose, --dry-run sample) back to 13+ everywhere, and document the COPY double-count as an accepted, known limitation in the README instead. Replaced the toplevel-behavior tests with a pin that the filter stays absent and a regression pin for the documented double-count. Also: added real ingest-level tests that FETCH and CLOSE are filtered as noise (a prior docstring claimed this was "already exercised... by the ordering tests below" — it wasn't; deleting fetch|close from _LEADING_NOISE left the full suite green). Corrected "near-zero calls, time and rows" for a DECLARE to note calls is accurate — only time and rows are misattributed to the FETCH statements that follow.
…ainst real postgres Fixes test_introspection_live.py's pre-Task-3 API (bare-string keys, flat fetch_schema shape) that had been rotting silently since it is deselected by default. Seeds orders into both public and staging with different row counts, an unindexed join key and GROUP BY, an unused index, and a DECLARE ... CURSOR FOR read, then adds four live tests proving multi-schema keying, ADV007/ADV008, and cursor-unwrapping against a real Postgres rather than canned fixtures. Brings CHANGELOG and the design spec's deviations section in line with what Batch 2 shipped; README was audited and found already correct.
…in their claims Review found 3 of 6 live assertions were decoration: reverting DECLARE unwrapping, removing either ADV007 or ADV008, or leaving reltuples=-1 unfixed all left the suite green. Fixes: the cursor's inner query now filters a predicate no other seeded statement uses, so its query group can only exist if unwrapping ran; ADV007/ADV008 are asserted individually instead of as a disjunction; public.order_items gets autovacuum_enabled=false (a bare "don't ANALYZE it" was observed live to be non-deterministic -- autovacuum analyzed it ~2.5s after seeding) and a dedicated test pins row_estimate is None plus the resulting LOW-confidence ADV007 proposal. Also adds a live assertion on fetch_schema's nested shape and non-vacuity guards to the two tests that lacked them, and brings the design spec's dataclass bodies (not just its deviations section) in line with the shipped Relation-keyed types. Each of the three fixed claims was confirmed to go RED under the exact regression it now guards against, then restored and reconfirmed GREEN, before this commit.
…minors
F1 (High) — ADV001 required no joint support, so it welded the hottest equality
columns and the hottest range column of a relation together whether or not any
single query used them together. A DECLARE ... CURSOR FOR read at 0.003% of window
cost turned (customer_id, created_at) into (customer_id, tenant_id, created_at) —
which cannot satisfy the hot query's ORDER BY created_at — and reported
fingerprints: 1 as if one query group backed all three when zero did. ADV001 now
carries a running intersection of fingerprint_ids as ADV008 does, and reports the
honest joint count under co_occurring_fingerprints with no per-column count beside
it (ADV004/ADV008's existing split).
F2 (Med-High) — the source comment and README justified the absent s.toplevel
filter with a claim measurement disproves: a COPY's nested row keeps its wrapper
while a PL/pgSQL body is recorded bare, so a narrow predicate does exist. The
filter stays out — naming s.toplevel raises the floor to PG14 and would cost a
PG13 user the whole workload capability — but for that stated price, not for
impossibility. Also documents the larger, genuinely unfixable half: under
track = all every PL/pgSQL call double-counts, halving every cost_share.
F3 (Med) — _stub_adapter hard-coded self.schemas = ("public",) after the CLI had
resolved --schema, so no default test could see adapter.schemas = schemas being
deleted. The stub no longer overwrites it, and two tests assert the tuple each
schema-scoped catalog query received. Subsumes M7.
F4 (Med) — ADV003 iterated every relation in `existing`, including rows arriving
through fetch_indexes' bare-name over-fetch, so it could propose DROP INDEX for a
relation the workload never touched. Scoped to hot_tables like ADV002, and
fetch_indexes' "no consumer" comment corrected.
F5 (Med) — markdown and JSON labelled the raw query-group count "analyzed" where
the terminal said "analyzed 6 of 8". One helper in models, three call sites.
F6 (Med) — _fold_discarded claimed a collapsed proposal "reached the same index",
never true for prefix collapse. The two collapse kinds now word their attribution
differently, naming the narrower column list.
F7 (Low) — the proposal-collapse layer is documented in README, CHANGELOG and the
design spec, including that a rule can fire and contribute no proposal and that an
absorbed proposal's evidence is discarded.
Minors: M2 pins the qualified-reference guard in _references_an_ambiguous_bare_table;
M3 deletes the dead mentions_table wrapper and retargets its test at
mentions_identifier; M4 corrects the false premise in _wide_relations_touched's
docstring and pins the real one; M6 deletes the dead DEFAULT_SCHEMA constant.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… that passed vacuously Re-review of the final fix wave. None of these change emitted advice. - ADV008's docstring still said its joint-support evidence was "deliberately unlike ADV001", which stopped being true when ADV001 gained the same running fingerprint intersection. ADV001, ADV004 and ADV008 are now all on the joint-support side of that split; ADV007 and ADV005 speak for a single column and keep their per-column count. - test_the_toplevel_tradeoff_... searched the whole of postgres.py for "postgresql 14", which _row_estimate's unrelated reltuples docstring satisfies — so the assertion passed with the sentence it exists to protect deleted. It is now scoped to the CAP_WORKLOAD comment block, and the enumeration of three stale phrasings is cut to the one that was actually there: whack-a-mole substrings give false confidence, and the positive assertion is what pins the reasoning. - Nothing in the suite read a catalog statement's second bind parameter, so replacing the table list with a bogus value in all three relation-scoped statements left every test green. A real server asked about the wrong relations returns nothing, which is indistinguishable from a table with no statistics and no indexes, and suppresses proposals with no message. Now asserted per statement rather than in aggregate. - Dropped the assertion pinning the PL/pgSQL measurement to 68.21/67.67 ms. Two runs of the same fixture gave 68.21/67.67 and 46.44/46.37: the shape reproduces, the numbers are that machine's. A test that fails on an honest re-measurement teaches people to edit tests instead of reading them. The README now says "on one PostgreSQL 16 run" and notes the absolute figures vary. Verified: feeding a bogus table list to all three catalog statements, and removing the version cost from the CAP_WORKLOAD comment, each turn exactly their own test RED from purged caches. 576 passed, 14 deselected; integration 14 passed against live postgres:16. 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 2 of 3 follow-ups to
sqlquality advise. Three deliverables, plus a fourth the reviewsforced into existence.
Multi-schema keying
Every catalog fact
advisecollected was keyed on the bare table name, so two schemas eachholding an
ordersaliased into one another — the last catalog row won the row estimate, andqualify()resolved columns against the union of both column sets.--schemaaccepted only onevalue, and rejected two with an honest explanation.
A
Relation(schema, table)value type now threads through extract → aggregate → catalog → therules → CLI and report, and
advise --schema public --schema salesworks.The load-bearing detail, found by probing sqlglot before writing the plan:
qualify()leavesTable.dbempty for a bare table reference, even when the nested schema resolves itunambiguously. Since production SQL relies on
search_pathand saysfrom orders, aTable.db-only implementation would key almost every real workload underschema="", match nocatalog fact, and silently suppress every proposal. Schema resolution therefore reads the
introspected schema map, and only trusts
Table.dbwhen it is both present and in that map —qualify()does not validateUPDATE/DELETEtargets, soUPDATE other.orders SET …wouldotherwise manufacture a phantom relation.
Ambiguity is counted and reported rather than guessed: a bare name held by two introspected
schemas increments
skipped_ambiguousand the run says which remedy applies.Two roles that were collected and thrown away
ColumnRole.JOINandColumnRole.GROUPwere classified, cost-weighted, and counted in everycost_sharedenominator — and read by no rule at all.a foreign key, so this gap is common and expensive.
GROUP BYalready sorted. Capped at MEDIUM with noHIGH branch, and the cap is unreachable by construction (verified across a 1,080-combination
sweep): whether the planner picks
GroupAggregateoverHashAggregatedepends onwork_mem,group count and aggregates, none of which this tool can see. HIGH would be a claim about the
planner rather than the catalog.
A collapse layer, because the rule set contradicted itself
With three index-creating rules,
advisecould recommend(customer_id, created_at)and(customer_id)in the same report, both at HIGH — and then adviseDROP INDEXon the narrowerone next run. Proposals are now collapsed by prefix within a relation, using the tool's own
definition of redundancy, and no caveat stated by a discarded proposal may vanish from the report.
Where two proposals cover the same column set in a different order, they are disclosed rather
than collapsed: different leading columns genuinely serve different probes.
Determinism is verified rather than assumed — all 24 permutations of a colliding set, and 400
shuffles of an 11-element set across six
PYTHONHASHSEEDvalues, produce the same SHA-256.Wrapped reads
DECLARE … CURSOR FOR SELECTandCOPY (…) TOare ordinary reads with real predicates, but bothbegin with a keyword the noise filter dropped — so on any workload using server-side cursors
(every psycopg2
cursor(name=…), which is what Django and SQLAlchemy emit for large result sets)those reads were counted as "filtered" and discarded. They are now unwrapped before the noise
check, which also means an inner introspection query is still caught.
Honest about what this delivers: measured on PG16,
FETCHcarries ~72.9 ms while theDECLAREcarries ~0.3 ms, so a cursor read contributes its columns but little cost. Documented.
What the reviews found
Each task was reviewed and its findings fixed; the whole-branch review then found seven more, as
it did on both predecessor branches.
The one that mattered was a cross-task regression no per-task review could see: cursor unwrapping
made ADV001 emit a worse index at HIGH, because a read costing 0.003% of the window welded a
third column into a composite that could no longer satisfy the hot query's
ORDER BY. ADV001 wasthe only index rule with neither a per-column cost floor nor a joint co-occurrence requirement; it
now carries the same running fingerprint intersection ADV008 uses, verified against
over-correction with an A/B harness over seven constructed cases.
The live suite kept earning its cost. It proved the two-schema separation on real catalog rows
(
orders_pkeyexists under the identical name in both schemas), confirmed thereltuples = -1sentinel and negative-
n_distinctpaths live rather than by fixture, and caught a fix that had tobe reverted:
AND s.toplevelremoved aCOPYdouble-count but excluded every nested statement —and since that is the only way Postgres exposes SQL inside a function, it produced a different
proposal graded
highwhile the genuinely hot columns vanished. Confidently wrong is worse thanan inflated cost share on a non-default setting.
Two limitations were found and documented rather than papered over: the cursor cost attribution
above, and that under
pg_stat_statements.track = allboth aCOPY (…) TOand every PL/pgSQLcall are counted twice.
Seven findings on this branch were "a test asserting over a set that checks one member" — a
four-statement SQL property checking one statement, an evidence contract pinned for two of six
rules, a composed-rules test covering two of seven that let a completely unwired rule pass. Where
a test now asserts over a set, every member was mutated independently.
Behaviour changes
Safe because
adviseis unreleased (every mention is under[Unreleased]):on the live seeded workload.
--json'sanalyzed.query_groupsis now the analysed count rather than the window total,matching what the terminal always printed. The total moved to
query_groups_in_window.MIN_ROWS_FOR_INDEX,_covered, and the partial/expression-index guards are unchanged.Verification
576 passed, 14 deselected— deselected, not skipped; the default suite needs no extras and noDocker
14 passedfor the integration suite against livepostgres:16ruff check,ruff format --check,mypy src/sqlqualityall cleanFollow-ups, not in this PR
rule that never calls
_covered. Pre-existing; the fix is a design question, not wiring.uv sync --all-extras, so no job exercises the Docker-free, extra-free default suitethat this branch depends on. Carried from Batch 1.
🤖 Generated with Claude Code