test: port three cost-model suites to pytest - #1136
Conversation
jdatcmd
left a comment
jdatcmd
left a comment
There was a problem hiding this comment.
Three ports, all three graded missing: 0, the COMPLETE list updated in both directions, ledger rows across five majors, and cluster_tests re-derived after the reseat rather than carried. None of that needed saying, and I checked each of them rather than reading the PR body for them.
One blocker and one that is nearly one, and they are the same shape: an arm that stays green when its subject never happened. I verified both from the source rather than from a summary.
Blocker: the ordering arm passes when the wide table never fetches by index at all
test/index_fetch_penalty_width.sh:113-126 and test/pytest/test_index_fetch_penalty_width.py:181-197:
FN="$(flip_rows ifw_n)"
FW="$(flip_rows ifw_w)"
check "premise: the narrow table does fetch by index somewhere on the ladder" # FN > 0
check "a wider decoded prefix gives up on per-row fetches no later than a narrow one (#803)"
[ "${FW:-0}" -le "${FN:-0}" ]
flip_rows initialises last=0 and breaks on the first rung that is not an Index Scan, so a table that never plans one returns 0 — and 0 <= FN is true for every FN >= 1. The premise guards FN and nothing guards FW. The pytest half has the identical shape: expect.at_least(fn, 1, ...) with no counterpart for fw, and "ordered" if (fw or 0) <= (fn or 0).
The failure this hides is the natural over-correction of the very term #803 added: charge the wide prefix too much, ifw_w abandons the index at the first rung, FW is 0, the arm prints ordered, and the suite is green while its own echo line says wide prefix ...B = 0 rows.
It is latent rather than live — your own #803 removal proof does redden this arm today, inverting it at 120 against 40 — and the neighbouring premise (the penalty is what moves this plan) proves the fixture has a usable index at k=400, so FW = 0 could only come from the penalty pricing it off entirely. That narrows what the gap means; it does not close it.
The fix is FW > 0 beside the existing FN > 0, in both harnesses. I know what that costs on the shell side: a new check name in a suite the ledger covers, so it pulls in rows seeded from five real runs merged in one call. I am asking for it anyway, because it is the arm the PR exists to protect and because a one-sided premise is exactly how this class goes quiet later, on a different major, with nothing red to say so.
Nearly a blocker: the #171 point-lookup arm ships with no removal proof
The PR says "every load-bearing arm has a removal proof" and lists four mutations. The #171 point-lookup arm in test_scan_decode_cost.py is not among them, and the fixture shrink cut its margin by roughly an order of magnitude. Either name and run the mutation that reddens it — inflating the index-fetch penalty is the failure it exists to catch — or say in the file that it is illustrative, so the next reader does not take it for a guard. If it will not redden, that is the more useful finding of the two.
Minor: three published values for one cell
For penalty on, 1,000 rows, stripe_row_limit=2000, this PR publishes 52.98 (body table), 52.50 (body attribution block) and 51.70 (module docstring, TESTS.md §57, CHANGELOG). The penalty-off control disagrees the same way, 36.00 against 35.20. The tree is self-consistent; the body is the odd one out — but which run is the stale copy is not established by the artifacts, so the fix is a provenance label on each table rather than an assumption about direction.
Worth noting what is NOT wrong with it: no arm asserts a literal cost, so nothing red depends on the drift and CI cannot see it. That is also why only a cell-by-cell reconciliation finds it. The disagreement is confined to the index-scan cells — both Custom Scan 2004.50 entries match, and the body is higher at 1,000/20,000/50,000 but lower at 100,000 — which rules out a storage-level difference and leaves either a per-run ANALYZE draw moving the correlation estimate or a build whose penalty term differs from the shipped one.
Merge-time, not a defect in this PR
cluster_tests on the composed tree with #1140 is 430, derived by collection on the merge; this PR states 426 and mine states 427. We both derived it independently and got the same number, by different routes. It sits behind a conflict so nobody merges it blind. Both PRs also claim TESTS.md section 57; the second to merge takes 58.
Re-review promptly once the FW premise lands — I am not asking for anything else.
87aa192 to
f9faf7e
Compare
|
Re-read 1. The ordering armI asked for with the measurement in the comment ( I verified the parity consequence rather than assuming it: the new name is reported as an extra, not a missing, so all three pairs still grade 2. The #171 armThe negative is worth more than a reddening mutation would have been. Inflating the scan decode charge a millionfold leaves the arm passing (#503 ratio 4.209 → 16.999) because a dearer scan makes the index more attractive; only 3. ProvenanceDirection established rather than assumed, which is what I asked for: the tree was right and the body was the stale copy, and the CHANGELOG now carries the label (one run, PG18 assert build, 2026-09-18). Approving when CI is green9 checks still pending on At merge time: whichever of #1136 and #1140 goes second re-derives |
native_index_fetch_stripe_cost, scan_decode_cost and index_fetch_penalty_width, 18 names, each graded missing: 0 by compare_to_bash.py. One subject: what the planner charges for the decode a scan or a fetch really performs. Each port asserts something its bash original does not. The first suite's two arms are not the same plan at its 100,000-row range -- the second is a Custom Scan -- so it compares the prices of two different nodes; the port matches at 1,000 rows, asserts the shape first, and adds the attribution cell showing the fetch penalty is the term carrying the option. The second's fixtures are smaller and the ratios were measured across a 20x range before they were shrunk. The third reads plan shape from FORMAT JSON by exact Node Type equality and reads the row-group geometry back from pgcolumnar.row_group rather than assuming it from the option that set it. No bash suite changes, so no ledger row moves and the census does not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs
f9faf7e to
463204a
Compare
|
Verified The COMPLETE check is the one I most wanted to run rather than trust, because that list has conflicted in four pull requests today and #1093 is the precedent for a stem going missing in exactly that resolution. Nothing dropped from either side. On the one-stem-per-line reflow: keep itYou were right to declare it rather than slip it in, and it is the right change on its own merits. The precedent is in the tree already — The
|
jdatcmd
left a comment
jdatcmd
left a comment
There was a problem hiding this comment.
Approving 463204a. All 14 check-runs completed/success on that sha, read from the sha rather than from the PR view, and the three items from my review are in with the first one fixed better than I asked.
What I verified rather than read, on the reseated tree:
reseated onto d8d755c yes
COMPLETE 21 from main + 3, none dropped
TESTS.md 58, 59, 60
ledger / budget / registry untouched
cluster_tests stated 430, collected 430 here; guard 380 unmoved
compare_to_bash all three pairs, missing: 0
the new FW premise is reported as an extra, not a missing
The ladder fix is the part worth reading twice. I asked for FW > 0 without checking that a correct build produces a non-zero FW; it does not, on the ladder as it was, so the premise I requested would have reddened a green tree. The real defect was one level down - the ladder started at rung 20 and the wide table's flip point is between 5 and 7 rows, so every rung it tried was already past it. Extending the ladder and re-running the #803 removal proof after the change, rather than assuming it survived, is the difference between the fix and the fix I asked for.
And the #171 answer is worth more than a reddening mutation would have been: inflating the scan decode charge a millionfold leaves the arm passing, because a dearer scan makes the index more attractive; only pgcolumnar_index_fetch_penalty moves it. The file now says so, and says what follows - nothing in either harness bounds the decode charge from the other side. That names a gap where there was an arm that looked complete.
Keeping the one-stem-per-line reflow, for the reason already on the thread: the precedent is in run_all_versions.sh, COMPLETE conflicted in four pull requests today, and #1093 is what that costs when it goes wrong.
Merging under jdatcmd now.
2 participants
Three suites ported, one subject: what the planner charges for the decode a scan or a
fetch really performs.
native_index_fetch_stripe_cost,scan_decode_costandindex_fetch_penalty_width, 18 names, each gradedmissing: 0.One pull request rather than three because the per-PR cost — the census, the two counts,
TESTS.md and its anchor — is paid once per review, not once per suite.
The first port found something in its bash original
native_index_fetch_stripe_cost.shsays it asserts that changing the per-tablestripe_row_limit"changes the estimated index-scan cost". At its 100,000-row rangethe second arm is not an index scan. Measured, PG18 assert build, total cost of the top
node:
So the two numbers it compares are the prices of two different plans. They differ, the
check passes — but a plan flip is one of the things a changed penalty causes, so that
comparison cannot separate "the penalty re-priced this fetch" from "the penalty moved the
planner onto another node". The port matches at 1,000 rows, where both arms stay on the
index, and asserts the shape before comparing the prices.
I have not touched the shell suite. Narrowing its range is a one-line change and it is
yours to make or decline; I would rather report it than quietly fix a suite in a PR whose
subject is the port.
The port also carries the attribution cell the original leaves out. With the penalty off,
the same two arms price identically:
Without that, the arm shows only that two costs differ while one option moved, which any
cost term reading that option would satisfy equally.
The fixtures are smaller, and the ratios were measured before they were shrunk
scan_decode_cost.shwrites 2,000,000 rows twice. The ratio its arm bounds at 1.5:Flat to three decimals across a 20x range, so the extra rows buy the assertion nothing.
200,000 is kept rather than the cheapest cell because at the default 150,000-row group
limit it still spans more than one row group.
The point-lookup arm is the one size actually decides, so its boundary was measured rather
than guessed: Custom Scan at 50,000 rows, Index Scan from 100,000. 200,000 is one
doubling of margin.
Every load-bearing arm has a removal proof
Four mutations against the C, each asserted to have APPLIED (source hash moved) and to have
been RESTORED byte-identical, each cell printing its own
.sohash so no cell measured theprevious one's binary:
stripe_row_limitarms collapse onto 810.46The second and third matter as a pair: they redden different arms on the same file, so
the two properties are separately pinned rather than jointly.
The row-group limit is a per-table option here, not a cluster setting
The shell twin pins
pgcolumnar.stripe_row_limit=20000in the cluster config so thewriting and planning sessions cannot disagree (#806). A pytest cluster is shared by every
test in the session, so that is not available — and is not needed.
set_optionsbefore thewrite is durable, belongs to the table, and is what both the writer and the planner read.
The geometry is then read back from
pgcolumnar.row_groupas a premise (20 groups oneach arm) rather than assumed from the option that set it.
What I ran
The whole pytest corpus on all five majors, on the rebased tree:
Identical check counts on all five, which is the part worth reading: a cost-model arm is
exactly the kind that meets a different planner on a different major and quietly asserts
something else.
The change touches
test/pytest/**,CHANGELOG.mdandTESTS.mdonly — no shell suiteand no
src/— so the shell matrix has none of this change to exercise, and I did not runit. What can break is a new test meeting a different planner on a different major, which
is what the five-major run above is.
No bash suite changes, so no ledger row moves and the census does not.
cluster_tests427 → 430, re-derived by collection on the tree reseated ontod8d755c, not by adding 3. This branch has stated 424, then 426, now 430 — three rightanswers to the same question, because main moved three times underneath it.
One thing outside the port, flagged so you can tell me to drop it. Resolving the
COMPLETEconflict for the third time today, I reflowed the list to one stem per line.The precedent is two directories away, in
run_all_versions.sh:COMPLETEhas conflicted in every pull request that touched it today — #1134, #1140 andboth of mine — and each conflict is a chance to drop a stem with
--ours, which is hownative_chunk_length_boundwas lost on #1093. It is a 25-line diff in a list I wasediting anyway; say the word and I will revert it to a minimal three-line insertion.
One thing I saw and did not cause
test_sorted_pathkeys.py::test_a_query_that_cannot_use_the_order_does_not_pay_to_decidefailed once during my runs, on its
abs(on - off) <= 5planning-buffer tolerance. Becausemy fixtures add 1.3M rows to the shared session cluster, "pre-existing" was a claim I had
to earn rather than assert, so I ran the corpus interleaved A/B — same tree, my three files
collected against deselected — and printed the arm's own quantity:
The failure landed in the arm where my files are never collected, so it is not mine.
I have not yet identified which test failed in that rep — the harness printed
delta=0, soit was not the planning-buffer arm — and I am characterising it separately rather than
holding this PR for it. Flagging it because an intermittent red in the pytest gate is worth
a number from whoever sees it first.
🤖 Generated with Claude Code
https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs