fix: price a covering projection scan from its own pages - #1155
Conversation
|
TDD excerpts from this session. Prior chat summaries were not used as evidence. Start SHA Shell
|
OffgridwithJD
left a comment
OffgridwithJD
left a comment
There was a problem hiding this comment.
Your test work is verified, not read. I ran your removal proof here and it reproduces
exactly the numbers your description claims:
CONTROL cover_run=19996.0 rel_pages=42.0 base_io=42000.0 ratio=0.476 .so 245cd2b659f1
MUTATED cover_run=41991.6 rel_pages=42.0 base_io=42000.0 ratio=1.000 .so 7fcfd0357aba
a covering projection is not priced from the base table's pages:
got 'base-pages' want 'proj-pages'
Different .so per cell, so each measured its own binary. Six ledger rows across all five
majors, both twins with matching names and neither reading the other, cluster_tests
436 → 437 which matches a collection here. Everything you were asked for last time is
present without being asked again.
One blocker, in the C rather than the tests, and it is four lines.
A projection the lookup cannot find is priced at one page
if (projSid == 0)
return 1;That is a fail-OPEN default. One page is essentially free, so ioProj collapses to
nothing, the covering path becomes the cheapest thing available, and the planner takes it
— on the strength of a lookup that just failed. The direction is exactly backwards: a
lookup failure should make the path look expensive, not free.
It should not happen, I agree — the path is only built when a covering projection was
found. But "should not happen" is the state that gets reached by a route nobody modelled,
and the cost of being wrong here is silent: the plan changes, nothing errors, and the
number that caused it is unreachable from SQL. rel->pages as the fallback keeps the old
behaviour for that case and cannot make the path look better than the base scan; an
elog(ERROR) would also be defensible since you believe it unreachable. Either is fine —
returning 1 is the one I would not ship.
Same shape one line down:
if (pages < 1)
pages = 1;That one I would keep: a real projection occupying less than a page is genuinely close to
free, and the clamp is arithmetic rather than an error path. Worth a word in the comment
saying the two 1s mean different things, because they read identically.
A question rather than a blocker: this scans every row group at plan time
pgcolumnar_projection_pages calls PgColumnarReadRowGroupList and sums every group, on
every planning of a relation with a covering projection. There is explicit precedent
against that in the tree, in pgcolumnar_written_stripe_row_limit:
The exact quantity is the real group count, and reading it is a scan proportional to the
number of groups on every plan, which is too much to spend refining a term that is
approximate by construction.
Your case is not identical — that comment is about refining an approximate term, while
this is the I/O estimate itself, so the trade may well be worth it. But the two decisions
now point opposite ways in the same planner for the same reason, and whoever reads them
next deserves to know that was considered rather than missed. I would put a sentence in
your function's header saying why this one earns the scan, and I would want @jdatcmd's
view on the planning cost on a table with many row groups, since that is an owner call
rather than mine.
Smaller
Your description says cluster_tests 437 "by collection on current main (18821ce)". 437
is on your branch; main is 436. The number is right and the derivation is right — only the
sentence attributes it to the wrong tree, and I mention it because a reader checking your
work against main will find 436 and wonder which of you is wrong.
CI is UNSTABLE, 2 of 14 outstanding, so I would not have approved this round regardless.
Fix the fallback and I will re-run the proof against the new head.
|
Your two red legs are one defect, and the fix will change under you unless you rebase first. Both, plus the numbers you will need, below. The two failures are the same assertionThe second leg runs the guard half in a subprocess and asserts it exits 0. It did not, because of the first. So there is one thing to fix, not two — the cluster leg goes green on its own when the guard leg does. Worth saying because two red legs naturally read as two problems, and chasing the second one leads nowhere. The defect itself: But do not fix the number yet — rebase first
So on current Your branch carries cd test/pytest
F="$(python3 -c 'import sys, pathlib; sys.path.insert(0, "."); from test_harness_deps import NO_CLUSTER;
print(" ".join(p.name for p in sorted(pathlib.Path(".").glob("test_*.py")) if p.name not in NO_CLUSTER))')"
PYTHONPATH=. pytest --collect-only -q --pg-config <your pg_config> $F | tail -1Re-derive Your new section will be 70, and it needs both the heading and the contents entry — that is the pair the guard checks. Running the guard corpus locally, which would have caught bothNeither leg is reachable from python3 -m venv /tmp/pgcvenv
/tmp/pgcvenv/bin/pip install -q $(grep -E '^(pytest|pytest-xdist)==' test/pytest/requirements-test.txt)
cd test/pytest
G="$(python3 -c 'import sys; sys.path.insert(0, "."); from test_harness_deps import NO_CLUSTER; print(" ".join(NO_CLUSTER))')"
PYTHONPATH=. /tmp/pgcvenv/bin/pytest -q --pgc-expect-tests "$(awk '$1=="guard_tests"{print $2}' expected_tests.txt)" $GThat needs no cluster and no driver, and it runs in about twelve seconds. It reproduces the first failure exactly. The C blocker is unchanged@OffgridwithJD's Your test work was verified rather than read — they reproduced your removal proof with a different 🤖 Generated with Claude Code |
|
Updating the numbers in my previous comment —
So your new section is 71, not 70, and the The diagnosis itself is unchanged and is still the whole of what is wrong: Fix the contents entry and both legs go green. Derive the counts with the recipe rather than from my figures — I have now had two sets go stale inside an hour, and that is exactly why the file says re-derive rather than carry. 🤖 Generated with Claude Code |
a2f125a to
211b887
Compare
|
@OffgridwithJD @jdatcmd rebased locally onto current What you asked for
Also: say why the plan-time row-group walk earns the scan that
CI reproduced that as Fallback TDD (PG18, this session, before the C change)Unfixed ( Shell: Pytest: After Pytest the same Happy-path arm unchanged: covering run 19996 / base_io 42000 / ratio 0.476. Derived on this tree, not copiedNew ledger rows majors Please re-review. Not merged. Not self-approved. |
|
Guard CI on Reproduced locally as jd. That sweep landed on main after this PR opened; two-constant IfExp arms collapse the ratio. Passing branches keep the named verdict ( Causation: restore Head is |
|
Suites PG17/PG18 failed after the pytest carry-measurement push. Reproduced: those three awk ternaries were two-constant. Passing branches stay Head is |
OffgridwithJD
left a comment
OffgridwithJD
left a comment
There was a problem hiding this comment.
Adversarial re-review of 8e19ba4, built and run in the audit container on PG 18.4 (assert build, /usr/local/pg18a).
The fix itself survived every attack I made on it. The arithmetic is right, and I can now show it is right to three decimals rather than say so.
The suite is green and the number it produces is exactly correct
-- cover_run=19996 rel_pages=42 base_io=42000 ratio=0.476
-- proj_bytes=56366 rel_bytes=344064
accounting: 9 passed + 0 failed + 0 unrunnable + 0 skipped = 9
I asked the catalog what the answer should be, using the same per-row-group rounding pgcolumnar_projection_pages uses:
SELECT count(*), sum(rg.byte_length),
sum(ceil(rg.byte_length::numeric / (8192-24)))
FROM pgcolumnar.row_group rg
JOIN pgcolumnar.projection p ON p.proj_storage_id = rg.storage_id
JOIN pgcolumnar.storage s ON s.storage_id = p.storage_id
WHERE s.relation_oid = 'psio'::regclass AND p.name = 'byik';proj_rowgroups=20 proj_bytes=56366 proj_pages_rounded=20
rel_pages=42
So the projection occupies 20 of the relation's 42 pages, 20/42 = 0.47619, and the planner quoted 0.476. The formula is not approximately right, it is exact.
That also answers something the suite gets wrong by implication. proj_bytes/rel_bytes is 0.164, and the premise arm uses that bytes ratio. The cost model does not use bytes, it uses pages rounded up per row group, and those differ by 2.9x here: 56366 bytes is 7 pages of payload but 20 pages of storage, because each of the 20 row groups is rounded up to a whole page. The bytes ratio is the wrong denominator. It is fine as a "minority" premise, but a reader will take it as the expected value, and it is not.
The finding: both verdict arms are ceilings, not bands
Same shape I proved blind on #1180. Each arm accepts an unbounded range on one side:
print (r+0 > 0.8) ? "base-pages ratio=" r : "proj-pages" # passes for any r <= 0.8
print (r+0 < 0.2) ? "one-page miss_ratio=" r : "not-one-page" # passes for any r >= 0.2Proven, not asserted. I mutated pgcolumnar_projection_pages on your branch so that it undercounts in both directions:
if (projSid == 0)
return fallbackPages / 3; /* was: fallbackPages */
...
pages = pages / 2; /* inserted */
if (pages < 1)
pages = 1;Rebuilt (.so f893ec4fcb8d, restored afterwards, git status --porcelain clean) and re-ran:
-- cover_run=9998 rel_pages=42 base_io=42000 ratio=0.238 (was 0.476)
-- miss_run=13997.2 base_io=42000 miss_ratio=0.333 (was 1.000)
accounting: 9 passed + 0 failed + 0 unrunnable + 0 skipped = 9
A projection priced at half its real page count, and a lookup failure priced at a third of the base file, both pass. The suite pins "smaller than the base file", which is the symptom, not "priced from its own pages", which is the claim in the PR title.
The suite already holds everything needed to fix this. It queries proj_bytes; the same query with ceil(byte_length/(BLCKSZ-24)) gives 20, which is the page count the cost model uses.
I would not build the band out of rel->pages, because #1180 changes what that is. Both arms have an oracle that does not mention it:
- Covering arm:
cover_runshould beseq_page_cost * proj_pages * sel, so withselat 1 the expected run cost is20 * 1000 = 20000against the measured19996. A 5% band reddens at my mutated9998and survives #1180 untouched. - Miss arm: the expected value is "the same as the base scan", which the suite can measure directly by running the same query with
pgcolumnar.enable_projection_scan = offand comparing the two run costs. That is the property the comment states, and it stays true howeverrel->pagesis computed.
Note that miss_ratio > 0.8 against the current base_io would be wrong for exactly this reason: once #1180 lands, the fallback returns 22 rather than 42 and that band would redden on a correct tree.
The other thing I attacked, and did not find
- Page arithmetic.
COLUMNAR_PAGE_ROUND_UProunds toCOLUMNAR_BYTES_PER_PAGEand the division uses the same constant, so the round-up and the divisor agree. No off-by-one. - The early return.
table_closeruns beforeif (projSid == 0) return fallbackPages, so the lookup-failure path holds no lock. selversusscale. The I/O term scales by rawseland the CPU term bysel/baseSurvival. I convinced myself this is deliberate: the projection's pages are not survival-discounted, so the raw selectivity is the right multiplier for them. It is worth one sentence in the comment, because the asymmetry reads as a slip.
Cross-PR: the same walk now exists twice
pgcolumnar_projection_pages here and pgcolumnar_sibling_projection_pages in #1180 (src/columnar_tableam.c) carry the identical inner loop:
rgs = PgColumnarReadRowGroupList(<sid>, snapshot);
foreach(...)
bytes += COLUMNAR_PAGE_ROUND_UP(rg->byteLength);
return (BlockNumber) (bytes / COLUMNAR_BYTES_PER_PAGE);They are complements of one number: #1180 subtracts every projection from rel->pages, this one adds one projection back. If both land, a change to the rounding in one file is a wrong answer in the other, and nothing in either tree ties them together. One shared helper would close it.
One consequence, derived rather than run, so treat it as a question: once #1180 lands, rel->pages for this fixture becomes 42 - 20 = 22, while this suite's base_io stays at pg_relation_size/8192 * 1000 = 42000. The arms still pass (0.476 and 0.524 clear their thresholds), but base_io stops being the planner's base I/O, and the comment "covering I/O is ... ~= 1.0 of base_io" stops describing the tree. A band would make that drift visible; the ceiling hides it.
Closing my own open question from 2026-09-19
I asked for @jdatcmd's view on planning cost with many row groups. I have since measured the same shape on #1180: at 400 base and 200 projection row groups the median planning time was 1.051 ms on main and 1.068 ms on the branch, which is a null result at that size. That was #1180's walk, not this one, so it transfers only as far as the shape does. I am no longer holding the PR on it.
Still CHANGES_REQUESTED, for one reason: turn the two ceilings into bands. The fix is right, and the suite should be able to say so.
Container pgcolumnar-audit, PG 18.4 assert build. Unmutated .so under test, mutated f893ec4fcb8d, source restored and verified clean after each arm.
8e19ba4 to
de6206a
Compare
|
@OffgridwithJD @jdatcmd rebased locally onto current What you asked for
Shell PG18: Causation ( Pytest twin: 9 pass. Re-derived: Please re-review. Not merged. Not self-approved. |
OffgridwithJD
left a comment
OffgridwithJD
left a comment
There was a problem hiding this comment.
Re-review of de6206a. The band works. I re-ran the exact mutation that proved the old arms blind, and it now reddens both of them.
The same mutation, before and after
pgcolumnar_projection_pages undercounting in both directions — pages / 2 and fallbackPages / 3:
| previous head | de6206a |
|
|---|---|---|
| result | 9 passed + 0 failed |
7 passed + 2 failed |
| covering arm | passed at ratio 0.238 | off-band got=9998 want=20000 |
| miss arm | passed at miss_ratio 0.333 | moved miss_ratio=0.333 |
Control on the same build, unmutated: 9 passed + 0 failed, cover_run=19996 against want_run=20000, miss_ratio=1.000. Source restored, git status clean.
That is the whole finding closed. The arms now pin the claim in the PR title rather than the symptom.
And they are robust to #1180, which I had flagged as the open interaction
This is the part I want to record, because my previous review asked for a band and warned that the obvious band would break when #1180 lands. Both arms avoid it:
proj_pages = sum(ceil(rg.byte_length / (8192 - 24))) -- from the catalog
want_run = proj_pages * seq_page_cost
base_run = run_of "$base_plan" -- MEASURED, not pg_relation_size
want_run never mentions rel->pages, so #1180 changing it cannot move the covering arm. And the miss arm now compares against the base scan's own measured run cost instead of pg_relation_size/8192 * page_cost, so when #1180 shrinks rel->pages both miss_run and base_run shrink together and the ratio stays at 1. A band built on the file size would have gone red on a correct tree; this one does not. That is the better of the two fixes available and it is the one you took.
The page rounding matches the C exactly — ceil(byte_length / (BLCKSZ - SizeOfPageHeaderData)) per row group, which is why proj_pages=20 and not the 7 that proj_bytes/8192 would give. The bytes ratio is still printed as context and no longer carries the verdict, which is the right division.
One note, not a request
premise: the covering projection occupies a minority of the relation still uses proj_bytes < rel_bytes * 0.7. That is fine as a premise — it exists to stop the two formulae agreeing by accident when the projection fills the file — but it is now the only place the bytes ratio appears, and a reader may take 0.164 as the expected value when the arm below wants 0.476. One clause saying the premise is about bytes and the verdict is about rounded pages would prevent that. Entirely optional.
Holding the approval for one reason, and it is procedural
mergeStateStatus=DIRTY — #1191 and #1193 merged under this branch. CI is 15/15 SUCCESS, the content is verified, and I have no outstanding objection.
I am not approving a head that is about to be rebased away. I learned today that gh pr review --approve binds to whatever HEAD is current, and that this repository does not dismiss stale reviews: an approval I place now would survive the rebase and stand on commits I have not read. Rebase onto main and I will approve on the new head the same day — the substance is settled.
Verified in container pgcolumnar-audit, PG 18.4 assert build, at de6206a.
de6206a to
d281557
Compare
|
@OffgridwithJD @jdatcmd rebased locally onto current What you asked for@OffgridwithJD (adversarial re-review of
Those bands were already on the prior head ( Derived on this tree, not copiedGreen on the reseated tree (PG18, this session)Shell: Pytest: Please re-review. Not merged. Not self-approved. |
jdatcmd
left a comment
jdatcmd
left a comment
There was a problem hiding this comment.
Approving d281557. @OffgridwithJD verified the costing substance in depth and said the only thing holding their approval was a head about to be rebased away. That rebase has happened: the merge base is 133c3fb and the PR reads MERGEABLE/CLEAN, 15 of 15. I have checked the parts that are mine to check rather than re-running their review.
One thing is live and I proved it rather than predicted it
cluster_tests 464 will merge silently and be wrong. #1180 merged twenty minutes ago and took main from 463 to 464 for its own new test. Your branch says 464 too, derived correctly against 133c3fb when 463 was the truth. Same value on both sides now, so git does not speak.
Composed on a real worktree, main at b986d8d merged with this branch:
the merged file says cluster_tests 464
the composed tree has 47 cluster files
collection reports 465 tests collected
And the merge is worse than plain silence. expected_tests.txt DID conflict, at lines 473-482, in the COMMENT block. The value at line 483 sat outside the conflict and auto-merged at 464. A reader resolving that conflict is looking at prose, one line above a number that is now wrong and that git has already declared resolved.
checks_never_observed_red 1460 will conflict loudly against main's 1461, which is the safe half of the same shape.
So before this merges: rebase onto b986d8d and re-derive cluster_tests by collection. Expect 465. Nothing in the change is wrong; the number was right for the base it was derived against and is stale by exactly one merge. I am approving rather than blocking because it is one command and not the change under review.
The same trap is live on #1127 and #1198, which also say 464. #1196 says 465, which is a DIFFERENT value and therefore conflicts loudly. That is worth saying out loud: the PR whose number disagrees is the safe one.
One comment that stopped being accurate twenty minutes ago
* fallbackPages is rel->pages. A lookup failure must not make the path look
* cheaper than the base scan; returning 1 would.
#1180 changed what rel->pages IS. pgcolumnar_relation_estimate_size now subtracts every sibling projection's pages from it, and pgcolumnar_sibling_projection_pages sums all projections of the base storage, including this covering one. So after both land, fallbackPages is the base-only page count rather than the whole relation file.
The guarantee still holds -- a fallback of base-only pages still does not undercut the base scan -- so this is not a defect and I am not asking for a code change. But the sentence now describes a different quantity than it did when it was written, and the next person reading it will reason from the whole file. One clause naming which of the two it is.
What I checked, that is mine rather than a re-run of the other review
| claim | how |
|---|---|
| the plan-time row-group walk is paid only when a covering projection is in play | the call sits inside the block where projName is already bound; a plan with no projection reaches it never |
| the walk is justified rather than inherited | the comment says why this one earns what pgcolumnar_written_stripe_row_limit declines, which is the right place to say it |
| the lookup-failure path cannot make the covering path win | it returns fallbackPages, and the pages < 1 floor below is labelled as arithmetic rather than the same case |
| the pair is declared | test_compare_to_bash.py gained the entry, which is the guard #1198 is currently red on |
And the note @OffgridwithJD left you
premise: the covering projection occupies a minority of the relation is the only place the bytes ratio still appears, and a reader can take 0.164 for the expected value when the arm below wants 0.476. They called it optional and I agree, but if you are touching the file for the rebase anyway it is one clause.
OffgridwithJD
left a comment
OffgridwithJD
left a comment
There was a problem hiding this comment.
Holding CHANGES_REQUESTED, and this time on a measured failure rather than a design point. #1180 has landed on main, and composed with it this suite goes red. The fix is one line and I have proved it.
The composed tree fails
I merged this branch with current main (c4f1c51, which carries #1180) and asserted the compose was real before measuring anything:
merge finished: yes
conflict markers: 0
main's #1180 present: 3
#1155's band present: 2
#1155's fn present: 3
Then:
-- cover_run=19996 proj_pages=20 want_run=20000
-- base_run=22000
-- miss_run=41991.6 base_run=22000 miss_ratio=1.909
FAIL a covering projection whose storage cannot be found is not priced as one page:
got [moved miss_ratio=1.909] want [not-one-page]
accounting: 8 passed + 1 failed + 0 unrunnable + 0 skipped = 9
The covering arm is fine. The miss arm is the casualty.
The mechanism: the ratio spans two different catalog states
82 base_plan="$(explain_scan off "$SQL")" <- projection INTACT
116 UPDATE pgcolumnar.projection SET proj_storage_id = 0 ...
123 miss_plan="$(explain_scan on "$SQL")" <- projection NOT FINDABLE
124 miss_ratio = miss_run / base_run
base_run is captured at line 82 while the projection is still findable, so #1180's relation_estimate_size subtracts its 20 pages and the base scan is priced from 22. The UPDATE at 116 then clears proj_storage_id, which is the same key #1180's pgcolumnar_sibling_projection_pages walks — so at line 123 there is no sibling to subtract and the scan is priced from the full 42. The ratio divides a 42-page number by a 22-page one and gets 1.909, which your new band correctly rejects.
Before #1180 this was invisible, because both catalog states gave rel->pages = 42 and the ratio was exactly 1.000. The mutation and its own denominator only diverged once something else started reading the field the mutation changes.
The fix, proved rather than suggested
Re-measure the denominator in the same state as the numerator:
base_plan_miss="$(explain_scan off "$SQL")"
base_run="$(run_of "$base_plan_miss")"
miss_plan="$(explain_scan on "$SQL")"On the same composed tree:
-- miss_run=41991.6 base_run=42000 miss_ratio=1.000
accounting: 9 passed + 0 failed + 0 unrunnable + 0 skipped = 9
Nothing else moves. The covering arm, the band and every premise are untouched.
Why I re-ran instead of trusting the rebase
I had reasoned on your previous head that the band was safe from #1180 because base_run is MEASURED rather than derived from pg_relation_size. That reasoning was right about the covering arm and wrong about the miss arm, and no amount of reading would have told me which. The branch's own code did not move; the thing underneath it did. A patch-identity check cannot see that — only running it on the composed tree can.
For the record, my first attempt at that compose was worthless and I nearly reported from it: the rebase died on the first commit, left a conflict marker, and the suite that ran was main's, printing base_io with no want_run at all. The assertions above exist because of that.
Everything else stands: the band reddens my blindness mutation at 7 passed + 2 failed against a 9/9 control, proj_pages uses the C code's own per-row-group rounding, and the covering arm never mentions rel->pages. Make the denominator match its numerator and I approve on sight.
Composed against c4f1c51 in container pgcolumnar-audit, PG 18.4 assert build.
jdatcmd
left a comment
jdatcmd
left a comment
There was a problem hiding this comment.
Withdrawing my approval of d281557. This branch is green alone and RED composed with main, and I reproduced it independently before saying so.
@OffgridwithJD found it. I built the compose myself rather than taking the report, because I am the one who approved this.
The composed run
main at c4f1c51 (which carries #1180) merged with this branch. The four conflicts are all bookkeeping files; neither src/ nor the suite conflicts. Both changes are present in the tree I measured (pgcolumnar_projection_pages in columnar_customscan.c, pgcolumnar_sibling_projection_pages in columnar_tableam.c), and zero conflict markers remain.
-- cover_run=19996 rel_pages=42.0000000000000000 base_io=42000 ratio=0.476
-- proj_bytes=56366 rel_bytes=344064 proj_pages=20 want_run=20000
-- base_run=22000
-- miss_run=41991.6 base_run=22000 miss_ratio=1.909
FAIL a covering projection whose storage cannot be found is not priced as one page:
got [moved miss_ratio=1.909] want [not-one-page]
8 passed + 1 failed
The mechanism: the ratio spans two catalog states
base_run is captured at line 82, while the projection is still findable. #1180 subtracts its 20 pages there, so the base scan is priced from 22.
The UPDATE at line 116 then sets proj_storage_id = 0, which is the same key #1180's sibling walk reads. By line 123 there is no sibling to subtract, so the scan is priced from the full 42.
42 over 22 is 1.909, and your own band correctly rejects it.
Before #1180 this was invisible. Both states gave rel->pages = 42 and the ratio was exactly 1.000. The mutation and its own denominator only diverged once something else started reading the field the mutation clears.
The comment asserts the property that fails
# Base scan with projection off: oracle for the miss-arm band, independent of
# how rel->pages is computed (survives #1180).
That is right about the covering arm, where want_run is built from the catalog and never mentions rel->pages. It is wrong about the miss arm, whose oracle is a measured plan cost and therefore moves with rel->pages like everything else. Worth rewording along with the fix, because the next reader will trust it.
The fix, proved rather than suggested
Re-measure base_run after the UPDATE, in the same catalog state as miss_run. I applied exactly that to the composed tree:
-- base_run=22000 <- the echo above, before the UPDATE
-- miss_run=41991.6 base_run=42000 miss_ratio=1.000 <- re-measured after it
9 passed + 0 failed
Both numbers in one run are the clearest statement of the defect: 22000 with the sibling findable, 42000 without, and the arm needs the second one.
Why my earlier approval missed it
I checked that your branch was rebased onto main and that its numbers were right, and I reasoned about the fallbackPages interaction in a comment. What I did not do is run the suite against the composed tree. This is the shape where that is the only thing that works: your code did not move, the thing underneath it did, and no reading of either diff shows it. Two PRs, green individually, red together, with no file in common and no shared line — the coupling is a catalog FIELD, proj_storage_id, that one branch clears and the other reads.
Everything else in my earlier review still stands, including the cluster_tests re-derive, which has moved again: main now says 466.
…ommandprompt#1155) The miss arm divided a plan cost measured AFTER the fixture clears proj_storage_id by one measured BEFORE it. Those are two different catalog states, and commandprompt#1180 made them two different prices. composed with main carrying commandprompt#1180, before this change: -- miss_run=41991.6 base_run=22000 miss_ratio=1.909 FAIL after: -- miss_run=41991.6 base_run=42000 miss_ratio=1.000 9 checks, PASSED THE COUPLING IS A CATALOG FIELD, not a file or a shared identifier. The suite clears `proj_storage_id` to simulate a lost projection; commandprompt#1180's sibling-pages walk READS that same key. So base_run was priced with the projection's twenty pages subtracted and miss_run was priced from the whole forty-two, and 42/22 is 1.909. Before commandprompt#1180 both states gave rel->pages = 42 and the ratio was exactly 1.000, so nothing here could have noticed. The comment claimed the property that failed -- "oracle for the miss-arm band, independent of how rel->pages is computed (survives commandprompt#1180)". That is TRUE of the covering arm, whose want_run is built from the catalog and never mentions rel->pages, and FALSE of this one, whose oracle is a measured plan cost. Both halves now say which arm it is true of. AND THE REORDER BOUGHT NO BLINDNESS, which is the thing moving a read can cost. With `pgcolumnar_projection_pages` mutated to always return fallbackPages: FAIL a covering projection is not priced from the base table's pages: got [off-band got=21995.6 want=20000] want [proj-pages] restored: 9 passed + 0 failed Rebased onto e2638b7 and every tracked number re-derived on the composed tree rather than carried across the rebase: cluster_tests 464 -> 468 collection, 48 cluster files guard_tests 398 -> 403 main's, untouched by this branch checks_never_observed_red 1460 -> 1499 awk over the ledger suites_not_covered 249 unchanged check_ledger.tsv conflicted and was resolved ADDITIVELY, both sides' rows kept, then checked rather than trusted: 1523 rows, zero duplicate (suite, part, check) keys. TESTS.md rebuilt from main with this branch's section re-applied once, so the diff carries no deletions, and its number DERIVED as max + 1: main's highest is 76, so test_projection_scan_io.py is 77 and not the 74 it was written as. 77 sections, 77 TOC entries, 77 of 77 pairing on number AND title, contiguous, zero bad anchors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n
d281557 to
1d127de
Compare
|
Pushed the fix and a rebase to this branch, at the owner's request. This clears my The fixThe miss arm divided a plan cost measured after the fixture clears The oracle is now re-read in the arm's own state, in both halves. The comment that claimed the property which fails -- "oracle for the miss-arm band, independent of how rel->pages is computed (survives #1180)" -- now says which arm that is true of: the covering arm, whose And the reorder bought no blindnessMoving a read is exactly the change that can buy correctness with silence, so I checked it rather than the reorder. With Full strength. Bookkeeping, re-derived on the composed tree
@OffgridwithJD: I wrote this fix and I am also the one clearing the block on it, so the sensitivity check above is the part I would most like you to attack. The question it answers is "does the reordered arm still see the defect", and the question it does not answer is whether there is a third state the ratio could span. |
OffgridwithJD
left a comment
OffgridwithJD
left a comment
There was a problem hiding this comment.
Independent read on 1d127de, since @jdatcmd authored this fix and is also clearing the block. Answering the question they asked, and the answer is yes — there is a third state, though not in the ratio.
The ratio itself now spans one state, and that is right
Both base_run and miss_run are measured after the UPDATE, with no DDL between the two EXPLAINs, each in its own session taking a fresh snapshot. There is no third catalog state for the ratio to straddle. The fix is sound.
But pgcolumnar_projection_pages has three outcomes and the suite reaches two
if (projSid == 0)
return fallbackPages; /* 1: the miss arm */
...
pages = (BlockNumber) (bytes / COLUMNAR_BYTES_PER_PAGE);
if (pages < 1)
pages = 1; /* 3: nothing reaches this */
return pages; /* 2: the covering arm */Path 3 is reached when the lookup succeeds and the row groups sum to less than one page. That is a different state from path 1: there the projection is not findable, here it is findable and nearly empty.
Mutating only that line, pages = 1 to pages = 997:
shell 9 passed + 0 failed (unchanged)
pytest 9 pass + 0 fail (unchanged)
Both harnesses are blind to it. Source restored, git status clean.
This is a coverage gap rather than a defect. The floor is deliberate and the comment says why — a real projection under one page is genuinely near free. But nothing pins it, and the arm named a covering projection whose storage cannot be found is not priced as one page is guarding against the value path 3 produces while exercising only path 1. If the arithmetic above it ever yielded 0 for a projection that is not nearly empty, the floor would hide it and no arm would fire.
Not asking for it here. It is worth one arm eventually: a projection with a valid storage id and no row groups.
The census, derived with a different instrument than yours
You flagged that you derived all three with the same command, which is the right thing to flag. I used two, on three independently cloned trees:
#1127 file 468/403 collected 468/403 gate accepted never 1499 = declared dupkeys 0
#1155 file 468/403 collected 468/403 gate accepted never 1499 = declared dupkeys 0
#1198 file 468/403 collected 468/403 gate accepted never 1494 = declared dupkeys 0
Instrument A is pytest's own N tests collected; instrument B is --pgc-expect-tests, which refuses a wrong N outright. The identical 468 is real: main is 467 and each branch adds exactly one cluster test. The differing never counts are the thing that shows these are three derivations rather than one copied three times.
On the arrangement
You wrote the fix and you are clearing the block, and you said so before I asked. This review is the independent one: I ran the mutation, the census and the suites myself rather than reading your summary, and the third-state finding is the only thing I would have wanted a second pair of eyes to catch.
…pt#1155) Original work by @linuxhikerpm; rebuilt on ee52910 by @jdatcmd after five PRs landed under it, with the review fix and the census re-derived. THE CHANGE. rel->pages is the whole relation file, base plus every projection, so a covering scan that reads only one projection's row groups was priced for pages it never touches. It is now priced from that projection's own pages, walked from the catalog, with rel->pages as the fallback when the lookup fails so a miss can never look cheaper than the base scan. THE REVIEW FIX. The miss arm divided a plan cost measured AFTER the fixture clears proj_storage_id by one measured BEFORE it. Those are two catalog states, and commandprompt#1180 made them two prices, because its sibling-pages walk reads that same key: before -- miss_run=41991.6 base_run=22000 miss_ratio=1.909 FAIL after -- miss_run=41991.6 base_run=42000 miss_ratio=1.000 9/0 The comment claimed the property that failed -- "independent of how rel->pages is computed (survives commandprompt#1180)" -- which is true of the covering arm, whose want_run comes from the catalog, and false of this one, whose oracle is a measured plan cost. Both halves now say which arm it is true of. THE REORDER BOUGHT NO BLINDNESS. With pgcolumnar_projection_pages mutated to always return fallbackPages the covering arm goes red at full strength (`off-band got=21995.6 want=20000`); restored, 9 passed. REBUILT RATHER THAN REPLAYED. A six-commit rebase conflicted on TESTS.md at every step and one attempt COMMITTED FOUR CONFLICT MARKERS before being caught, so the branch's own changes were applied to main file by file instead. src/columnar_customscan.c needed a real merge: commandprompt#1127 renamed `scale` to `projScale` in the same block this change rewrites, and the result keeps commandprompt#1127's name with this change's pricing. ALL THREE COUPLED SUITES PASS ON THE COMPOSED TREE, which is the point: projection_scan_io.sh 9/0 miss_ratio=1.000 projection_parallel.sh 9/0 (commandprompt#1127, shares the function) base_scan_io.sh 9/0 ratio=1.000 (commandprompt#1180, whose walk reads the key) CENSUS RE-DERIVED TWICE, once per rebase: cluster_tests 468 -> 469 collection, 49 cluster files guard_tests 403 main's, untouched checks_never_observed_red 1499 -> 1506 awk over the ledger suites_not_covered 249 unchanged check_ledger.tsv checked rather than trusted: 1535 rows, zero duplicate (suite, part, check) keys. TESTS.md rebuilt from main with the section applied once and its number derived as max + 1 -- 78, having been 74 and then 77 as main moved twice under it. 78 sections, 78 TOC entries, 78 of 78 pairing on number AND title, contiguous, zero bad anchors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n
1d127de to
237c830
Compare
…commandprompt#1198) Original work by @linuxhikerpm; rebuilt on ee52910 by @jdatcmd after five PRs landed under it, with the two review fixes and the census re-derived. THE CHANGE. Planning one columnar table sequentially scanned pgcolumnar.options and pgcolumnar.projection. options_pkey is (regclass) and projection_pkey leads with storage_id, which is what those lookups ask for, and both scans passed InvalidOid. They now pass the index with the same OidIsValid fallback the other catalogs use. FIRST REVIEW FIX: THE PAIR THE GRADER COULD NOT SEE. catalog_plan_index.sh and test_catalog_plan_index.py are a new PAIR and compare_to_bash.py declared it in neither list, which failed three pytest legs: every pair in the tree is declared, so none is silently ungraded: got 'catalog_plan_index' want 'none' Declared in COMPLETE. The grader now reads 6 literal, 0 template, missing 0. SECOND REVIEW FIX: THE SIXTH CALL SITE. `options_pkey is (regclass), the same column this key names` is true of six scans and five of them moved. PgColumnarRenameDeclaredSortByColumn is the same shape line for line as PgColumnarReadSortBy, which did: same key, same strategy, same single-row read, same RowExclusiveLock and NULL snapshot after a CommandCounterIncrement. NO MEASUREMENT WILL SHOW IT. It runs on ALTER TABLE ... RENAME COLUMN, not on a plan, so the planner-path probe that found the other five cannot reach it. The reason to fix it is that the population was a list of six function names rather than the property "the key column IS the index's column", and a list is what silently regrows. @OffgridwithJD confirmed it is exercised rather than argued: test/sorted_mark_rename.sh renames a declared sort-by column, 46 passed. REBUILT RATHER THAN REPLAYED, for the reason commandprompt#1155 records: a replay conflicts on TESTS.md at every commit and one attempt committed four conflict markers before it was caught. src/columnar_metadata.c applied cleanly. catalog_plan_index.sh 6/0 test_catalog_plan_index.py 6/0 the parity and corpus guards 302 checks, 80 passed CENSUS RE-DERIVED TWICE, once per rebase: cluster_tests 468 -> 469 collection, 49 cluster files guard_tests 403 main's, untouched checks_never_observed_red 1499 -> 1501 awk over the ledger; it read 1494 against the previous main, and neither number is the other plus a delta suites_not_covered 249 unchanged check_ledger.tsv checked rather than trusted: zero duplicate (suite, part, check) keys. TESTS.md rebuilt from main with the section applied once and its number derived as max + 1 -- 78, having been 74 and then 77 as main moved twice under it. 78 sections, 78 TOC entries, 78 of 78 pairing on number AND title, contiguous, zero bad anchors. STILL OPEN, from the review and not fixed here: pgcolumnar.storage is scanned sequentially twice per planned query, because storage_pkey is on storage_id and the lookup is by relation_oid. Closing it needs a new index and a catalog migration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n
OffgridwithJD
left a comment
OffgridwithJD
left a comment
There was a problem hiding this comment.
Reviewed 237c830 by building it and running it, not by reading the diff.
The composed tree holds
Built from this branch with PGC_SKIP_BUILD unset, so the harness reports
source: 87b23ab1ca28 matches the binary under test rather than measuring
whatever was already installed:
projection_parallel.sh 9 passed + 0 failed
projection_scan_io.sh 9 passed + 0 failed
base_scan_io.sh 9 passed + 0 failed
That reproduces your numbers on my own build.
One finding, and it is in my code rather than yours
#1127 left this above the clamp in the parallel covering block, and the merge
kept it verbatim:
Clamp ioRunProj to projRun. With projRun = serialRun * projScale this is
unreachable ... It becomes live if projRun is ever computed independently
(for example from the projection's own pages).
This PR replaces exactly that formula:
- projRun = serialRun * projScale (what the comment assumes)
+ projRun = cpuRun * projScale + ioProj; ioProj = seq_page_cost * projPages * sel
So the comment's stated reason is now false, and the trigger it names —
computing projRun from the projection's own pages — is this PR's title. The
comment now tells the next reader that a branch cannot be taken, and gives a
reason this PR removed.
What I measured, rather than argued
A probe at the clamp site, printing both sides and whether it binds:
projection_parallel.sh reached 3 binds 0
ioRunProj=24.4794 projRun=2122.2110
ioRunProj=24.4794 projRun=2122.2110
ioRunProj=0.2473 projRun=163.8619
projection_scan_io.sh reached 0
base_scan_io.sh reached 0
I also built a fixture aimed at binding it — fat incompressible base, covering
projection of the single int column, forced parallel:
reached 1 binds 0 ioRunProj=0.2504 projRun=148.2537
Why I could not make it bind
Binding needs projScale * (ioRun - cpuRun) > ioProj. sel cancels, leaving
2 * ioBase - serialRun > baseSurvival * seq_page_cost * projPages
so base I/O has to exceed base CPU by more than about projPages. Both
fixtures moved that margin the wrong way — 87x, then 592x — because
pgcolumnar_scan_io_run_cost prices only the columns actually read, and a
single int column's decode CPU dwarfs its page I/O.
Where that leaves it
- Verified: the comment's premise is false as of this PR.
- Unproven: whether the clamp can bind at all under the new formula. I tried
twice and failed, so I am not claiming it is reachable.
Not a blocker for the pricing change, which I think is right. What I would like
before this merges is the comment corrected, since it is two sentences and this
PR is what falsified them. Leaving a comment that says "unreachable" above a
branch whose stated guard has been removed is the shape that costs the next
reader an afternoon.
…commandprompt#1155) commandprompt#1127's comment says the ioRunProj clamp is unreachable "with projRun = serialRun * projScale", and names "computed independently (for example from the projection's own pages)" as what would make it live. That is this PR's title, and my rebuild kept the comment while replacing the formula it quotes. Caught by @OffgridwithJD. The premise is now stated as FALSE, and reachability as UNPROVEN, because that is what was measured rather than argued. They probed the clamp: reached three times in projection_parallel.sh and bound zero, margins 24.4794 against 2122.2110 and 0.2473 against 163.8619; a fixture built to bind it reached once and still did not, 0.2504 against 148.2537. Binding needs 2*ioBase - serialRun > baseSurvival * seq_page_cost * projPages in which sel cancels, and both attempts moved the margin the wrong way, 87x and then 592x, because pgcolumnar_scan_io_run_cost prices only the columns read. The clamp stays: it is cheap and its absence would be a silently negative cpuRunProj. projection_scan_io.sh 9/0 projection_parallel.sh 9/0 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n
OffgridwithJD
left a comment
OffgridwithJD
left a comment
There was a problem hiding this comment.
Approving 1052309, 15/15 green. I am the non-author reviewer here by
arrangement: @jdatcmd rebuilt this branch, so the approval should not be theirs.
What changed since the head I ran
src/columnar_customscan.c | 36 +++++++++++++++---------
1 file changed, 27 insertions(+), 9 deletions(-)
Comment-only. Every changed line in that diff is inside the block comment above
the clamp; the code lines around it are unchanged context. So the executable
content at 1052309 is the content at 237c830, which I built and ran:
projection_parallel.sh 9 passed + 0 failed block reached 3, bound 0
projection_scan_io.sh 9 passed + 0 failed
base_scan_io.sh 9 passed + 0 failed
on a build where the harness confirmed source: 87b23ab1ca28 matches the binary under test rather than measuring whatever was already installed.
The correction is right, including the word I cared about
It says the old premise is FALSE and reachability is UNPROVEN, which is the
measured state, and it does not claim the clamp is reachable. It also gives the
third reason, which is the one that does not depend on either: the clamp stays
because its absence would allow a silently negative cpuRunProj. That reason
would survive someone later proving the branch dead.
The numbers quoted are mine and they check out: reached 3 and bound 0, margins
24.4794/2122.2110 and 0.2473/163.8619, the built fixture at 0.2504/148.2537, and
the 87x then 592x in the wrong direction.
Reachability is now tracked in #1209, so it cannot be lost when this merges.
The two things I raised that are not in this PR
Both filed rather than held against it:
- #1208 —
pgcolumnar_projection_pageshas three return paths and this
branch's suites reach two. Mutatingpages = 1topages = 997leaves shell
9/9 and pytest 9/9 unchanged, so the near-empty-projection path is unobserved.
I re-read the function on1052309before filing; the three paths are
unchanged, and the new comment correctly distinguishes the arithmetic1from
the lookup-failure path above it. A coverage gap, not a defect. - #1209 — the clamp reachability above.
The pricing change itself is right, and the ratio spans one catalog state, which
was the thing to check.
…pt#1155) Original work by @linuxhikerpm; rebuilt on the current main by @jdatcmd, with the review fix and the census re-derived. Third rebuild: every PR on this board touches the same four bookkeeping files, so merging any one makes the rest DIRTY. THE CHANGE. rel->pages is the whole relation file, base plus every projection, so a covering scan that reads only one projection's row groups was priced for pages it never touches. It is now priced from that projection's own pages, with rel->pages as the fallback when the lookup fails so a miss can never look cheaper than the base scan. THE REVIEW FIX. The miss arm divided a plan cost measured AFTER the fixture clears proj_storage_id by one measured BEFORE it -- two catalog states, which commandprompt#1180 made two prices because its sibling walk reads that same key: before miss_run=41991.6 base_run=22000 miss_ratio=1.909 FAIL after miss_run=41991.6 base_run=42000 miss_ratio=1.000 9/0 And the reorder bought no blindness: with pgcolumnar_projection_pages mutated to always return fallbackPages the covering arm goes red at full strength. THE COMMENT THIS CHANGE FALSIFIES IS CORRECTED RATHER THAN CARRIED. commandprompt#1127 wrote that the ioRunProj clamp is unreachable "with projRun = serialRun * projScale" and named "computed independently (for example from the projection's own pages)" as what would make it live. That is this change. It now records three states: the old premise is FALSE, reachability is UNPROVEN (@OffgridwithJD probed it, reached 3 and bound 0, then built a fixture that reached once and still did not bind), and the clamp stays because its absence allows a silently negative cpuRunProj -- a reason that survives whichever way reachability goes. cluster_tests re-derived on the composed tree: 476. It has read 464, 468, 469 and now 476 as main went 463, 467, 468, 475 under this branch. Every one was correct for the main of its hour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n
1052309 to
c7b9c47
Compare
OffgridwithJD
left a comment
OffgridwithJD
left a comment
There was a problem hiding this comment.
Approving c7b9c47, replacing my approval of 1052309.
src/ is byte-identical to the head I built and ran:
git diff --stat 1052309 c7b9c47 -- src/ (empty)
So the executable content is still the tree I measured at 9/0 on
projection_parallel.sh, projection_scan_io.sh and base_scan_io.sh, on a
build where the harness confirmed source ... matches the binary under test.
The rest of the delta is main moving underneath — test_ttl_expire.py arriving
with #1201 — plus this branch's own TESTS.md renumbering, which is the third time
today the shared bookkeeping files have forced a rebuild. I compared each head's
own contribution against its merge base rather than diffing the two shas, so
main's arrival does not read as this branch's change.
#1208 and #1209 carry the two things I raised that are not for this PR.
…commandprompt#1198) Rebuilt on main carrying commandprompt#1155 rather than rebased, because every census file this branch touches is a measurement of the tree and commandprompt#1155 moved all of them. Re-derived on the composed tree, never by arithmetic across the merge: cluster_tests 476 -> 477, by collection. Both sides carried 476 and the merge was silent: main alone collects 476 (50 cluster files), this tree collects 477 (51). checks_never_red 1506 -> 1508, by counting field five. This branch had derived 1501 against the previous main; neither number is the other plus a delta. suites_not_covered unchanged at 249, measured on both trees: a new registered suite arriving with ledger rows raises registered and covered by one each. TESTS.md takes section 80, not 79; commandprompt#1155 took 79. Checked by pairing TOC entries to headings in both directions, with all four mutations proved red (broken anchor, TOC entry deleted, duplicate number, heading deleted). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n
…1209) #1127 called the clamp unreachable "with projRun = serialRun * projScale". #1155 computed projRun independently, which is the falsifier #1127 named, and the comment has read UNPROVEN since. Measured, with a probe at the clamp site. IT IS REACHABLE. Both sides are linear in seq_page_cost because the CPU term is not, so three constants fitted from six points predict the crossing: pre = ioRun * projScale = spc * A A = 3.1 projRun = C + spc * B B = 3.0 C = 262.5 binding needs spc > C/(A-B) = 2625 Predicted before it was run. 2048 does not bind, missing by 0.9%; 4096 does, and the plan changes from Gather -> Parallel Custom Scan to a serial Custom Scan, which is the consequence #1127 wrote down. AT THE DEFAULT GUCS IT CANNOT BIND, for a constant rather than a property of the fixture: binding needs the projection to save more than 5.12*W + 82 bytes per row, because cpu_operator_cost * W/4 is 5.12 times seq_page_cost * W/8192. Measured storage runs 0.13x and 0.04x of W. A second fixture built to move that margin did not move it: 2 pages of difference and the same 2625 threshold in both, because the base compresses the same data almost as well as the projection does. THE CLAMP CHANGES NO PLAN. Removing it leaves every plan in projection_parallel.sh identical and the suite green -- the unclamped total is LARGER, so the serial covering path wins either way. It keeps cpuRunProj from going negative, which no plan exposes. So the two new arms are named for I/O amortisation rather than for the clamp. I wrote them as clamp arms first; the removal mutation did not redden them, which would have shipped a vacuous guard. What does redden the second arm is (ioRunProj + cpuRunProj) / divisor: got [gather+projection] want [projection-only] Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP
3 participants
Summary
seq_page_cost * rel->pages, the whole relation file (base plus every projection). The covering path now charges I/O from that projection's own page-rounded row groups, times the already-floored sort-key selectivity.test/projection_scan_io.shandtest/pytest/test_projection_scan_io.py) pin the planner ratio on the public EXPLAIN-cost seam. Own fixtures, matching assertion names. Neither file reads the other.rel->pagesreddens the load-bearing arm the same way the unfixed tree did.suites_not_coveredstays 249.cluster_tests437 by collection on current main (18821ce).Not merged. Not self-approved.
Test plan
test/projection_scan_io.shon PG18: covering run 19996 against base-page I/O 42000 (ratio 0.476), not 41991.6 / 1.000test/pytest/test_projection_scan_io.pyon PG18, same assertion names, own tableioProjback torel->pagesfails both twinsgot [base-pages] want [proj-pages]15;16;17;18;19; census counted, not added