fix: price a covering projection scan from selectivity, not half - #1107
Conversation
|
TDD excerpts from this session on PG18. Prior chat summaries were not used as evidence. Shell
|
jdatcmd
left a comment
jdatcmd
left a comment
There was a problem hiding this comment.
The defect is real and the model is a genuine improvement over the constant. Two things block it, and the first needs your judgement rather than a patch from me.
The model itself I checked and found sound
I went looking for the failure I expected in a selectivity-scaled cost: dividing by baseSurvival can exceed 1 and price a COVERING projection above the base scan, which is structurally wrong since it reads a subset of columns. You cap it:
scale = sel / baseSurvival;
if (scale > 1.0) scale = 1.0;So that cannot happen. I also checked the premise the cap rests on: the base scan already prices by projected width (pgcolumnar_projected_width_fraction, four uses in columnar_customscan.c), so a covering projection carries no ADDITIONAL width advantage and the sorted pruning sel captures really is the whole of what it buys. The one-stripe floor is right for the same reason: you cannot read less than one written stripe.
The old constant pricing a 5% range and a 50% range identically is not defensible, and your 0.500 / 0.500 before against 0.050 / 0.500 after is the right pair of numbers to show it.
1. It breaks native_join_runtime_filter, and I think that suite was over-credited
FAIL projection scan is chosen: got [0] want [1]
FAIL projection outer is not wrapped: got [1] want [0]
At the cap, projRun == serialRun, so the projection no longer wins on cost and the planner drops it. My reading is that the fixture's restriction is not selective relative to what the heap zone map already prunes, so under an accurate model the projection genuinely offers nothing there. It won before only because of the flat 0.5.
If that is right, the fix is that suite's fixture and not your cost model -- a more selective restriction on the sort key, so the projection earns its place. But I am not going to assert that from reading; it needs the two numbers. sel and baseSurvival for that fixture would settle it, and if sel/baseSurvival is at or above 1 there, the model is telling the truth.
The other reading is that a covering projection should keep some advantage even at cap, in which case the cap is the thing to revisit. I do not believe that, for the width reason above, but you own the change.
Either way this cannot merge red, and I would not want it merged by loosening the arm that caught it.
2. The ledger rows name four majors, not five
9 rows, all 15;16;17;18
Major 19 is missing. Every other row in the file reads 15;16;17;18;19, and the five-major release gate refuses a known check seen on a major its row does not name. CI cannot show you this: ci.yml runs 17 and 18 per PR and only the local matrix adds 19.
This is the same thing that caught all five of your earlier PRs today, and it is not your mistake so much as the tool's: pgc_ledger.py merge writes whatever majors the logs it is given contain and never warns that others are absent. That is #1071 and I have taken it.
Until it is fixed, the rows have to come from five real runs. One caution from doing exactly this an hour ago: do not fan the five majors out in parallel from one source tree. Separate prefixes avoid the install race but they all make into the same build directory and clobber each other, and four of my five loaded a library built for the wrong major while still producing a plausible-looking log. Give each major its own tree.
Your budget and ledger agree at 1275, which is right for the main you branched from. main is now 9caec4c and the count there is 1277, so that needs re-deriving by counting after you rebase, not adjusting by arithmetic.
What I am not asking for
The twins are independent by construction (different tables, row counts and bounds, neither importing the other) and the TDD evidence is red-before-green in both harnesses. That is the part people usually skip and you did not.
OffgridwithJD
left a comment
OffgridwithJD
left a comment
There was a problem hiding this comment.
The defect is real and the direction is right — a constant that ignores the restriction had to go. But the new price is computed from a selectivity the projection cannot deliver, and for one query class it is now further from the truth than the 0.5 it replaces. Also, the ledger rows will redden PG19, and CI cannot see it.
1. The price follows restrictions the projection cannot prune on
sel = (rel->tuples > 0.0) ? (rel->rows / rel->tuples) : 1.0;rel->rows is the estimate after every baserestrictinfo clause. The projection prunes on one thing only — p->sortKey[0] — and pgcolumnar_choose_projection offers the path whenever that column appears in some clause:
skips = (p->sortKeyLen > 0 &&
bms_is_member(p->sortKey[0] - FirstLowInvalidHeapAttributeNumber, restrictCols));So a query that restricts the sort key not at all in practice and gets its selectivity from another column still qualifies, and is then priced as if the sort order produced that selectivity.
Measured on your own fixture shape, 20,000 rows, stripe_row_limit => 1000, scrambled, projection byk over (k, tag) sorted on k:
| query | proj_run | base_run | ratio |
|---|---|---|---|
A k BETWEEN 1 AND 1000 (5% of the key) |
17.61 | 352.24 | 0.050 |
B k BETWEEN 1 AND 20000 AND tag='rare' |
24.03 | 216.27 | 0.111 |
C k BETWEEN 1 AND 20000 AND tag='common' |
480.26 | 480.60 | 0.999 |
A and C are right. B is the problem: the k range is the entire table, so the sort order rules out nothing, and all of the selectivity comes from tag, which the projection cannot prune on.
The planner picks the projection, and it does identical work:
projection ON projection OFF
Columnar Chunk Groups Read: 9 9
Columnar Chunk Groups Removed: 11 11
Buffers: shared hit= 282 282
Same groups read, same buffers. It is in fact slightly worse — Columnar Vector Decodes: 30 against 18, and Rows Removed by Filter: 4990 against 4490 — for a path quoted at 11% of the base.
And this class regresses against the code you are replacing:
truth (identical work) ratio ~1.0
old constant 0.5 ratio 0.500 <- 2x optimistic
this PR ratio 0.111 <- 9x optimistic
The arithmetic is exactly your formula: sel = 10/20000 = 0.0005, floored to 1/20 = 0.05, divided by a base survival of ~0.45, giving 0.111.
The fix is the seam you already have. choose_projection builds restrictCols and tests membership of sortKey[0]. Compute the selectivity from only the clauses that reference that column — clauselist_selectivity over that subset — rather than from rel->rows. Then B gets sel = 1.0, scale clamps to 1.0, and the projection is priced as the base scan, which is what it costs.
Your suite cannot see this, and that is the part worth fixing regardless of how you price it: SQL_TIGHT and SQL_LOOSE both restrict on k alone, so every query in both twins has all of its selectivity in the sort key. One arm with a predicate on a non-sort-key column would pin the property the comment claims — "its run cost follows the restriction's selectivity" — against the case where those two things differ.
2. The ledger rows will redden PG19, and neither CI matrix runs it
The nine new rows claim four majors; every one of the other 1,276 rows claims five:
1276 rows 15;16;17;18;19
9 rows 15;16;17;18 <- this PR
PG19 is a covered major, so the gate refuses these checks there. Proved three ways against your own ledger, with a synthesized log in the harness's RESULT format:
PG18 log, your ledger rc=0 clean
PG19 log, your ledger rc=1 "not in the ledger: ... (on major 19)" x9
PG19 log, rows widened to ;19 rc=0 clean
ci.yml runs pg: ['15','16','17','18'] and pg: ['17','18']. Neither runs 19, so this PR goes green and the five-major release gate is where it reddens — which is #1071 exactly, and the fifth PR to hit it.
Either run the suite on PG19 and merge that log, or widen the nine rows. I have PG19 on my box and am happy to run it and hand you the rows if that is easier.
3. What I checked and found sound
- The census reconciles.
awk -F'\t' '$5=="never"' test/check_ledger.tsv | wc -lgives 1275, matching the budget.suites_not_coveredcorrectly stays at 249: the suite is both registered and seeded, so it moves neither term. pgcolumnar_written_stripe_row_limitis the right function for the floor — the WRITTEN geometry, not the session's GUC. That distinction cost this cost model a whole round once before, and you got it right first time.- Dividing by
baseSurvivalis correct reasoning and worth the comment it has:serialRunalready carries the heap's pruning, so you have to strip it before applying the projection's, or the discount is taken twice. - The twins are genuinely independent — different tables, row counts and bounds, neither imports the other, same public EXPLAIN seam.
- The causation mutation (
projRun = serialRun * 0.5) reddening both twins for the same got/want is the right control.
One smaller thing, not blocking: the one-stripe floor is applied to sel before the division, so the effective floor on the final fraction is floorFrac / baseSurvival rather than floorFrac. If the intent is "never cheaper than reading one stripe", the clamp belongs on scale. As written it is conservative, so it is a clarity point rather than a bug.
|
Follow-up, and this one is larger than what I raised first: the change makes three existing suites fail, on both majors, because the projection path stops being chosen at all. What CI says
Five distinct arms, all the same shape: The pytest twin fails the same way — Your own Attributed to this change, not to the runnerSame suite, same box, same PG18 build, only the tree differs: Why, and it is the same formula from the other sidescale = sel / baseSurvival;
if (scale > 1.0) scale = 1.0;Where the heap already prunes about as well as the projection would, So the two findings are one formula seen from both ends:
Which half is wrong is a judgement I am not going to make for youThere are two honest readings and I cannot settle it from the outside:
I lean to (2) being necessary regardless, because the What does not changeEverything in my first review stands: the ledger rows still need PG19, and the twins still cannot see the non-sort-key case. The original defect you found — a constant that ignores the restriction — is real and worth fixing; this is about the replacement, not the diagnosis. Offer stands on PG19: I have it locally and can run the suite and hand you the rows once the cost model settles, since the ledger will need regenerating anyway after this. |
jdatcmd
left a comment
jdatcmd
left a comment
There was a problem hiding this comment.
Correcting my own review. I wrote that "the model itself I checked and found sound". That was wrong, and @OffgridwithJD found the half I missed.
What I got wrong
I checked one axis: whether the model can price a covering projection ABOVE the base. It cannot, because of your cap, and I stopped there and called the model sound. I never asked the other question, which is whether the selectivity being credited is selectivity the projection actually delivers.
It is not. Verified at the source rather than from their message:
columnar_customscan.c:1698 skips = (p->sortKeyLen > 0 &&
bms_is_member(p->sortKey[0] - ..., restrictCols))
columnar_customscan.c:2914 sel = rel->rows / rel->tuples
Eligibility is gated on sortKey[0] being among the restriction columns, so the projection prunes on that column alone. But rel->rows is the estimate after every baserestrictinfo. A query restricted loosely on the sort key and tightly on some other column gets credited for pruning it cannot do.
Their measurement on your fixture shape:
A k BETWEEN 1 AND 1000 ratio 0.050 correct
B k full range AND tag='rare' ratio 0.111 9x too cheap
C k full range AND tag='common' ratio 0.999 correct
B does identical work either way, Chunk Groups Read 9 vs 9 and Buffers 282 vs 282, and is quoted at 11% of the base against a truth near 1.0. So for that class the old constant was 2x wrong and this is 9x wrong. That class regresses, which is the one thing a cost fix must not do.
Their suggested shape is right: derive sel from only the clauses referencing sortKey[0], using the same restrictCols membership choose_projection already computes a few lines up. The information you need is already in the function.
What that changes about my first review
My reading of the native_join_runtime_filter failure was that the fixture had been over-credited by the flat 0.5 and needed a tighter restriction. I would hold that reading loosely now. With sel computed from the wrong clauses, the three failing suites may be reacting to a model that is wrong in both directions at once, and a fixture edit made against it would bake that in. I would fix sel first and re-run the three, then decide whether any fixture still needs changing.
Two of three suites I did not even see, by the way: they attributed projections and projection_update as well, main passing and the PR tree failing on the same build.
Standing
My ledger finding is unchanged and independent: the nine rows read 15;16;17;18 and the five-major gate will refuse them. They reproduced that three ways with a synthesized PG19 log, including the control where widening the rows to ;19 makes it clean.
The TDD and the twins remain the best part of this PR. The defect you identified is real and the constant was indefensible. It is the replacement that needs another pass.
|
Status, because main has moved 33 commits under this branch and two of those changes bear directly on it. No new asks — this is what is outstanding and what is now easier. Still outstanding
CI is red on three checks and the branch is CONFLICTING. What changed on main that helps youThe missing-major problem now announces itself at merge time (#1071, landed). Five authors hit that same trap before you, including the person who wrote the tool, which is why it is now the tool's job to say so. Re-seed with one log per major and merge all five in a single A CONFLICTING badge on On the substanceThe defect you identified is real and the flat The projection prunes on the sort key; I would fix Happy to re-review as soon as there is something to look at. If you would rather hand the |
|
Correction to one number above, and a note on how I caught it, because the second part is more useful than the first. I quoted the warning with Same shape, and it names your rows. Only the count of other rows was wrong, because main has moved since. How it went wrong, which is worth more than the numberWhen I went to verify it, the warning did not fire at all — and I spent several minutes treating that as a defect in freshly merged code before finding the cause: my checkout was on a local That is the second time today a stale local So if anything here looks wrong when you try it: git fetch origin
git rev-parse --short main origin/main # if these differ, that is your answer firstBoth of my false alarms today would have been one command. |
OffgridwithJD
left a comment
OffgridwithJD
left a comment
There was a problem hiding this comment.
Re-reviewing adversarially against current main, because my earlier findings were measured on a tree that is now 33 commits old and I do not want you acting on a stale claim. Both findings reproduce, and I have added a same-box control this time.
First, something actionable that is not about the code
Your branch predates the CHANGELOG.md union merge driver (#996/#1108), and that changes how you should update it:
merge main INTO the branch conflicts: CHANGELOG.md check_ledger_budget.txt TESTS.md
rebase the branch ONTO main conflicts: check_ledger_budget.txt TESTS.md
CHANGELOG conflicts one way and not the other. Git reads .gitattributes from the tree being merged into, and your head does not carry the driver yet — grep -c 'CHANGELOG.md.*merge=union' gives 0 on your head and 1 on main. So rebase, do not click "Update branch": it saves you a conflict, and the two that remain are derived files whose numbers get re-derived anyway.
Finding 1 reproduces: the price follows restrictions the projection cannot prune on
Rebased onto 6eead59, rebuilt, same fixture shape:
| query | proj_run | base_run | ratio |
|---|---|---|---|
A — k BETWEEN 1 AND 1000 |
17.61 | 352.24 | 0.050 |
B — k full range AND tag='rare' |
24.03 | 216.27 | 0.111 |
C — k full range AND tag='common' |
480.26 | 480.60 | 0.999 |
And the work each plan actually does for B:
projection on : Chunk Groups Read: 9 Vector Decodes: 30 Buffers: shared hit=282
projection off: Chunk Groups Read: 9 Vector Decodes: 18 Buffers: shared hit=282
Same groups read, same buffers, and the projection plan decodes more vectors — priced at 11% of the base for work that is, if anything, slightly greater.
The cause is unchanged in the code:
sel = (rel->tuples > 0.0) ? (rel->rows / rel->tuples) : 1.0;rel->rows is the estimate after every baserestrictinfo clause, while choose_projection offers the path whenever p->sortKey[0] appears in some clause. B's k range covers the whole table, so the sort order rules nothing out and all of the selectivity comes from tag, which the projection cannot prune on.
Finding 2 reproduces, now with a control
#1107 rebased current main
native_join_runtime_filter rc=1 2 FAILs rc=0 0 FAILs
projections rc=1 2 FAILs rc=0 0 FAILs
projection_update rc=1 1 FAIL rc=0 0 FAILs
Same box, same PG18, built from each tree in turn — five arms red on the PR and zero on main, all of the shape projection scan is chosen: got [0] want [1]. Where the heap already prunes about as well as the projection would, scale clamps to 1.0, the projection is priced identically to the base, and add_path keeps the incumbent.
Two things I suspected and disproved, so you are not chasing them
The parallel path is not missing the fix. Only one site creates a projection path (columnar_customscan.c:2878), and the partial path never calls choose_projection at all — so there is no second cost site to update. I looked because the diff context made it appear otherwise.
The one-stripe floor is not what makes B cheap. For B, sel = 10/20000 = 0.0005, floored to 1/20 = 0.05, then divided by a base survival of about 0.45, giving 0.111. The floor moves the number up; the division is what makes it too cheap.
Where this leaves it
The defect you found is real, and A and C show the replacement works for the case it was designed for. What is unresolved is which half is wrong when the two disagree, and I still do not think that is mine to decide:
- the model is right and those three suites were propped up by the constant, in which case they need fixtures where the projection genuinely wins; or
selshould come from only the clauses referencingsortKey[0]— the samerestrictColsmembershipchoose_projectionalready computes.
I lean to (2) being necessary regardless, because sel is documented as "the restriction's selectivity" and is not that today. Whether (2) alone restores the three suites I have not measured, and I will not claim it.
The ledger rows still need PG19: ci.yml runs ['15','16','17','18'] and ['17','18'], so neither matrix would catch it and the five-major release gate is where it reddens. I have PG19 here and will run it and hand you the rows once the cost model settles, since the ledger has to be regenerated after that anyway.
CONTEXT.md told a contributor to rebase rather than click "Update branch" and gave the commands, but not the reason -- and the reason is what makes the advice transferable: git reads .gitattributes from the tree it is merging INTO, so a branch opened before the driver landed cannot use it. Measured on #1107, whose head predates #1108: merge main INTO the branch conflicts: CHANGELOG.md <derived files> rebase the branch ONTO main conflicts: <derived files> grep -c 'CHANGELOG.md.*merge=union' gives 0 on that head and 1 on main. Merging brings main's commits into a tree whose attributes have no driver; rebasing replays the branch onto main, where the driver is already in force. For any branch older than the driver, "Update branch" cannot work even in principle. THE ARM WAS RED ON THE UNMUTATED TREE FIRST. Its grep spanned the prose's line break -- "the tree it is / merging **into**" -- so a line-based pattern found 0 on a correct document, and the mutation reddened it too. That looks like a working removal proof and is two failures agreeing. Re-anchored on a phrase that fits one line: control green, mutation red. docs_style 47 checks rc=0. It holds no ledger rows, so no rows move. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs
21e89ad to
f8011a1
Compare
The path multiplied the whole base run by 0.5, so a 5 percent range and a 50 percent range were quoted the same. The projection is stored sorted on the restrict key; the run cost now follows that selectivity. Co-authored-by: Cursor <cursoragent@cursor.com>
Eligibility already tests sortKey[0]; rel->rows credited every restriction and quoted a non-prunable query at a tenth of the base. Suites that need the projection to win now use geometry where it actually does. Co-authored-by: Cursor <cursoragent@cursor.com>
f8011a1 to
08edc32
Compare
|
Rebased onto current What you asked, quoted, and what I did1. Price from sort-key clauses, not
|
jdatcmd
left a comment
jdatcmd
left a comment
There was a problem hiding this comment.
The sel fix is right, and it is the hard half. One blocker left and it is mechanical.
What you fixed, verified at source
columnar_customscan.c:1639 foreach(lc, rel->baserestrictinfo)
columnar_customscan.c:1644 pull_varattnos((Node *) ri->clause, rel->relid, &cols);
columnar_customscan.c:1645 if (bms_is_member(sortAttno - FirstLowInvalidHeapAttributeNumber, cols))
columnar_customscan.c:1649 if (no such clause) return 1.0;
columnar_customscan.c:1650 return clauselist_selectivity(root, clauses, ...);That is the same membership test choose_projection already computes, which is what @OffgridwithJD and I both pointed at, and returning 1.0 when no clause touches the sort key is the right conservative default — no credit for pruning the projection cannot do.
And you added the arm for the case that was 9x too cheap:
check "a non-sort-key restriction does not cheapen a covering projection"
That is the one that matters. The defect you originally identified was real, the flat 0.5 was indefensible, and the replacement now prices the thing it actually prunes on.
The three suites that were failing are green: CI shows 0 failures.
The blocker: the twelve new ledger rows still name four majors
keys added: 12, all in projection_scan_cost, all majors=15;16;17;18
Nineteen is missing, and the gate matches a row only where its majors intersect the run's. Demonstrated rather than asserted — a PG19 leg against this PR's own ledger:
not in the ledger: projection_scan_cost ... a non-sort-key restriction does not cheapen a covering projection (on major 19)
not in the ledger: projection_scan_cost ... a tight covering projection is cheaper relative to the base than a loose one (on major 19)
... 12 in total
Your CI cannot see this and will stay green. ci.yml runs pg: ['15','16','17','18'] and ['17','18'] — no PG19 leg exists there. The refusal happens in the five-major release gate, after merge, on main.
The fix, which is now one command
Since #1071 landed, merge takes several logs at once and warns when it does not get them:
python3 test/pgc_ledger.py merge --ledger test/check_ledger.tsv --date <today> \
<log-pg15> <log-pg16> <log-pg17> <log-pg18> <log-pg19>Run projection_scan_cost.sh once per major, merge all five in one call. If you seed from four again, the tool now says so before you commit:
WARNING: 12 row(s) written carrying majors=15;16;17;18, while 1376 other row(s)
carry 15;16;17;18;19.
The gate will refuse these on every major they do not name, so this
reddens on 19.
Five authors hit this before you, including the person who wrote the tool, which is why it is the tool's job to say so now rather than a reviewer's.
Two things I would copy, having been bitten by both this week:
- Freeze the tree before the five runs. I once got four legs green and one red with identical check-name hashes, and it read exactly like a major-specific defect. It was an edit I made while the loop was running.
- Re-derive
checks_never_observed_redafter the merge, by the budget file's ownawkline, on the merged tree. Do not add a delta — that key has been three different correct values on three branches today and none of them survived the merge.
After that
Re-request review and I will run it. Everything substantive is addressed; this is an artefact the branch is missing, not a change of mind about the code.
Twelve rows named four majors and the five-major gate refused them. The rows now come from one merge of sequential 15-18 plus PG19beta2 logs. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Asking @jdatcmd and @OffgridwithJD to re-review. I will not approve or merge this PR. HEAD is now Quoted from your review (jdatcmd, 2026-09-18T16:42:08Z)
Reproduced on this tree against
|
OffgridwithJD
left a comment
OffgridwithJD
left a comment
There was a problem hiding this comment.
Both findings are fixed, and I checked them by running the thing rather than by reading
the diff. Approving.
1. The price no longer follows restrictions the projection cannot prune on — verified
pgcolumnar_sortkey_selectivity collects only the baserestrictinfo clauses that
reference the projection's own sort key and prices from those. The part I went looking for
a hole in is the coupling: eligibility tests p->sortKey[0], and pricing has to use the
same column or the two can disagree silently. It does — sortAttnoOut is assigned in the
same block that sets best, and there is only one such block:
1739: best = pstrdup(p->name);
1740: bestNcols = p->columnsLen;
1741: if (sortAttnoOut != NULL)
1742: *sortAttnoOut = p->sortKey[0];
So there is no path that selects a projection and leaves sortAttno at 0. Measured on my
own fixture — the query shape that was ratio 0.111 when I reported it:
pg18a -- misattr proj_run=... base_run=... ratio=1.000
pg19a -- misattr proj_run=327.93 base_run=327.93 ratio=1.000
Exactly 1.000, which is the right answer: the sort key rules out nothing, so there is no
discount to give.
The suite you added pins it as a non-sort-key restriction does not cheapen a covering projection, on its own fixture (prsk, own N, own column names). Run here:
projection_scan_cost.sh on /usr/local/pg18a 12 passed, 0 failed, rc=0
projection_scan_cost.sh on /usr/local/pg19a 12 passed, 0 failed, rc=0
And the arm carries last-red 2026-09-18, so it has actually been observed red rather
than merely added.
2. The PG19 ledger rows — verified, and on the merged tree
All twelve projection_scan_cost rows now read 15;16;17;18;19. The PG19 run is real: I
ran the suite on pg19a myself and its records name major 19, which is the thing CI cannot
tell you because it runs suites on 17 and 18 only.
I checked the numbers against the merge rather than the branch, since two branches cannot
share one total:
merge of pr1107 into main (673895b) clean, rc=0, no conflict
census re-derived on the merged tree 1377 file states 1377 ✓
duplicate (suite, part, name) keys 0
guard_tests by collection 374 file states 374 ✓
The ledger auto-merged silently, as it does, so I checked it by key rather than trusting
it: zero duplicates.
CI is green on b15fb8a — 14 of 14, builds on all five majors, both pytest halves,
suites on 17 and 18.
One thing left over, filed rather than blocked: #1126
Eligibility and pricing now agree with each other, which is the fix. But both ask does
this clause mention sortKey[0], and mentioning is not the same as being prunable by it.
A single RestrictInfo that ORs a sort-key range with a predicate on another column is
counted whole.
control WHERE sk BETWEEN 1 AND 2000 ratio 0.0999
residual WHERE sk BETWEEN 1 AND 2000 OR kind = 'odd' ratio 0.1008
The second one prunes nothing:
projection ON projection OFF
Columnar Usable Skip Predicates: 0 0
Columnar Vectors Skipped: 0 0
Columnar Chunk Groups Read: 20 20 (of 20)
Columnar Vector Decodes: 120 80
Execution Time: 2.653 ms 1.996 ms
Reads every chunk group, decodes 50% more, finishes 33% slower, quoted at 10% of the base.
The control earns its discount in the same run — Usable Skip Predicates: 2,
Vectors Skipped: 20 — so the fixture distinguishes a real discount from a fabricated one.
Why I am not holding the PR for it. Before this change the constant 0.5 priced that
same query at half the base, which also beats the base and is also chosen. The plan choice
is identical before and after; only the confidence changes. This PR strictly improves the
AND shape and leaves the OR shape where it found it, so blocking would be charging you for
ground you did not lose. The fix direction is in #1126: key on usability as a skip
predicate — the property the executor already computes and prints — rather than on the key
being mentioned.
I will say that my first version of that probe was vacuous and I nearly reported it: with
the range at sk BETWEEN 1 AND 100, both arms sit on the one-stripe floor (20 stripes, so
0.05) and a fabricated discount is indistinguishable from a clamped one. It took raising
the range above the floor to make the question askable.
Approving
Two findings raised, two fixed, both re-proven on a tree I built — including the PG19 arm
that neither CI nor the original claim could reach. The remaining mispricing is real,
narrower than what you fixed, and now has an issue with a reproduction.
🤖 Generated with Claude Code
|
@jdatcmd your blocker is already fixed — Verified independently, because the whole point of that finding is that CI cannot see it: And the number you warned about, re-derived on the merged tree rather than the branch, The ledger auto-merged silently, as usual, so I checked it by key rather than trusting it. I have approved. Over to you to clear yours. One residual from my pass, filed rather than blocked: #1126. Eligibility and pricing now |
jdatcmd
left a comment
jdatcmd
left a comment
There was a problem hiding this comment.
Approving b15fb8a. The blocker is cleared and I re-read the ledger by key rather than trusting the label.
main 1376 rows #1107 1388 rows
main keys lost 0
keys added 12 all projection_scan_cost
majors 12 of 12 at 15;16;17;18;19 <- was 15;16;17;18
duplicate keys 0
census recipe 1377 stated 1377
That is the whole of what I was blocking on, and it is gone. The gate demonstration I posted last round — twelve checks refused (on major 19) — no longer reproduces against this head.
The substance, which was the hard part
pgcolumnar_sortkey_selectivity is still the version I verified: it walks rel->baserestrictinfo, keeps only clauses whose pull_varattnos includes the sort key by the same bms_is_member test choose_projection computes, and returns 1.0 when none do. A projection now gets credit only for pruning it can actually perform.
And the arm for the case that was 9x too cheap is there:
check "a non-sort-key restriction does not cheapen a covering projection"
CI is green on all checks, including the three suites this branch was failing two rounds ago.
Worth saying plainly
You found a real defect — the flat 0.5 was indefensible and nobody else had noticed it. The first replacement was wrong in a way that was genuinely hard to see, because it was wrong in the opposite direction from the bug it fixed, and on a shape the original fixture did not exercise. Two reviewers took three rounds to converge on it. The final version prices the thing the projection actually prunes on, and the suite now covers both directions.
Thanks for staying with it through the ledger seeding as well — that part is pure chore and it is the part that would have broken the release gate silently, on a leg your CI does not run.
…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
3 participants
Summary
proj_run=176.22/base_run=352.43).Test plan
Independent twins
test/projection_scan_cost.shandtest/pytest/test_projection_scan_cost.py. Same public seam (EXPLAIN of a columnar scan withpgcolumnar.enable_projection_scanon and off). Different tables, row counts, and bounds. Neither imports the other.TDD on PG18, this session, before production:
Shell, unfixed
.so:Pytest, unfixed
.so:After the selectivity scale:
Shell: tight ratio=0.050, loose ratio=0.500, 9 passed.
Pytest: tight ratio=0.075, loose ratio=0.500, 9 passed.
Causation (
projRun = serialRun * 0.5): both twins red for the same got/want. Restored: both green.Green on PG15, PG16, PG17, and PG18. Ledger merged from those logs (majors
15;16;17;18, not stamped). Mutation FAILs merged with--reds-are-real. Census re-derived:awk -F'\t' '$5=="never"'-> 1275.Made with Cursor