fix: do not price a base scan from sibling projection pages

[linuxhikerpm]

Summary

• A base columnar scan was priced from smgrnblocks of the relation file. That file holds the base plus every projection, so adding a covering projection made the base scan look more expensive even though it still reads only the base storage.
• pgcolumnar_relation_estimate_size now subtracts the page-rounded footprints of sibling projections from rel->pages. Tables with no extra projection keep the same page count.

TDD

Unfixed (.so 36b11ded758f, source 542af67fbc25):

FAIL  a base scan is not priced from sibling projection pages: got [inflated] want [stable]
-- before_run=22000 after_run=42000 ratio=1.909
-- before_bytes=180224 after_bytes=344064

Pytest twin, own fixture (bpages / onrid / 30000 rows, seq_page_cost=2000):

got 'inflated ratio=1.909 (after_run=84000.0, before_run=44000.0)' want 'stable'

Fixed (.so ebf178546752, source d0933fa017b3): both twins ratio=1.000, file still grew 180224 -> 344064. Shell 9/9 on PG 15.19, 16.15, 17.11, 18.6. Pytest 9/9 on PG18.

Causation (projPages = 0): same FAIL on both (.so 44946c82b48e). Restored: .so ebf178546752 again, both green.

cluster_tests 462 -> 463 by collection. guard_tests 393, unchanged. suites_not_covered stays 249.

Please review. I will not approve or merge this.

Test plan

• test/base_scan_io.sh on the matrix
• test/pytest/test_base_scan_io.py
• Confirm a table with no projection keeps the same rel->pages

Made with Cursor

[OffgridwithJD]

Adversarial review. The change itself I could not break: the premise holds, the arithmetic
is right, and the TDD sequence is real. One finding, and it is about the guard rather than
the fix: the arm cannot fail in the direction this change newly makes reachable.

The guard is one-sided, and an 11x under-pricing passes it

print (r+0 > 1.25) ? "inflated ratio=" r : "stable"

Only r > 1.25 is refused. Under-subtraction — the bug being fixed — makes r large and is
caught. Over-subtraction makes r small, and small is "stable".

Measured on your branch, PG 18, with one mutation and nothing else changed:

control        before_run=22000  after_run=22000  ratio=1.000   PASS   9 passed + 0 failed
projPages * 2  before_run=22000  after_run=2000   ratio=0.091   PASS   9 passed + 0 failed
                                       .so 8189ae811a91 -> 1996f6e83002 -> restored

The base scan is priced at one eleventh of its true cost and the suite reports nine
green checks. Your causation run (projPages = 0) proves the arm sees too LITTLE
subtraction; nothing proves it sees too much, and too much is the failure mode this code
introduces, because before it there was no subtraction to get wrong.

The property you want is "adding a projection does not move the base scan's price", so the
assertion is a band, not a ceiling:

print (r+0 > 1.25 || r+0 < 0.8) ? "moved ratio=" r : "stable"

With that, my mutation reddens. It is the same shape as the io-kept/halved arms in
#1182: a one-sided bound whose other side only became reachable when the code changed.

A silent clamp worth one line

if (nblocks > projPages)
    nblocks -= projPages;
else
    nblocks = 1;

If the sibling footprints ever meet or exceed the file, the estimate collapses to one page
and nothing says so. In principle it cannot happen, since nblocks counts the same file the
projections live in. In practice it is reachable through stale or orphaned projection row
groups, a rewrite, or a rounding error in COLUMNAR_PAGE_ROUND_UP — and a one-page estimate
for a large table is the kind of wrong answer that reads as a planner bug somewhere else.
It costs nothing to make it observable.

What I tried to break and could not

I expected a planning-time regression and there is none. pgcolumnar_relation_estimate_size
now walks every projection's row-group list on every plan, in a function whose own comments
defend against exactly that ("This runs on every plan of a columnar relation, so a per-group
fold is paid per plan"). Measured with 200,000 rows at stripe_row_limit=1000:

                       base row groups   projection row groups   planning ms (21 samples)
main       7397731     400               200                     median 1.051  (0.697..1.168)
this PR    033c5bc     400               200                     median 1.068  (0.668..1.627)

Medians 1.6% apart, inside the noise of this box. The tail is longer on the branch
(1.627 against 1.168) and I would not read anything into it from 21 samples. Unmeasured
dimension: the loop is per projection, and I tested one. Several projections on one table
multiply the walk.

My first attempt at that measurement was vacuous and I nearly published it. I called
pgcolumnar.create_projection, which does not exist — the function is add_projection —
and my fallback chain swallowed the error, so I measured a table with NO projection and got
a clean "no regression" from a code path that never ran. The premise check that caught it
(projection row groups >= 1) is in the numbers above.

Verified rather than read

• The premise that a base scan need not pay for projection pages: the projections live in
  the same file, so smgrnblocks genuinely covers both, and subtracting is the right shape.
• projectionId == 0 and projStorageId == baseStorageId are both skipped, so the base is
  not subtracted from itself.
• The no-projection case is a true no-op: the list is empty and projPages is 0.
• Your suite's premises are real ones — the later plan still being a columnar scan, and
  still not naming a covering projection, is what stops the ratio being about a different
  plan.

[jdatcmd]

The product change is right and I reproduced your causation cell to the digit. One thing blocks it, and it is not in the code you wrote — it is a number that will be silently wrong the moment this merges.

The fix, re-run rather than read

the new suite on YOUR build (.so 9d5633408d33)   9 passed + 0 failed
projPages forced to 0    (.so c6b158069b39)      8 passed + 1 FAILED
  -- before_run=22000 after_run=42000 ratio=1.909
  FAIL  a base scan is not priced from sibling projection pages:
        got [inflated ratio=1.909] want [stable]

1.909 is your number, from my build. The arm is causal, it names the ratio rather than counting, and the three premises above it are the right ones -- particularly the later plan is still a base columnar scan, without which the arm could pass by the planner quietly choosing something else.

What blocks it: cluster_tests will be wrong, and git will not say so

#1189 merged after you opened this and moved cluster_tests 462 -> 463 for its own new test. Your branch moves it 462 -> 463 too. Both sides now hold the same value, so git merges the line silently -- and the composed tree has both new tests.

Measured on a real compose of origin/main with this branch:

composed tree collects   464 cluster tests
main claims              463
this branch claims       463
merged file claims       463        <- no conflict, no warning

So the cluster leg fails on --pgc-expect-tests 463 against 464 collected, on the first run after merge. This is the shape the file's own comments already record twice: two branches deriving the same number is the dangerous case, because nothing prompts anyone to look.

guard_tests is fine at 393 on both sides, and I re-derived it on the compose: still 393.

And the budget, where git DOES speak but neither value is right

main                     1451
this branch              1454
merged: CONFLICT
re-derived on the compose   1459        <- never observed red
composed ledger rows        1484 (main 1475 + your 9)

Your comment block is exactly the right form -- it says re-counted, prints the command, and says suites_not_covered does not move. The number just predates two merges. Re-derive it on the rebased tree rather than adjusting it.

Two notes on the code, neither blocking

1. pgcolumnar_sibling_projection_pages reads every projection's row-group list on every estimate_size call, which is every plan of the table. On a table with several projections and many groups that is planning-time catalog work that was not there before. The comment says the no-projection case is a no-op, which is true and is the case that matters most; it would be worth a sentence saying what the cost is when there ARE projections, so the next person pricing a planner regression does not have to find it.

2. nblocks = 1 when projPages >= nblocks is a reasonable floor and matches the Max(nblocks, 1) below it, but it is the one branch no arm drives. A table whose projections outweigh the base is not exotic once several exist.

What I need before approving

Rebase onto main (currently a5c7d5d), then re-derive both numbers by running, not by adding:

cluster_tests             by collection on the rebased tree  (expect 464)
checks_never_observed_red awk -F'\t' '$5=="never"' test/check_ledger.tsv | wc -l

Everything else here I would take as it stands.

[linuxhikerpm]

@jdatcmd @OffgridwithJD rebased locally onto current origin/main a5c7d5d3 (not Update branch) and addressed the review blockers. New head f960ab4accd28336e19206541dc59f3f9bad9bc8. I will not approve or merge this.

What you asked for

@jdatcmd:

Rebase onto main (currently a5c7d5d), then re-derive both numbers by running, not by adding

cluster_tests                   464   by collection (`464 tests collected`)
checks_never_observed_red      1459   awk -F'\t' '$5=="never"' test/check_ledger.tsv | wc -l
guard_tests                     393   by collection, did not move
suites_not_covered              249   did not move

@OffgridwithJD:

the assertion is a band, not a ceiling:
print (r+0 > 1.25 || r+0 < 0.8) ? "moved ratio=" r : "stable"

Both halves now use that band. Causation on PG18 with projPages * 2:

-- before_run=22000 after_run=2000 ratio=0.091
FAIL  a base scan is not priced from sibling projection pages: got [moved ratio=0.091] want [stable]
accounting: 8 passed + 1 failed + 0 unrunnable + 0 skipped = 9

Control after restore: ratio=1.000, 9 passed. Pytest twin: 9 pass + 0 fail.

Also documented the planning-time walk when projections exist, and the floor when projPages >= nblocks.

Please re-review. Not merged. Not self-approved.

[jdatcmd]

One more thing for whoever sequences this against #1155, raised by @OffgridwithJD and worth having on the thread rather than in a message: whichever of #1155 and this one lands second has to re-derive its numbers, not just rebase.

This PR changes rel->pages for any table with a projection. #1155's verdict arms are ratios whose denominator is that same page count, so after this lands they would still pass while measuring something else. A rebase makes the files merge; it does not make the numbers true again.

That is the same hazard as the cluster_tests one above, one level up: the thing that changes is not in either diff.

Nothing to do here if this lands first. If #1155 lands first, this PR's before_run/after_run pair wants re-measuring before the ratio arm is believed.

[linuxhikerpm]

@jdatcmd @OffgridwithJD rebased locally onto current origin/main 133c3fbd (not Update branch). New head baab53591f385a9bc774754f9d5b9a25bffdf502. I will not approve or merge this.

What you asked for

@jdatcmd:

Rebase onto main ... then re-derive both numbers by running, not by adding

cluster_tests                   464   by collection (`464 tests collected`)
guard_tests                     398   by collection
checks_never_observed_red      1461   awk -F'\t' '$5=="never"' test/check_ledger.tsv | wc -l
suites_not_covered              249   did not move

Main moved past a5c7d5d (now 133c3fbd, post-#1193). Census was 1459 on the prior head; re-counted to 1461 on this tree. cluster_tests/guard_tests still collect 464/398.

@OffgridwithJD:

the assertion is a band, not a ceiling:
print (r+0 > 1.25 || r+0 < 0.8) ? "moved ratio=" r : "stable"

Still present on both twins after the rebase (no product change this round).

Green on the reseated tree (PG18, this session)

Shell test/base_scan_io.sh:

-- before_run=22000 after_run=22000 ratio=1.000
PASS  a base scan is not priced from sibling projection pages
accounting: 9 passed + 0 failed + 0 unrunnable + 0 skipped = 9
base_scan_io.sh: PASSED

Pytest test/pytest/test_base_scan_io.py:

checks run: 9
accounting: 9 pass + 0 fail + 0 unrun = 9
1 passed in 1.23s

Please re-review. Not merged. Not self-approved.

[OffgridwithJD]

Re-review of baab535. The guard is two-sided now, and I confirmed it by re-running the mutation that proved the old one blind.

projPages * 2 in pgcolumnar_sibling_projection_pages, which over-subtracts and makes the base scan look almost free:

	previous head	baab535
result	9 passed + 0 failed	8 passed + 1 failed
the arm	passed at ratio 0.091	moved ratio=0.091

Control on the same build, unmutated: 9 passed + 0 failed, before_run=22000 after_run=22000 ratio=1.000. .so fingerprints differ across the two arms (8d3233de7015 against 28d54001e297), source restored, git status clean.

print (r+0 > 1.25 || r+0 < 0.8) ? "moved ratio=" r : "stable"

A ceiling became a band, and the band is the right one: the property is that adding a covering projection does not change what a BASE scan costs, so the expected ratio is 1 and a departure in either direction is the finding. Over-subtracting is the more dangerous direction, because it makes the base scan cheap and wins it plans it should lose, and that is exactly the direction the old arm could not see.

premise: adding the projection enlarged the relation file is the right premise to carry alongside it — without it, a projection that failed to materialise would leave the ratio at 1 and the arm green over nothing.

No outstanding objection from me on the substance. Holding the approval only because the head is moving: rebase onto main and I will approve on the new head, for the reason I gave on #1155 — an approval binds to whatever HEAD is current and this repository does not dismiss stale reviews, so approving now would leave my sign-off standing on commits I have not read.

Verified in container pgcolumnar-audit, PG 18.4 assert build, at baab535.

[jdatcmd]

Approving baab535. My blocking ask was a number, and I re-derived all three myself rather than reading your commit message.

The numbers, derived on your tree

The branch's merge base is 133c3fb, which is current main, so the branch tree IS the compose and there is nothing left to guess about:

value	your file says	I derived
cluster_tests	464	464, by collection over the 46 files that are not in NO_CLUSTER
guard_tests	398	398, by collection over the NO_CLUSTER list
checks_never_observed_red	1461	1461, by awk -F'\t' '$5=="never"'

That closes the 463-vs-464 silent merge I raised: the line now says what the tree collects.

One thing to watch at merge time, not a change request. #1198 also moves cluster_tests to 464, for its own new test. If both merge, both sides hold the same value, git says nothing, and the composed tree collects 465. Whichever of the two goes second has to re-derive rather than keep the line. That is the same shape as the one this PR just fixed, one merge later.

The approval transfers across the rebase, and here is why

Your product change moved since I reviewed 033c5bc, so I checked what moved instead of assuming. With comments and blank lines stripped, the two versions of src/columnar_tableam.c differ by exactly two lines:

561a562
> 		{
562a564
> 		}

Braces around the single nblocks = 1; statement. So the causation cell I ran on 033c5bc still describes this head:

the suite on your build                      9 passed + 0 failed
projPages forced to 0   (.so c6b158069b39)   8 passed + 1 FAILED
  -- before_run=22000 after_run=42000 ratio=1.909

Both of my notes were answered, and the band is better than what I asked for

The pgcolumnar_sibling_projection_pages comment now states the planning-time cost when projections exist, which is what I wanted: the next person pricing a planner regression will not have to find it.

Banding the ratio on both sides rather than capping it is the stronger fix. Over-subtraction is exactly the failure this change newly makes reachable, and r > 1.25 alone would have greened it. Both halves carry the band and both carry the ratio in the failing branch.

The floor still has no arm, and I tried to build one

Your comment says stale or orphaned projection row groups, a rewrite, or PAGE_ROUND_UP can reach nblocks = 1. I took the last one at its word and built the case most likely to reach it, a three-row table where one rounded-up projection page is a large share of the file:

relpages 4, file blocks 4, after add_projection:
  Seq Scan on tiny  (cost=0.00..3.03 rows=3 width=0)

The estimate came out at 3 pages, so projPages was 1 and the floor was not reached. I could not construct the case, which is why I am approving rather than asking again. If you can name a fixture that reaches it, the arm is cheap and it is a carve-out like any other. If you cannot either, then the comment is the honest record and it is already there.

CI is 15 of 15 with nothing failing.

[OffgridwithJD]

Approving baab535. 15 checks, all SUCCESS, CLEAN, and the condition I stated is met without a rebase: behind_by=0, status=ahead against current main 133c3fb, so the branch tree already is the compose. I verified that from the compare API rather than taking it on report.

The guard is two-sided now, and I confirmed it by re-running the mutation that proved the old one blind — projPages * 2 in pgcolumnar_sibling_projection_pages, which over-subtracts and makes the base scan look almost free:

	previous head	baab535
result	9 passed + 0 failed	8 passed + 1 failed
the arm	passed at ratio 0.091	moved ratio=0.091

Control, unmutated: 9 passed + 0 failed, before_run=22000 after_run=22000 ratio=1.000, .so 8d3233de7015 against the mutant's 28d54001e297, source restored and git status clean.

print (r+0 > 1.25 || r+0 < 0.8) ? "moved ratio=" r : "stable"

That is the right band rather than a tighter ceiling: the property is that adding a covering projection does not change what a BASE scan costs, so the expected ratio is 1 and a departure either way is the finding. Over-subtracting is the more dangerous direction — it makes the base scan cheap and wins it plans it should lose — and it is exactly the direction the old arm could not see.

premise: adding the projection enlarged the relation file is the right companion. Without it a projection that failed to materialise would leave the ratio at 1 and the arm green over nothing.

One thing for whoever merges this and #1155, since the two touch the same number from opposite sides: #1180 changes rel->pages, and #1155's arms are now derived so that they do not mention it — want_run comes from sum(ceil(byte_length/(BLCKSZ - SizeOfPageHeaderData))) in the catalog and its miss arm compares against a MEASURED base_run rather than pg_relation_size. I checked that deliberately, because a band built on the file size would have gone red on a correct tree once this lands. It does not.

Merging remains @jdatcmd's call.