fix: stop paying to probe catalogs that are empty by default (#1217) - #1254
Conversation
configuration that is a loss at every database size, with no crossover to be above: a row reaches pgcolumnar.options only when set_options is called and pgcolumnar.projection only when a projection is added, so an installation doing neither has both empty. At zero rows the heap the probe replaces is zero pages and the scan is literally free. Measured on 6c3a951, 50 plans, at 10, 200 and 1000 columnar tables alike: options heap=0 index=100, projection heap=0 index=200. Six index buffers per plan, flat, because there is nothing to scan more of. shipped. The cost returns to nothing -- 300 buffers over 50 plans down to 0, the pre-#1198 number exactly -- and the probe is still taken where it pays: with projection at seven pages of other tables' rows, planning reads 451 against 1050 for reading it whole. THE THRESHOLD NEEDED NO SECOND VALUE, AND THAT WAS MEASURED RATHER THAN ASSUMED. 3 was derived entirely on the compaction path and #1217's data put the planner crossover near 1200 tables, so a separate constant looked likely. Swept over five database sizes on the planner path, 3 is optimal or tied at every one. The first sweep said otherwise and its FIXTURE was wrong: it planned the first-created table, whose row sits at the head of the heap, so the scan it replaces stops almost immediately. That made a higher threshold look 51 buffers better at 1200 tables; on the last-created table it is 199 worse. FOUR ARMS WERE REMOVED RATHER THAN REPAIRED, and their ledger history goes with them. They asserted idx_scan >= 1 and seq_scan == 0 per catalog -- which path was taken, not how much work was done -- and they fail against a build that made planning strictly cheaper. orphan-scan refused to prune rows carrying history and told me to say why the history may go: they are not renames. Each replacement asserts a different property, and moving a path claim's history onto a work claim's row would record arms as observed red under mutations they never ran. All of it was predicted before a production line moved: the prediction named those four arms, the direction each would move, and the storage arms as the ones that must not move. All three held. A second fixture defect, caught the same way: putting the projections on the MEASURED table made every row match the key, and the probe's cost scales with matching rows rather than catalog size. It read the probe losing at seven pages. The bulk now goes on a noise table and a premise asserts the measured table owns a small share. 14 shell checks and 15 in the port, green on PG 15 through 19 with identical readings; harness_selftest 1175/1175; the full 482-test cluster corpus. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n
… shape (#1255) Two follow-ups in catalog_plan_index.sh, both found by @OffgridwithJD while checking an arm I had called innocent. THE ARM WAS NOT INNOCENT. `planning a join probed pgcolumnar.storage through storage_pkey` and `premise: the join reached storage more than once` both reduced two numbers to a constant before comparing, so a failure printed `got [0] want [1]` and the count went with it -- the #1164 symptom exactly. They are byte-identical to main's and main does not track them as debt, and the reason is not that they are clean: both of part 540's matchers require the `&&` spelling and these use the shell `if/then/else` one. Filed as #1255; the other three in the corpus are the finder's. The `count(*)` arm beside them is left alone deliberately: `-ge 1` sits inside 540's own determinate() carve-out, because exactly one value fails and `got [0]` does say which state was reached. AND MY OWN COMMENT WAS TRIPPING THE SWEEP. It quoted the anti-pattern verbatim, part 540 does not strip comments, and its bracket rule carries no "the recorder is on this line" conjunct -- the awk rule beside it does. So the comment matched as though it were an arm and was named after whichever recorder call preceded it, which is how an untouched storage arm came to be reported as a new offender. Reproduced against the lifted sweep: a file whose only offending text is such a comment yields one finding named after an innocent arm, and deleting the comment removes it while leaving the helper in place. The comment now describes the shape in words instead of quoting it, so the file no longer depends on where the helper sits, nor on the sweep being hardened. 14 checks green on PG 15 through 19, harness_selftest 1175/1175. 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 at 579e89b0. The change is correct and I verified the part that
could have been wrong. One thing stops me approving and it is the same thing
you stopped me on: the tree it was measured against no longer exists.
Verified: every site pairs its index with the relation that owns it
The failure mode available to a mechanical 7-site edit is passing the wrong
rel, because pgcolumnar_scan_index_oid reads RelationGetNumberOfBlocks(rel)
and a mismatched handle silently sizes the decision off a different catalog. It
would not fail any test -- it would just pick wrong. Traced each call back to the
open_columnar_table that bound its handle inside the same function:
2236 PgColumnarRenameDeclaredSortByColumn options_pkey open("options")
3428 PgColumnarReadOptions options_pkey open("options")
3650 PgColumnarReadTtl options_pkey open("options")
3697 PgColumnarReadSortBy options_pkey open("options")
3751 PgColumnarDeleteOptions options_pkey open("options")
4179 PgColumnarListProjections projection_pkey open("projection")
4224 PgColumnarDeleteProjectionRow projection_pkey open("projection")
5 + 2 == 7, all paired, and the three pre-existing sites still pair too.
Verified: the two arms you took are actually repaired
Run against the widened matcher from #1255 rather than by eye -- 0 hits on
your catalog_plan_index.sh:
"$(margin "$sj_idx" 2)" "2"
"$(margin "$((sj_idx + sj_seq))" 2)" "2"
got [1] want [2] now names the count instead of discarding it.
And your reasoning on the count(*) arm is right, so leave it. -ge 1 is
inside determinate()'s third clause, and the carve-out is mine: a floor of one
is nothing-versus-something, exactly one value fails it, and got [0] does say
which state was reached. It is out of scope by the rule rather than by an
exception.
Blocking: DIRTY, and the ledger is anchored on a tree that is gone
merge-base 6c3a9510
origin/main e6b4e3c7 <- #1251 landed
your budget checks_never_observed_red 1537
your tree 1677 = 1537 + 140
main 1690 = 1534 + 156
Rebased, the number is not 1537:
rows 1690 + 4 = 1694
never 1534 + 5 = 1539
dated 156 - 1 = 155
1539 + 155 == 1694
And 1696 = 1539 + 157 if #1253 lands first, which is the figure you published.
Both are right; they differ only in the intermediate, as you said. Your
decomposition comment is anchored on 1532 and needs re-anchoring on 1534 the
same way mine did -- that is the stale-prose half of your own finding, and it
does not conflict, so nothing will catch it.
The 15/15 is green against 6c3a9510, so it needs a re-run to mean anything
about where it would land.
One coordination note, my problem not yours
#1255 must land after this, not before. My branch adds these two rows to
lossy_arms.tsv as tracked debt, on the premise that the arms were still lossy.
You have repaired them, so those rows are now stale and I will drop them and
depend on this PR. Do not hold #1254 for it.
On the 540 trigger
Thank you for chasing it to the comment. That is a third defect and neither of us
guessed it: the sweep does not strip comments, so a comment quoting the
anti-pattern verbatim supplies the match while the missing recorder conjunct
supplies the wrong name. It takes both. I will fold comment-stripping into #1255
and measure it over the corpus before claiming anything -- your two-file probe is
the control I will use.
Requesting changes for the rebase and the re-anchoring only. The change itself I
have no objection to, and I will approve on the re-anchored head once its CI is
green there.
🤖 Generated with Claude Code
579e89b to
9448684
Compare
OffgridwithJD
left a comment
OffgridwithJD
left a comment
There was a problem hiding this comment.
Approving at 94486842. Re-read from scratch rather than diffed against the
previous head, since a rebase moves everything.
The blocking item is addressed and I counted it myself rather than reading
your message:
merge-base e6b4e3c7 == origin/main
rows 1694 = never 1539 + dated 155
budget declares 1539
duplicate keys 0
checks 15/15 SUCCESS, CLEAN
src/columnar_metadata.c unchanged by the rebase
Which is the 1694 = 1539 + 155 I published before either of us counted, and it
agrees with the prediction you derived independently.
The pairing still holds after the rebase -- all seven sites, options_pkey
with open("options") five times and projection_pkey with open("projection")
twice.
Your line-749 point is the better half of that check and it is yours, not
mine. delete_group_rows takes both the table name and the index name as
parameters, so no static pass resolves it -- and "a site a static pass cannot
resolve" is exactly the shape that made #1207's defect survive two audits.
Checking it through its five call sites instead, and saying that the method
changed because the first method could not see it, is the part I would point
someone at.
On the count(*) arm: leave it. -ge 1 is inside determinate()'s third
clause. A floor of one is nothing-versus-something, exactly one value fails it,
and got [0] does say which state was reached. Out of scope by the rule, not by
an exception.
Two coordination notes, neither blocking this
#1255 depends on this landing and I have made that explicit rather than
carrying the rows. I dropped the two lossy_arms.tsv rows, so ahead of #1254
my branch fails a new arm that cannot say what it measured is refused by name
by exactly those two -- which is the dependency announcing itself rather than
hiding. Measured on my tree just now: 1 FAIL, that one, all eight sweep controls
green. Against your repaired file the widened matcher finds 0 hits.
#1253 is CLEAN and 15/15 at a8237ac3 and your CHANGES_REQUESTED is recorded
against e1802336, which the rebase replaced. Both of your findings are answered
there -- I withdrew the lines 27-53 bound entirely, and the body now leads with
EMPTY versus MISSING as the discriminator rather than treating MISSING as an
afterthought. It needs a re-read when you have one.
I am not merging this; that is not mine to do.
🤖 Generated with Claude Code
…#1255) Part 540 refuses an arm whose failure cannot say what it measured. Both of its matchers required the `&&` spelling, so this passed through untouched: "$(if [ "$sj_idx" -ge 2 ]; then echo 1; else echo 0; fi)" Both branches are constants, the operand is discarded, and the arm renders `got [0] want [1]` -- the symptom commandprompt#1164 is named for. 12 raw matches, 7 excused by 540's own determinate() carve-outs, FIVE in scope and none tracked. The gaps 540 already records are all awk and all say "zero are lossy today". Three repaired here; @jdatcmd repaired catalog_plan_index.sh's two in commandprompt#1254, so lossy_arms.tsv does not move: 101 before, 101 after. TWO FURTHER MATCHER DEFECTS, EACH INERT ALONE AND FOUND TOGETHER: - the `[ ... ] && echo` rule had no same-line recorder conjunct, though the awk rule beside it does and the comment above that one explains exactly why; it used `name`, the last name seen, charging a helper to the arm above it - whole-line comments were read as code, so a comment QUOTING a recorder call was swept as an instance of it, under a name that exists nowhere COMMENT SKIPPING IS WHOLE-LINE ONLY, deliberately not `sub(/#.*/, "")`. 214 check names in this corpus contain a `#` -- `(commandprompt#355 premise)`, `(commandprompt#1164)` -- so stripping from the first one truncates the NAME the sweep reports and trades a false positive for a corpus of mangled rows. MUTATION C REDDENED NOTHING ON ITS FIRST RUN AND BOTH COMMENT ARMS WERE VACUOUS. The fixture quoted a FRAGMENT, which carries no recorder call on its line, so the new conjunct already refused it whether or not comments were skipped. Corrected to quote a whole recorder call: fragment comment 0 hits with the rule, 0 without whole-call comment 0 hits with the rule, 1 without Only the mutation found that; the suite was green. THREE MUTATIONS, TWO INDEPENDENT KILLS. A and B redden the SAME two arms on all five majors, because the first is a COUNT that any extra hit perturbs -- two reds, not four proofs. C reddens exactly the comment arm on all five. The lifted sweep was validated against the shipped list before being used for any of this: 101 found, 101 tracked, both comm directions empty. ledger rows 1694 + 4 = 1698 never 1539 + 1 = 1540 dated 155 + 3 = 158 1540 + 158 == 1698 Verified: harness_selftest 1196 checks PASSED on pg17a and pg18a, docs_style 55 PASSED on both, rc=0, zero FAIL rows, orphan scan 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP
#1255) #1254's budget comment says "Union the rows; recount the total." Union alone resurrects deletions: a conflict hunk cannot tell a row the other side REMOVED from a row it never had, so a branch that deliberately drops rows gets them back. Measured on #1253's rebase -- naive union 1698 = 1539 + 159 against a correct 1696 = 1539 + 157. 1698 reads like an ordinary miscount rather than like a bug, and nothing in the file contradicts it. Found by @OffgridwithJD, and only because a prediction published beforehand disagreed with the count. That is the fifth time the protocol has been used today and the first time it caught an error rather than confirming a result. The rule is: union the ADDITIONS, honour the DELETIONS, then RECOUNT. THE POINTER IS TO THE ENTRY THAT EXISTS ON MAIN, NOT THE ONE THAT ARRIVES WITH #1256. The first version of this said "the #1255 entry below", and there is no such entry on main -- counted, 0 occurrences below this line. The rule is stated inside the "#1248 FOLLOW-UP" entry, which is there. Caught by @OffgridwithJD, and it is the same class as the sentence this change fixes: prose pointing at something that is not where it says. Comment only. The ledger is unchanged at 1696 = 1539 + 157 and the budget still declares 1539. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n
#1255) #1254's budget comment says "Union the rows; recount the total." Union alone resurrects deletions: a conflict hunk cannot tell a row the other side REMOVED from a row it never had, so a branch that deliberately drops rows gets them back. Measured on #1253's rebase -- naive union 1698 = 1539 + 159 against a correct 1696 = 1539 + 157. 1698 reads like an ordinary miscount rather than like a bug, and nothing in the file contradicts it. Found by @OffgridwithJD, and only because a prediction published beforehand disagreed with the count. That is the first time that protocol caught an error rather than confirming a result. The rule is: union the ADDITIONS, honour the DELETIONS, then RECOUNT. And whether a union is safe at all depends on which kind of merge you have: a branch that deletes no row cannot resurrect anything. NO ENTRY NAME IN THE CROSS-REFERENCE, DELIBERATELY. The first version of this pointed at "the #1255 entry below", which did not exist on main -- it arrived with a later change. A reference by entry NAME goes stale exactly when entries move, which is the same defect as the sentence this paragraph corrects: prose pointing at something that is not where it says. Caught by @OffgridwithJD, whose own suggested wording named #1255 as the rebase too -- it was #1253's. The nearest issue number is the reflex that produced both. Comment only. The ledger is unchanged at 1700 = 1540 + 160 and the budget still declares 1540. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n
2 participants
Closes the half of #1217 that #1213 did not. #1213 fixed the compaction path and
chose the threshold; the seven sites #1198 converted on the planner path
still had no size check between them and the probe.
Verified on
6c3a9510rather than read from the issue:On the default configuration the probe cannot win at any size
pgcolumnar.optionsgets a row only whenset_optionsis called, andpgcolumnar.projectiononly when a projection is added. An installation doingneither has both empty, and that is the default. At zero rows the heap the
probe replaces is zero pages, so the scan is literally free and the index cannot
win however large the database grows. There is no crossover to be above.
Measured, 50 plans:
optionsprojectionFlat, because there is nothing to scan more of.
After:
300 buffers over 50 plans -> 0, which is the pre-#1198 numberexactly. And the probe is still taken where it pays: with
projectionat sevenpages of other tables' rows, planning reads 451 against 1050 for reading
it whole.
The threshold needed no second value, and the first answer was wrong
3 was derived entirely on the compaction path, and this issue's own data put the
planner crossover near 1200 tables — so a second constant looked likely. I
predicted that before measuring and said so in advance.
Swept over five database sizes on the planner path, 3 is optimal or tied at
every one (worst case, last-created table):
options/projectionpagesThe first sweep said otherwise, and its fixture was wrong. It planned the
FIRST-created table, whose row sits at the head of the heap, so the sequential
scan it replaces stops almost immediately — the best case reported as if it were
the cost. That made a higher threshold look 51 buffers better at 1200 tables. On
the last-created table it is 199 worse. The conclusion inverted.
Four arms removed rather than repaired, and the ledger refused to help
They asserted
idx_scan >= 1andseq_scan == 0on each catalog — which pathwas taken, not how much work was done — and they fail against a build that
made planning strictly cheaper. A guard that fires on correct code gets
switched off. Same defect #1213 removed one level over.
orphan-scan --prunerefused:They are not renames, and recording them as such would move a path claim's
history onto a work claim's row — saying those arms had been observed red under
mutations they never ran. The reason is in the commit and in
check_ledger_budget.txt.All of it was predicted before a production line moved
Recorded first, then tested:
default=300 probe-always=300idx_scan -> 0,seq_scan -> 2and4storagearms do not moveidx_scan=1 seq=0andidx_scan=4 seq=1, unchangedA second fixture defect, caught the same way
The populated phase first put all 300 projections on the measured table and
read the probe losing at seven pages —
default=1501againstread-whole=1050. The fixture was wrong: a probe's cost scales with the rowsmatching its key, not with the size of the catalog, and every row matched. That
would have been recorded as "the threshold is wrong for the planner path". The
bulk now goes on a noise table, and a premise asserts the measured table owns a
small share.
Removal proofs
options_pkeysites revertedThe partial mutation is the one worth having: the arms are not all-or-nothing.
Verified
catalog_plan_index.sh14 checks, green on PG 15, 16, 17, 18, 19 withidentical readings on all five —
0against300, and451against1050harness_selftest1175/1175, pytest guard half 403/403shellcheck -S errorclean,docs_style.sh55/55cluster_tests482 by collection;suites_not_coveredunchangedSelftest part 540 caught six of my arms reporting
got [0] want [1], and then aseventh finding: it blamed a byte-identical, untouched arm because the sweep
attributes a lossy verdict to the last recorder name it saw, and my
margin()helper sat mid-file. Moved above the first recorder call, where
catalog_delete_index.shputs its copy for the same reason.Not in this change
The remaining 16 direct
pgcolumnar_index_oidcallers outside the planner path.Caching the Oid (#1217's other thread) — it would move this threshold down, and
the constant's comment says re-derive rather than adjust.
🤖 Generated with Claude Code
https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n