fix: price a covering projection by what it can prune, not what it mentions - #1128
Conversation
…ntions (commandprompt#1126) commandprompt#1107 replaced a constant 0.5 with the selectivity of clauses referencing sortKey[0]. That fixed the case where the selectivity came from a different column. It left a narrower one: a single RestrictInfo that ORs a sort-key range with a predicate on another column REFERENCES the sort key, so the membership test counted it whole and credited the projection with a selectivity its sort order cannot deliver. WHERE sk BETWEEN 1 AND 2000 priced 0.100 earns it WHERE sk BETWEEN 1 AND 2000 OR kind = 'odd' priced 0.101 earns nothing The second prunes nothing and is slower than the base scan it undercuts tenfold: 0 usable skip predicates, 0 vectors skipped, all 20 chunk groups read, 120 vector decodes against 80, 2.653 ms against 1.996 ms. ASK THE FUNCTION THAT DECIDES SKIPPING. pgcolumnar_clause_to_scankey already answers "can this clause prune, and on which column": it returns 0 for a BoolExpr, because a BoolExpr is not an OpExpr and never becomes a scan key, and it records sk_attno per key. Pricing now keeps a clause only when it yields at least one key and every key it yields is on the sort key. One definition of "can skip", shared by the price and the executor, rather than a second one restated in the cost path. NOT GATED ON exact. The batch fold needs exactness because scan keys are its whole row filter (commandprompt#715); pruning does not. An anchored LIKE (commandprompt#426) and an IN-list range (commandprompt#704) prune honestly, and gating on exactness would decline a projection that genuinely wins. That is the silent direction, so its control ships beside the arm: mutating the gate onto exact reddens the IN-list arm exactly as intended. THE FIXTURE HAD TO CLEAR THE ONE-STRIPE FLOOR. The first version of the arm used a 100-row range; at 20 stripes the floor is 0.05 and both the fabricated discount and the honest one price there, so a broken guard and a working one were indistinguishable and the arm passed against the defect. The range is 10% now and a premise asserts it is above the floor, so the arm cannot quietly go vacuous again. Checking only the first scan key rather than every key reddens nothing, because no current clause shape writes keys on two columns. That is recorded in the comment as untested insurance rather than claimed as a property. THE PARITY TOOL CAUGHT THE PORT BEFORE CI DID, on three of the five new names. This suite is declared one-for-one with its pytest twin, so a property has to be asserted under the SAME name on both sides, and I had written three of them differently: MISSING premise: the prunable range is priced above the one-stripe floor, so the arms differ extra premise: the prunable range is priced above the one-stripe floor The bash names are canonical here because the ledger rows were seeded from a bash run, so the port adopts them rather than the reverse. Renaming the ledger side would have meant re-seeding five rows across five majors to fix a typo. NOT A REGRESSION FROM commandprompt#1107: the old 0.5 also beat the base for this query and the planner also chose the projection. What changed is how confidently. Five ledger rows, seeded from one run per major merged in a single call, all carrying 15;16;17;18;19. Census re-derived by counting on the merged tree: 1396. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs
ec2c719 to
08fc6ea
Compare
|
Pushed
This suite is declared one-for-one with its pytest twin, so a property has to be asserted under the same name on both sides. I wrote three of the five new names differently: The bash names are canonical here, and for a reason worth stating: the ledger rows were seeded from a bash run across five majors, so the names on that side are already committed to After the alignment, on the committed tree rather than the working one: Why I did not catch it locally: I ran the guard half before the reseat onto The two full matrices and the five-major seed in the PR body were run on the pre-reseat tree and are unaffected by this: the change is three assertion names in the Python port, and the C, the shell suite and the ledger rows are byte-identical. |
jdatcmd
left a comment
There was a problem hiding this comment.
Approving. I reproduced the red independently rather than reading the before/after block.
The removal proof, run here
I dropped this PR's test/projection_scan_cost.sh onto unfixed main (6ceb7dc,
.so md5 2da74f8d3630) under PG17 and ran it:
-- unprunable OR ratio=0.101 prunable range ratio=0.100 IN-list ratio=0.125
PASS premise: every unprunable-clause scan has a positive run cost
PASS premise: the prunable range is priced above the one-stripe floor, so the arms differ
FAIL a clause that mentions the sort key but cannot prune on it does not cheapen a covering projection: got [cheap] want [not-cheap]
PASS while a plain range on the sort key still earns its discount
PASS and an IN-list on the sort key keeps its discount, which gating on exactness would lose
accounting: 16 passed + 1 failed + 0 unrunnable + 0 skipped = 17
Your three ratios reproduce to the digit, and exactly one arm fails. Both premises
pass in the red state, so neither is carrying the failure, and both controls stay green
where controls should.
What I checked in the C rather than took
pgcolumnar_clause_to_scankeyreally does return 0 for the OR:if (!IsA(clause, OpExpr)) return 0;atcolumnar_customscan.c:980, and aBoolExpris neither that
nor theScalarArrayOpExprhandled just above it. The fix's central claim holds.ScanKeyData scratch[2]is the documented maximum, not a guess.PgColumnarBuildScanKeys
allocates2 * list_length(qual)on the same reasoning (:1103-1105), the SAOP path
returns 2 (:943), andPgColumnarQualsExactlyKeyed:1141already uses the identical
scratch[2]with the identical comment. This is the established shape, as you said.- The five ledger rows each carry
15;16;17;18;19.
The vacuity I went looking for and did not find
The OR query references kind. Had the projection simply not been offered for it — not
covering — the arm would read 1.000 and pass without the pricing fix ever running. Your
red run rules that out: at 0.101 the projection was demonstrably chosen and priced. Worth
noting because it is the one way this arm could have gone quiet, and it is the
floor-vacuity you already caught wearing a different hat.
Recording the parity failure and its cause in the thread, rather than folding it into a
quiet push, is the part that made this quick to review.
Closes #1126. The remainder of #1107, found while reviewing it.
The defect
#1107 replaced a constant
0.5with the selectivity of the clauses referencingsortKey[0]. That fixed the case where the selectivity came from a different columnentirely. It left a narrower one: a single
RestrictInfothat ORs a sort-key range witha predicate on another column references the sort key, so the membership test counted it
whole and credited the projection with a selectivity its sort order cannot deliver.
The second query prunes nothing and is slower than the base scan it undercuts tenfold:
20,000 rows,
stripe_row_limit => 1000, scrambled physical order, projection sorted onsk,kind = 'odd'wheresk % 1000 = 0so its rows sit in every stripe.Not a regression from #1107. The old
0.5also beat the base for this query and theplanner also chose the projection. What changed is how confidently.
The fix: ask the function that decides skipping
pgcolumnar_clause_to_scankeyalready answers "can this clause prune, and on whichcolumn". It returns 0 for a
BoolExpr— aBoolExpris not anOpExprand never becomesa scan key — and it records
sk_attnoper key. Pricing now keeps a clause only when ityields at least one key and every key it yields is on the sort key.
One definition of "can skip", shared by the price and the executor, rather than a second
one restated in the cost path. That is selftest 320's rule: a check that recomputes a rule
tests the world instead of the code.
PgColumnarQualsExactlyKeyedalready uses this exactloop with a
ScanKeyData scratch[2], so this is the established shape and not a new one.Not gated on
exact. The batch fold needs exactness because scan keys are its wholerow filter (#715); pruning does not. An anchored
LIKE(#426) and an IN-list range (#704)prune honestly, and gating on exactness would decline a projection that genuinely wins.
Red, then green, with both controls
The two controls are the point. Declining a projection that would have won costs a plan
and reddens nothing, so the silent direction ships beside the defect.
The arm was vacuous first, and that is why there is a floor premise. My initial fixture
used a 100-row range. At 20 stripes the one-stripe floor is 0.05, and both the fabricated
discount and the honest one price there — so a broken guard and a working one were
indistinguishable and the arm passed against the defect. The range is 10% now and
premise: the prunable range is priced above the one-stripe floorasserts it, so the armcannot quietly go vacuous again.
Mutation testing, including the part that found nothing
The second is recorded in the code comment as exactly that rather than claimed as a
property: no current clause shape writes keys on two different columns, so requiring every
key to be on the sort key is cheap insurance I cannot demonstrate the need for.
Verification
The two matrices ran on
0603216, the pre-reseat tree; the reseat onto3a741adcarriesonly #1124's test changes, which were themselves gated. The five-major preflight above ran
on the shipped tree.
Five ledger rows, seeded from one run per major merged in a single call, so every row
carries
15;16;17;18;19and #1071's short-major warning stayed silent.One note recorded in the budget file because it cost me a wrong number today:
check_ledger.tsvhas no header row, sotail -n +2drops a real row and reports oneshort. The census command is unaffected because it skips nothing.
Interaction with #1127
#1127 reuses
projScalerather than recomputingsel, so this fix reaches its parallelcovering path with no second edit. Had the cost model been copied, one of us would have
fixed one copy. Whichever of these lands second needs its census re-derived: each states
1396 against the current main, and both landing makes that 1401.
🤖 Generated with Claude Code
https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs