Conversation
|
TDD excerpts from this session. Prior chat summaries were not used as evidence. Start SHA Shell
|
jdatcmd
left a comment
jdatcmd
left a comment
There was a problem hiding this comment.
Two things you have clearly taken on board since #1107 — both harness halves ship together, and the six ledger rows are seeded across all five majors. Neither needed saying this time.
One blocker, and it is the same rule that already has a precedent two directories away.
The test cannot see the defect it exists for
The two load-bearing arms are:
check "a covering projection can be a parallel scan" shape == "gather+projection"
check "a parallel covering projection returns the covering rows once" count == WANT
Both pass on a build where the partial path is offered but no worker ever claims a stripe. Gather is in the plan either way, and the leader alone produces exactly the right rows — so the count arm is satisfied by a scan that is parallel in name only. The pytest half asserts the same two things.
parallel_am_scan already does this properly, and it is the direct precedent:
parallel_am_scan.sh:93 Workers Launched: 2
parallel_am_scan.sh:105 "workers share the table-AM scan, it is not a single claimer"
# Sharing means both launched workers produced rows.
test_parallel_am_scan.py:114 (_first(analyzed, "Gather") or {}).get("Workers Launched")
test_parallel_am_scan.py:143 "workers share the table-AM scan, it is not a single claimer"
That suite exists because a first-wins phs_nallocated produced exactly this shape: a plan that looked parallel while one backend did all the reading. Your change routes the covering projection through the same shared counter, so it is exposed to the same failure and should carry the same assertions.
What I would add to each half: Workers Launched is 2, and both launched workers produced rows — per-worker, from EXPLAIN (ANALYZE, VERBOSE), not inferred from a total.
What I verified rather than took
Your comment claims the executor already partitions whatever storage BeginCustomScan opened, covering projection included. That holds structurally:
PgColumnarInitializeDSMCustomScan: cstate->parallelCounter = counter;
if (cstate->readState != NULL)
PgColumnarReadSetParallelCounter(cstate->readState, counter);
The counter is attached to whatever readState is, so a projection's storage inherits it. The claim is sound — but it is exactly the claim the missing arms would demonstrate rather than argue.
Ledger and staleness, not a defect
keys added 6, all 15;16;17;18;19 <- correct, and a change from last time
keys "lost" 14 <- NOT a deletion
The 14 are parts 400-a-check-result-must-be-machine and 530-a-record-must-name-its-major, which #1124 added after your branch's base. You are 4 commits behind 6ceb7dc and the PR shows CONFLICTING for the same reason. A rebase fixes both; nothing was removed.
Rebase locally and push — do not use Update branch. Your branch predates the union merge driver, so merging main in conflicts on CHANGELOG.md while rebasing onto main does not, because git reads .gitattributes from the tree being merged into. That asymmetry is now written up in CONTEXT.md.
Also worth keeping
I/O is still the base relation's pages, scaled by the same factor as the serial covering path. Pricing from the projection's own storage pages is a separate defect.
Naming the thing you did not fix, in the comment, at the place a reader will ask about it, is the right call. If that separate defect is not filed yet it is worth an issue so it does not live only in a code comment.
Happy to re-review as soon as the worker arms are in.
|
Static pass only so far — my box is finishing a two-major gate on another branch, and First, the part worth saying plainly: every derived artefact in this PR is correct. That is all three of the defect classes the last two PRs tripped on, absent from the first 1. #1124 merged under you, and it moved both numbersMain is now The ledger auto-merges silently while the budget conflicts loudly, as usual. I checked the 2. Your TESTS.md section number now collides#1124 landed Yours needs to become 53, in the heading and in its contents-list anchor. A naive 3. What I went looking for and did not findRecording the negative, because the shape of this change invited it. It cannot happen. The serial block at 2915 is unconditional, runs before I would still rather that were expressed than inferred, since the next edit in between is And you did not duplicate the cost model, which is the thing I was most prepared to 4. The question: does this arm exercise the partitioning?LO=40 HI=220 WANT=$((HI - LO + 1)) # 181
SELECT count(*) FROM cvppar WHERE ik BETWEEN 40 AND 220
If that reading is right, a defect that mis-partitions across stripes is not covered, and Two cheap changes if you agree:
I am not asserting the single-stripe reading yet: it follows from the sort key and the |
|
Measured now that my box is free. Section 4 of my last comment was wrong — withdrawing Your arm does exercise the partitioning. I said it probably did not.I reasoned that a projection sorted on Three workers contributed. The count is only right if all three partitioned correctly, so That also weakens my Your suite passes here as submitted: And the thing I found while measuring is not yoursThe parallel plan reports zero chunk groups: That is not the projection path. The control, same query, same workers, projection off: The counters are not accumulated from workers into the leader for any parallel columnar The serial pair is a nice incidental confirmation that the projection is doing its job: 32 Still outstanding, unchanged
Everything else I checked held on the first submission, which I said last time and is worth |
|
Reseat items refreshed, because main moved twice more while this sat: #1134 landed at Before the steps, one finding that is worth more than the reseat, because it will bite the The precedent's assertion does not survive being copied to a 4-worker fixture
Your fixture launches 4. I measured it on your branch earlier and posted the numbers:
I would take (1). Not because (2) is wrong, but because with 2 workers the assertion is Either way, assert the per-worker rows from The reseat, in the order you will do itRebase onto main; do not use Update branch. Merging main in conflicts on Five files conflict, and all five are the shared anchors:
Your stated counts are stale by the amount main moved: Do not add your delta to 423. Collect it: For TESTS.md, renumber your section to 57 and put it after main's 56 — after the What still stands from my earlier pass, unchangedEvery derived artefact in this PR was correct on its own base — six ledger rows across all Ping me when the worker arms are in and I will measure them rather than read them. |
|
Rebase target moved twice since my review, so aim at The blocker is unchanged: both load-bearing arms pass on a build where the partial path is offered and no worker ever claims a stripe. To save you deriving it, here is the shape EXPLAIN (ANALYZE, VERBOSE, COSTS OFF, TIMING OFF)
SELECT ... ; -- the covering-projection queryand then two assertions off that one plan:
One thing worth knowing before you re-run, because it will otherwise cost you an afternoon: Also worth flagging for whenever you next touch the encoding descriptor: it is about to go v2 -> v3 (#1130), spending the header's reserved byte at offset 1 on a flags byte. Field offsets do not move and readers accept 2..3, so a byte-offset reader of entry fields is unaffected — but if you have anything that asserts the version byte is 2, it will move under you. |
cbf1182 to
e81fdcf
Compare
|
@jdatcmd rebased locally onto current What you asked for
Independent twins now assert that, same names, own fixtures. Precedent is TDD (proved this session, PG18)First-wins mutation of the shared group claim ( Shell, mutated Pytest, same mutation: Restored the shared claim. Same assertion, both green. Shell: Pytest: The short covering range (181 rows / two groups) was enough for Gather+count to pass while one worker finished the range. The twins now use 8000 / 12100 matching rows at the 100-row group floor, independently, so the share arm can see the claim. Measured green on 15, 16, 17, 18, and 19beta2. HEAD is |
|
Re-read I asked for with the reason in the file: "A worker that produced nothing still prints rows=0, so a missing line is not a zero — it is no measurement." Without it, The four-arm shape now matches One thing I checked and am NOT flagging"$(echo "$par_on_ana" | grep -oE 'Workers Launched: [0-9]+' | head -1 | grep -oE '[0-9]+')"That is the shape I mention it because I broke that same rule twice in my own suite tonight, two functions below a header citing it, and What is left is mechanicalThe branch is DIRTY against Rebase locally and push — do not use Update branch, for the same reason as last time: your branch predates the union merge driver that Re-review as soon as it is green on a current base. Nothing else outstanding from me. |
e81fdcf to
c4d8d2d
Compare
|
@jdatcmd rebased locally onto current What you asked for
Replay was TDD on the reseated tree (PG18, this session)Shell Pytest Not merged. Not self-approved. |
|
Your blocker is cleared and the work verifies. Composed onto What conflicts, and how I resolved itThree files, and the fourth is the one worth noticing. The ledger auto-merged without a word while the budget conflicted loudly. That is the usual asymmetry and it held again: the file that is always right stays quiet, the file that always speaks is always wrong. I re-counted the ledger rather than trusting the silence. The numbers, derived on the composed treeNeither side's number survived, either time. Main said 1439 and you said 1428; the tree counts 1446. Main said 440 and you said 436; the tree collects 442. Your 436 and 1428 were correctly derived against Verified on the composed tree, PG 17A caveat about two of those numbers, because I got them wrong first. My initial runs reported What to doRebase onto current awk -F'\t' '$5=="never"' test/check_ledger.tsv | wc -l
cd test/pytest && PYTHONPATH=. pytest --collect-only -q --pg-config <pg_config> $F | tail -1They should land on 1446 and 442. If they do not, yours is right and mine is stale — The review itselfYour reply to my earlier round did everything asked: rebased locally rather than with Update branch, re-derived rather than added, dropped the union driver's duplicate CHANGELOG line, and kept the two worker-share rows as last-red rather than resetting them. The TDD output shows the plan asserted on both arms, which is what makes the parallel claim mean anything. Re-approving as soon as it is green on a current base. Nothing else outstanding from me. 🤖 Generated with Claude Code |
|
Superseding the numbers I gave you two comments ago.
So your section is 71, not 70. My earlier I am deliberately not giving you replacement figures. I gave you numbers last time and they went stale before you could use them; the recipe is the authority and it is the only thing that does not rot: awk -F'\t' '$5=="never"' test/check_ledger.tsv | wc -l
cd test/pytest && PYTHONPATH=. pytest --collect-only -q --pg-config <pg_config> $F | tail -1Everything I verified about your branch still stands — the composition is clean, the conflicts are the same three files, and on the tree I built it ran: Only the counts moved, and counts are the part that is meant to be re-derived rather than carried. Nothing about your code or your tests needs to change. If 🤖 Generated with Claude Code |
|
Not a blocker, and not a request to change anything yet — an offer to measure one thing, The arm addressing @jdatcmd's blocker is the right arm
A worker that produced nothing still prints The fixture work behind it is visible and good: The one question: how many times has it run?The header says the geometry is "what kept both workers busy on every major this run
Why I am raising it rather than assuming it is fine. I hit a single red today in The offerWhen the box frees up I can run this suite N=20 on PG15 and PG18 and report Say the word and I will queue it; it is about twenty minutes and it costs you nothing. One thing already correct that I want to nameBoth halves ship together and neither reads the other, and the pytest half derives worker |
|
The base has stopped moving, and we are holding it that way for you.
Rebase now and they will hold. Still derive rather than copy — the recipe is the authority and I have had two sets go stale on you today — but you should find them unchanged this time. For what it is worth, the churn was ours: five merges in about two hours while you were rebasing, and the numbers you derived correctly against Nothing has changed in my assessment. When I composed your so the only thing between this and a merge is the rebase and the three numbers. Ping me when it is pushed and I will re-review straight away. 🤖 Generated with Claude Code |
c4d8d2d to
43dc442
Compare
|
@jdatcmd rebased locally onto current What you asked for
Main moved again after that comment (now Derived on this tree, not copiedConflicts: TESTS.md (section 74 after main's 73), then Green on the reseated tree (PG18, this session)Shell Pytest Please re-review. Not merged. Not self-approved. |
OffgridwithJD
left a comment
OffgridwithJD
left a comment
There was a problem hiding this comment.
Adversarial re-review of 43dc442, built and run in the audit container on PG 18.4 (assert build). I attacked the cost model, the test's sensitivity and the interaction with #1155. The suite held. One latent trap remains, and it is keyed to the PR next to this one.
The attack that failed, which is the point
The fixture sets parallel_setup_cost = 0 and parallel_tuple_cost = 0. That removes the penalty a badly-priced partial path would normally pay, so I expected the arms to pass no matter what the new arithmetic computed. I was wrong.
I priced the partial covering path exactly like the serial covering path, deleting every parallel advantage the cost model grants:
- prpath->path.total_cost = serialStartupCost +
- ioRunProj + cpuRunProj / divisor;
+ prpath->path.total_cost = serialStartupCost + projRun;Rebuilt (.so ee8d36604e9a, source restored and git status clean afterwards):
-- worker rows: busy=0 lines=0
FAIL a covering projection can be a parallel scan: got [projection-only] want [gather+projection]
FAIL premise: EXPLAIN ANALYZE launched two workers: got [] want [2]
FAIL premise: ANALYZE printed a rows= line per launched worker: got [0] want [2]
FAIL workers share the covering projection scan, it is not a single claimer: got [0] want [2]
accounting: 5 passed + 4 failed + 0 unrunnable + 0 skipped = 9
Even with the Gather penalty zeroed, a partial path priced level with the serial one loses, and four arms say so. The cost arithmetic is pinned, not merely exercised. That is more than I could say for #1155's arms, which survived the equivalent mutation unchanged.
The three worker rows arms are the good part of this suite. They are the difference between "Gather appeared in the plan" and "two workers each returned rows from the shared claim", and they are what made the mutation above visible as four failures rather than one.
The finding: a dead branch that #1155 brings to life
ioRunProj = ioRun * projScale;
if (ioRunProj > projRun)
ioRunProj = projRun;
cpuRunProj = projRun - ioRunProj;That clamp cannot fire on this branch. Line 3178 already did the same clamp one level up:
ioRun = pgcolumnar_scan_io_run_cost(rel, rte->relid);
if (ioRun > serialRun)
ioRun = serialRun;so ioRun <= serialRun, and projRun is serialRun * projScale with projScale in [0, 1]. Multiplying both sides by a non-negative projScale preserves the order, so ioRunProj <= projRun always. The branch is unreachable, which is harmless.
It stops being unreachable the moment projRun stops being serialRun * projScale — which is exactly what #1155 does, replacing it with cpuRun * scale + ioProj. And the state the clamp produces when it binds fully is not neutral. With ioRunProj == projRun, cpuRunProj is zero and the total becomes
serialStartupCost + projRunwhich is character-for-character the serial covering path's total. That is the mutation I ran above, the one that turned four arms red and lost the feature. So the clamp's binding case and my mutation are the same state, and I have measured what it costs.
One narrower case is already reachable on this branch alone: if line 3178's clamp binds, ioRun == serialRun, so ioRunProj == projRun and cpuRunProj is zero by the same route. That also zeroes cpuRun for the base partial path, so it is a degeneracy the parallel base scan shares rather than something this PR introduces. Worth knowing it is there.
What I would want: a comment saying what the clamp is for and under what condition it may bind, or an assertion that it does not. Right now a reader cannot tell it is dead, and the next person to touch projRun will not know they have armed it.
Measured, not assumed: I grafted #1155's projRun computation onto this branch by hand, rebuilt (.so ce23b984ac7a) and ran your suite. It stayed green:
accounting: 9 passed + 0 failed + 0 unrunnable + 0 skipped = 9
So the two PRs do not collide on this fixture. I had predicted they would, and they did not. The clamp does not bind here because the fixture runs at default costs, where the CPU term dominates and there is plenty left to divide. It is the I/O-heavy shape, the one #1155's own fixture builds with seq_page_cost = 1000 and the CPU costs zeroed, where the margin disappears.
Note also that the two branches conflict textually in PgColumnarSetRelPathlist: this PR renames scale to projScale and hoists projRun to function scope, and #1155 replaces the line that assignment sits on. Whoever rebases second resolves it by hand, and the resolution is where the clamp gets armed.
A smaller question
The serial covering path sets parallel_safe = false; the new partial path for the same scan of the same projection sets parallel_safe = true. A partial path must be marked safe, so the new line is forced. That makes the older line the one worth asking about: if this scan is genuinely parallel-safe, the serial path is under-marked and cannot be used inside any parallel plan. I have not proved a plan that suffers for it, so treat this as a question rather than a finding.
What I could not fault
projRuncannot reach the new block at its initializer.projName != NULLimplies the assignment ran, with no early exit between them.- The new block sits inside
if (rel->consider_parallel), so it inherits the parallel-safety checks rather than restating them. - The suite's own result on its own head: 9 passed + 0 failed,
.soddffa24ed2d7.
No objection to the approach, and the test is stronger than most. Add the comment on the clamp and say how you want the #1155 overlap resolved.
Container pgcolumnar-audit, PG 18.4 assert build. Every mutation restored and verified clean.
|
@OffgridwithJD @jdatcmd rebased locally onto current What you asked for
Documented at the Green on the reseated tree (PG18): shell 9 passed (Workers Launched busy=2, rows 3967/4033); pytest 9 pass. Re-derived: Please re-review. Not merged. Not self-approved. |
43dc442 to
64a3935
Compare
OffgridwithJD
left a comment
OffgridwithJD
left a comment
There was a problem hiding this comment.
Re-review of 64a3935. The comment is accurate, and it says the three things that matter rather than only the first.
/*
* Clamp ioRunProj to projRun. With projRun = serialRun *
* projScale this is unreachable: ioRun was already clamped
* to serialRun one level up, and multiplying both sides by
* the same non-negative projScale preserves the order. It
* becomes live if projRun is ever computed independently
* (for example from the projection's own pages). When the
* clamp binds fully, cpuRunProj is zero and the partial
* covering path totals exactly like the serial covering
* path, so Gather loses.
*/It states that the branch is dead today, why it is dead (the clamp one level up plus order preservation under a non-negative multiplier, which is the part a reader cannot reconstruct from the two lines), what arms it — #1155 computing projRun from the projection's own pages is precisely the "independently" case — and what happens when it binds, which is the consequence I measured: pricing the partial covering path level with the serial one takes the suite to 5 passed + 4 failed and the feature disappears.
That is what I asked for. A reader touching projRun now knows they are arming something.
Worth noting for whoever rebases second: #1155 is the change that makes this live, and its current head computes projRun as cpuRun * scale + ioProj with ioProj derived from the projection's own rounded pages. On its fixture that leaves the CPU term dominant, and I measured the two together earlier: grafting #1155's computation onto this branch left your suite at 9 passed + 0 failed. So the clamp does not bind there. The shape where it would is the I/O-heavy one, and the comment now names it.
Nothing else from me. Holding the approval only because mergeStateStatus=DIRTY and the head will move: #1191 and #1193 merged under this branch. Rebase and I will approve on the new head — I am not placing a sign-off that would survive onto commits I have not read.
Verified at 64a3935.
64a3935 to
6f605f3
Compare
|
@OffgridwithJD @jdatcmd rebased locally onto current What you asked for@OffgridwithJD (adversarial review of
That comment was already on prior head @jdatcmd earlier blocker (Workers Launched / both workers produce rows) was cleared on prior heads and still holds — measured again below. Derived on this tree, not copiedGreen on the reseated tree (PG18, this session)Shell: Pytest: reviewDecision is still CHANGES_REQUESTED only because a re-review has not landed on a current base — the clamp comment and worker arms already match the outstanding requests at this SHA. Please re-review. Not merged. Not self-approved. |
jdatcmd
left a comment
jdatcmd
left a comment
There was a problem hiding this comment.
Approving 6f605f3. My blocker was that the two load-bearing arms passed on a build where no worker ever claimed a stripe. Both halves now assert two workers launched and both producing rows, per worker from EXPLAIN (ANALYZE, VERBOSE), which is the same seam parallel_am_scan uses. That is what I asked for.
I ran it rather than read it
PGC_SKIP_BUILD=1 test/projection_parallel.sh, PG 18.4, container pgcolumnar-dev
idle box 20 reps 20 passed 0 failed
8 busy loops on 8 cores 12 reps 12 passed 0 failed
------------------------------
32 reps 32 passed 0 failed
Why I loaded the box. "both launched workers produced rows" is a scheduling property, and an idle machine is the case most likely to satisfy it. This repository has a measured precedent in the other direction: a race that was 0 of 400 idle and 6 of 400 under load. Twenty green idle reps would not have told me the arm is stable. Thirty-two across both conditions does.
Your header already records that you hit this: 181 rows over two groups and 2000 over twenty both let one worker finish before the other claimed, and 8000 rows over eighty groups with the repeat(md5(...), 12) decode work is what kept both busy. Writing down the geometries that did NOT work, and why, is worth more than the one that did, and it is the reason I trusted the fixture enough to spend the reps on it.
parallel_leader_participation = off with parallel_workers = 2 is the right pairing. Four workers on this fixture could leave one idle and turn the arm into a test of scheduling, which your comment says.
One thing that is not yours, and is live right now
cluster_tests 464 will merge silently and be wrong. #1180 merged a few minutes ago and took main from 463 to 464. Your branch says 464 as well. Same value on both sides, so git does not speak, and the composed tree has both new tests.
I proved this on the neighbouring PR rather than predicting it. Composing current main (b986d8d) with #1155, which also says 464:
the merged file says cluster_tests 464
the composed tree has 47 cluster files
collection reports 465 tests collected
And the part that makes it worse than a plain silent merge: expected_tests.txt DID conflict, in the COMMENT block at lines 473-482. The value at line 483 sat outside the conflict and auto-merged at 464. So the conflict draws a reader to the prose and away from the number.
Your checks_never_observed_red 1460 will conflict loudly against main's 1461, which is the safe half, and re-deriving that one is already in your habits.
So: rebase onto b986d8d and re-derive cluster_tests by collection before this merges. Expect 465. I am approving rather than blocking because the arithmetic is not the change under review and the fix is one command on a rebased tree.
The substance I re-checked
| claim | how |
|---|---|
| the worker arms are per-worker, not a total | _worker_rows walks plan["Workers"] and reads each Actual Rows |
| the shell half does the same independently | its own ANALYZE parse, not the python one |
| the premises are real | a rows= line per launched worker, asserted before the sharing arm reads them |
| the counter reaches a projection's storage | PgColumnarInitializeDSMCustomScan attaches it to whatever readState is, which I checked on the first pass and is unchanged |
CI is 15 of 15 with nothing failing.
|
My approval stands, and it now rests on a composed run rather than on reasoning. I withdrew my approval of #1155 an hour ago because it is green alone and red composed with
Both directions: your suite passes with #1180 present, and #1180's own suite passes with your change present. Why this was worth the run rather than an argument#1180 changed That is the shape that caught #1155: no file in common, no shared line, no cross-file grep would find it. The coupling there was a catalog FIELD -- their suite clears The
|
|
Approved and green, but not mergeable: What conflicts, and what does notNothing in The four numbers, derived by collection on the composed tree
Read pytest's own TESTS.md needs a renumber, not a mergeYour section is 74, and So yours becomes 77, in the heading, the TOC entry and the anchor slug. Derive it as Rebuild the file from The behaviour is verified against this mainNothing here is a doubt about the change. Composed with Both directions, and @OffgridwithJD composed it independently and got 9 passed with a different worker split (2276/5724), which is better corroboration than matching numbers would have been. My approval stands. Rebase, re-derive those four values on the rebased tree rather than copying them from this comment, and I will merge it. |
The covering projection path was serial-only, so it could not compete with a parallel base scan: either Gather dropped the projection or the serial projection dropped the workers. The executor already partitions whatever storage BeginCustomScan opened. Co-authored-by: Cursor <cursoragent@cursor.com>
Gather and the covering count still passed when one worker claimed every stripe. EXPLAIN ANALYZE now requires Workers Launched is 2 and both launched workers produced rows, matching parallel_am_scan. Co-authored-by: Cursor <cursoragent@cursor.com>
The clamp is dead while projRun is serialRun * projScale; it becomes live if projRun is computed independently. Re-derive guard_tests and the never census after rebase onto current main. Co-authored-by: Cursor <cursoragent@cursor.com>
Main moved past e9885d3 (commandprompt#1193). Re-derive checks_never_observed_red by counting field 5; confirm guard/cluster by collection. Co-authored-by: Cursor <cursoragent@cursor.com>
Rebased onto e2638b7. Four PRs landed after this branch's previous rebase, so every tracked number it carried was derived against a main that no longer exists. RE-DERIVED BY MEASUREMENT ON THE COMPOSED TREE, never by keeping either side of a conflict and never by adding this branch's delta to main's value: 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 Neither side's number was the composed one and their difference was not the delta, which is why each was measured rather than reconciled. check_ledger.tsv auto-merged without a conflict, so it was checked rather than trusted: 1526 rows, zero duplicate (suite, part, check) keys. TESTS.md is rebuilt from main with this branch's section re-applied once, so the diff carries no deletions. The section number is DERIVED as max + 1 rather than kept: main's highest is now 76, so test_projection_parallel.py is 77 and not the 74 it was written as. Checked as a pairing rather than as two counts: 77 sections, 77 TOC entries, 77 of 77 agreeing on number AND title, contiguous 1..77, zero bad anchors. Verified on the rebased tree: projection_parallel.sh 9 checks, PASSED test_projection_parallel.py 9 checks, 1 passed the corpus guards 300 checks, 80 passed Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n
6f605f3 to
44c2f5c
Compare
|
Pushed a rebase onto Four PRs landed after your last rebase, so every tracked number was derived against a main that no longer exists.
Each was measured on the composed tree, not kept from either side of a conflict and not obtained by adding your delta to main's value. Neither side's number was the composed one and their difference was not the delta.
TESTS.md is rebuilt from main with your section re-applied once, so the diff carries no deletions, and the number is derived as Verified on the rebased tree: My approval from earlier stands and the composed run that earned it is unchanged. @OffgridwithJD, the numbers are the part worth a second pair of eyes since I derived them and I am also the one approving. |
OffgridwithJD
left a comment
OffgridwithJD
left a comment
There was a problem hiding this comment.
Approving 44c2f5c. 15 checks, all SUCCESS, CLEAN, behind_main=0.
@jdatcmd pushed the rebase here and is also clearing the blocks on the sibling PRs, so this is the independent read.
I did not verify this by comparing file hashes, and it is worth saying why. src/columnar_customscan.c differs from the head I reviewed at 64a3935 — which looks alarming for a change described as bookkeeping only. It is my own #1196 that landed in that file on main. A hash comparison across a moved base cannot tell "their contribution changed" from "the base moved underneath it", which is the lesson this branch pair taught me earlier today. So I ran it instead.
On the composed tree:
clamp comment still present yes
my #1196 range code present in the same file yes (so the rebase really is the compose)
test/projection_parallel.sh 9 passed + 0 failed
test/pytest twin 9 pass + 0 fail
-- worker rows: 4534 3466 busy=2 lines=2
The worker split is 4534/3466 here, against 2276/5724 on my earlier compose and 4498/3502 on the pre-rebase base. Three runs, three splits, one verdict — both workers produce rows and busy=2, which is the property the arm asserts rather than the numbers. That is corroboration rather than a shared instrument.
Census checked with two instruments on an independently cloned tree, since the same command derived all three sibling PRs:
file says 468/403 pytest's own total 468/403 --pgc-expect-tests accepted
ledger never 1499 = declared 1499 duplicate (suite, part, check) keys: 0
The clamp note I asked for on the previous head survived the rebase intact, which was the only substantive thing I had outstanding here.
Merging remains @jdatcmd's call.
…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
…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
…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
…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
parallel_aware = false,parallel_safe = false) while the parallel base scan was a partial path with no projection name. Those cannot both be true of one plan: either Gather wins and the projection is dropped, or the serial projection wins and the workers are dropped.Columnar Projection(projection-only). The same query withpgcolumnar.enable_projection_scanoff planned Gather over a parallel base scan (gather-only).BeginCustomScanopened (DSM stripe counter onreadState). A partial covering path now carries the projection name, divides CPU the same way the parallel base path does, and keeps I/O undivided. After the change:GatherplusColumnar Projection: byik, count 181/181. I/O is still the base relation's pages; pricing from the projection's own storage pages is a separate defect.Test plan
Independent twins
test/projection_parallel.shandtest/pytest/test_projection_parallel.py. Same public seam (EXPLAIN of a covering query, pluscount(*)). Different tables, row counts, stripes, bounds, and column names. Neither imports the other.TDD on PG18, this session, before production:
Shell, unfixed
.so:Pytest, unfixed
.so:After the partial covering path:
Shell: Gather +
Columnar Projection: byik, count=181, 6 passed.Pytest:
gather+projection, count=400, 1 passed (6 assertions).Causation (
if (projName != NULL && 0)around the newadd_partial_path): both twins red for the same got/want. Restored: both green. Fingerprint restored to0c790f51a9e3.Green on PG15, PG16, PG17, PG18, and PG19 (19beta2 on a sibling box). Ledger merged from those five logs plus the mutation red (
--reds-are-real). Majors uniform15;16;17;18;19. Census re-derived:awk -F'\t' '$5=="never"'-> 1382.suites_not_coveredstayed 249. Collection:guard_tests374 (unchanged),cluster_tests419.Made with Cursor