From 0cce5066de21d0eaac0203840a5cc17943c7a0ca Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Thu, 24 Sep 2026 12:52:26 -0600 Subject: [PATCH 1/2] fix: stop paying to probe catalogs that are empty by default (#1217) 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 6c3a9510, 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) Claude-Session: https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n --- CHANGELOG.md | 47 +++++ src/columnar_metadata.c | 14 +- test/catalog_plan_index.sh | 224 +++++++++++++++++++---- test/check_ledger.tsv | 16 +- test/check_ledger_budget.txt | 48 ++++- test/pytest/TESTS.md | 20 +- test/pytest/conftest.py | 48 +++++ test/pytest/expected_tests.txt | 29 ++- test/pytest/test_catalog_delete_index.py | 49 +---- test/pytest/test_catalog_plan_index.py | 177 +++++++++++++++--- 10 files changed, 532 insertions(+), 140 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b662328..d4c0b129 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1080,6 +1080,53 @@ true until the next version shipped. first version passed on prose, green on a caller that consulted nothing, and one arm now builds that exact text and requires zero. `selftest/190` has the same shape on `pgc_build_needs_clean`, tracked in #1222. +- Planning paid to probe two catalogs that are empty in the default + configuration, and the probe could not win there at any database size (#1217). + + `pgcolumnar.options` gets a row only when `set_options` is called, and + `pgcolumnar.projection` only when a projection is added. **An installation + doing neither has both empty, and that is the default.** At zero rows the heap + the probe replaces is zero pages, so the sequential read it replaces is + literally free and the index cannot win however large the database grows. + There is no crossover to be above. Measured on `6c3a9510`, 50 plans: + + | columnar tables | `options` | `projection` | per plan | + | ---: | --- | --- | ---: | + | 10 | heap 0, index 100 | heap 0, index 200 | 6 | + | 200 | heap 0, index 100 | heap 0, index 200 | 6 | + | 1000 | heap 0, index 100 | heap 0, index 200 | 6 | + + Flat, because there is nothing to scan more of. + + #1198's seven planner-path sites now go through the size check that #1213 + shipped, and the cost returns to **nothing** -- 300 buffers over 50 plans down + to 0, which is the pre-#1198 number exactly. The probe is still taken where it + pays: with `projection` at seven pages of other tables' rows, planning reads + 451 buffers against 1050 for reading it whole. + + **The threshold did not need a second value, and that was not assumed.** 3 was + derived entirely on the compaction path, and #1217's own data put the planner + crossover near 1200 tables, so a separate constant looked likely. Swept across + 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 sequential + scan it replaces stops almost immediately. That made a higher threshold look + 51 buffers better at 1200 tables; measured on the last-created table it is 199 + worse. + + `test/catalog_plan_index.sh` (14 checks) and + `test/pytest/test_catalog_plan_index.py` (15). Four arms were **removed** + rather than repaired: they asserted `idx_scan >= 1` and `seq_scan == 0` on each + catalog, and they fail against a build that made planning strictly cheaper. + That is a guard firing on correct code, the same defect #1213 removed one + level over, and it was predicted before this change was written -- the + prediction named those four arms, the direction each would move, and the + `storage` arms as the ones that must not move, and all three held. + + The port runs on a private database (`pgc_own_db`, moved to `conftest.py` now + that a second file needs it), because these catalogs are per database and + inside the corpus other files have already populated them. + - Retiring a row group read five catalogs whole, once each per group, and the cost grew with every unrelated columnar table in the database (#1207). diff --git a/src/columnar_metadata.c b/src/columnar_metadata.c index 2a1a1007..b78b3b51 100644 --- a/src/columnar_metadata.c +++ b/src/columnar_metadata.c @@ -2233,7 +2233,7 @@ PgColumnarRenameDeclaredSortByColumn(Oid relid, const char *oldName, * cannot reach it. That is the reason to fix it rather than a reason not to. */ { - Oid optIdx = pgcolumnar_index_oid("options_pkey"); + Oid optIdx = pgcolumnar_scan_index_oid(rel, "options_pkey"); scan = systable_beginscan(rel, optIdx, OidIsValid(optIdx), NULL, 1, key); } @@ -3425,7 +3425,7 @@ PgColumnarReadOptions(Oid relid, PgColumnarOptions *opts) * asked about one of them. */ { - Oid optIdx = pgcolumnar_index_oid("options_pkey"); + Oid optIdx = pgcolumnar_scan_index_oid(rel, "options_pkey"); scan = systable_beginscan(rel, optIdx, OidIsValid(optIdx), snapshot, 1, key); } @@ -3647,7 +3647,7 @@ PgColumnarReadTtl(Oid relid, char **column, Interval **interval) ScanKeyInit(&key[0], Anum_options_regclass, BTEqualStrategyNumber, F_OIDEQ, ObjectIdGetDatum(relid)); { - Oid optIdx = pgcolumnar_index_oid("options_pkey"); + Oid optIdx = pgcolumnar_scan_index_oid(rel, "options_pkey"); scan = systable_beginscan(rel, optIdx, OidIsValid(optIdx), snapshot, 1, key); } @@ -3694,7 +3694,7 @@ PgColumnarReadSortBy(Oid relid) F_OIDEQ, ObjectIdGetDatum(relid)); { - Oid optIdx = pgcolumnar_index_oid("options_pkey"); + Oid optIdx = pgcolumnar_scan_index_oid(rel, "options_pkey"); scan = systable_beginscan(rel, optIdx, OidIsValid(optIdx), snapshot, 1, key); } @@ -3748,7 +3748,7 @@ PgColumnarDeleteOptions(Oid relid) F_OIDEQ, ObjectIdGetDatum(relid)); { - Oid optIdx = pgcolumnar_index_oid("options_pkey"); + Oid optIdx = pgcolumnar_scan_index_oid(rel, "options_pkey"); scan = systable_beginscan(rel, optIdx, OidIsValid(optIdx), NULL, 1, key); } @@ -4176,7 +4176,7 @@ PgColumnarListProjections(uint64 storageId) * those writes; dropping it would not. */ { - Oid projIdx = pgcolumnar_index_oid("projection_pkey"); + Oid projIdx = pgcolumnar_scan_index_oid(rel, "projection_pkey"); scan = systable_beginscan(rel, projIdx, OidIsValid(projIdx), NULL, 1, key); } @@ -4221,7 +4221,7 @@ PgColumnarDeleteProjectionRow(uint64 storageId, int projectionId) F_INT4EQ, Int32GetDatum(projectionId)); { - Oid projIdx = pgcolumnar_index_oid("projection_pkey"); + Oid projIdx = pgcolumnar_scan_index_oid(rel, "projection_pkey"); scan = systable_beginscan(rel, projIdx, OidIsValid(projIdx), NULL, 2, key); } diff --git a/test/catalog_plan_index.sh b/test/catalog_plan_index.sh index 2c0d60f6..54deb6b3 100755 --- a/test/catalog_plan_index.sh +++ b/test/catalog_plan_index.sh @@ -22,6 +22,32 @@ pgc_setup "${1:-/usr/local/pg17/bin/pg_config}" q "CREATE EXTENSION IF NOT EXISTS pgcolumnar;" >/dev/null +# margin MARGIN FLOOR -- MARGIN when it falls short of FLOOR, else FLOOR. +# +# AN ARM'S FAILURE MUST SAY WHAT IT MEASURED (#1164, selftest part 540). A bare +# `$([ A -lt B ] && echo 1 || echo 0)` reports `got [0] want [1]`, which is the +# word FAILED spelled twice: a reader cannot tell whether the two readings were +# one buffer apart the wrong way or a thousand. Comparing the MARGIN against its +# floor reports the margin itself when it falls short. +# +# The same helper appears in catalog_delete_index.sh. It belongs in lib.sh the +# moment a third suite wants it; two copies is not yet a population. +margin() { [ "$1" -lt "$2" ] && echo "$1" || echo "$2"; } + +# DEFINED HERE, ABOVE THE FIRST RECORDER CALL, AND THE PLACEMENT IS LOAD-BEARING. +# Selftest part 540 folds continuations, remembers the NAME from the last +# recorder call it saw, and attributes any later lossy verdict to it. A helper +# defined mid-file therefore gets blamed on whichever arm happens to precede it: +# with this function further down, the part reported +# +# a new arm that cannot say what it measured is refused by name: +# got [catalog_plan_index: planning a join probed pgcolumnar.storage ...] +# +# naming an arm that is byte-identical to main's and was never touched here. +# Above every recorder call there is no name to inherit. catalog_delete_index.sh +# puts its copy in the same place for the same reason. + + # Other columnar tables sit in the same catalogs. A sequential scan of # options or projection walks their rows too; an index probe does not. q "CREATE TABLE noise_a (id int) USING pgcolumnar; @@ -41,37 +67,27 @@ q "SELECT pg_stat_force_next_flush();" >/dev/null check_num "premise: the filtered scan returned every row" \ "$(q "SELECT count(*) FROM plan_cat WHERE id > 0;")" "800" -opt="$(q "SELECT coalesce(idx_scan,0)::text || ' ' || coalesce(seq_scan,0)::text - FROM pg_stat_all_tables - WHERE schemaname = 'pgcolumnar' AND relname = 'options';")" -opt_idx="${opt%% *}" -opt_seq="${opt##* }" -echo "-- options idx_scan=$opt_idx seq_scan=$opt_seq" -if [ "$opt_idx" -ge 1 ]; then - opt_idx_ok=1 -else - opt_idx_ok=$opt_idx -fi -check_num "planning probed pgcolumnar.options through options_pkey" \ - "$opt_idx_ok" "1" -check_num "planning did not sequentially scan pgcolumnar.options" \ - "$opt_seq" "0" - -prj="$(q "SELECT coalesce(idx_scan,0)::text || ' ' || coalesce(seq_scan,0)::text - FROM pg_stat_all_tables - WHERE schemaname = 'pgcolumnar' AND relname = 'projection';")" -prj_idx="${prj%% *}" -prj_seq="${prj##* }" -echo "-- projection idx_scan=$prj_idx seq_scan=$prj_seq" -if [ "$prj_idx" -ge 1 ]; then - prj_idx_ok=1 -else - prj_idx_ok=$prj_idx -fi -check_num "planning probed pgcolumnar.projection through projection_pkey" \ - "$prj_idx_ok" "1" -check_num "planning did not sequentially scan pgcolumnar.projection" \ - "$prj_seq" "0" +# THE FOUR ARMS THAT STOOD HERE ASSERTED THE ACCESS PATH, AND THIS CHANGE FAILS +# THEM (#1217). They read +# +# planning probed pgcolumnar.options through options_pkey idx_scan >= 1 +# planning did not sequentially scan pgcolumnar.options seq_scan == 0 +# ... and the same two for projection +# +# On this fixture both catalogs are EMPTY, so the size check declines both probes +# and those readings become idx_scan=0 seq_scan=2 and idx_scan=0 seq_scan=4. They +# fail against a build that made planning strictly cheaper -- 300 buffers over 50 +# plans down to nothing -- which is a guard firing on correct code, and a guard +# that fires on correct code gets switched off. +# +# The same defect #1213 removed from catalog_delete_index.sh, one level over. It +# was PREDICTED before this change was written rather than found by running it: +# the prediction named these four arms, the direction each would move, and the +# storage arms below as the ones that must NOT move. All three held. +# +# What replaces them is the work at the default against the work of each extreme +# setting, measured where each claim is measurable: above, on the empty catalogs +# this change is about, and below, on a populated one. # ---- and pgcolumnar.storage, through storage_pkey (#1237) ------------------- # @@ -135,4 +151,150 @@ check_num "premise: the join reached storage more than once" \ check_num "planning a join probed pgcolumnar.storage through storage_pkey" \ "$(if [ "$sj_idx" -ge 2 ]; then echo 1; else echo 0; fi)" "1" +# --------------------------------------------------------------------------- +# THE DEFAULT CONFIGURATION, where the probe cannot win at any size (#1217) +# +# `pgcolumnar.options` gets a row only when set_options is called, and +# `pgcolumnar.projection` only when a projection is added. An installation doing +# neither has BOTH EMPTY -- and that is the default. The fixture above is such an +# installation: it creates three columnar tables and calls neither. +# +# At zero rows the heap being scanned is zero pages, so the scan the probe +# replaces is LITERALLY FREE and the probe cannot win however large the database +# grows. Measured on main 6c3a9510, 50 plans, at 10, 200 and 1000 columnar +# tables alike: +# +# options heap=0 idx=100 projection heap=0 idx=200 +# +# Six index buffers per plan, no heap work at all, and flat in the table count +# because there is nothing to scan more of. There is no crossover to be above. +# +# THESE ARMS ASSERT THE WORK, not the access path, for the reason the four arms +# above do not: a claim about WHICH path was taken cannot tell a revert from an +# improvement. See #1213, where arms of that shape failed 13 of 21 against a +# build that was cheaper at every size. +# --------------------------------------------------------------------------- + +PLAN_CATS="'options','projection'" + +# plan_work N [INDEX_MIN_BLOCKS] -- buffers options and projection serve while +# the same query is planned N times, heap and index both. +# +# N plans rather than one, because the effect is per-plan and six buffers is too +# small a base to divide into. Fifty makes it three hundred against nothing. +plan_work() { + local n="$1" set_clause="" i body="" + [ $# -ge 2 ] && set_clause="SET pgcolumnar.index_min_blocks = $2; " + for ((i = 0; i < n; i++)); do + body="${body}EXPLAIN (COSTS OFF) SELECT count(*) FROM plan_cat WHERE id > 0;" + done + q "SELECT pg_stat_reset();" >/dev/null + q "${set_clause}${body}" >/dev/null + q "SELECT pg_stat_force_next_flush();" >/dev/null + q "SELECT coalesce(sum(heap_blks_read + heap_blks_hit + + coalesce(idx_blks_read,0) + coalesce(idx_blks_hit,0)), 0) + FROM pg_statio_all_tables + WHERE schemaname = 'pgcolumnar' AND relname IN ($PLAN_CATS);" +} + +# THE PREMISE THE WHOLE SECTION RESTS ON. If either catalog had rows, the heap +# would not be free and the arms below would be about a different claim. +check_num "premise: this fixture is the default configuration, both catalogs empty" "$(q "SELECT (SELECT count(*) FROM pgcolumnar.options) + + (SELECT count(*) FROM pgcolumnar.projection) + + (pg_relation_size('pgcolumnar.options') / 8192) + + (pg_relation_size('pgcolumnar.projection') / 8192);")" "0" + +plan_default="$(plan_work 50)" +plan_probe="$(plan_work 50 0)" +echo "-- 50 plans: default=$plan_default probe-always=$plan_probe" + +# AN ARM EXPECTING ZERO IS OWED A PREMISE THAT ANYTHING WAS MEASURED. A run that +# planned nothing reports 0 exactly as loudly as one that planned fifty times +# for free, so the forced-probe reading is what says the instrument was working. +check_num "premise: forcing the probe costs something, so the instrument measured" "$(margin "$plan_probe" 50)" "50" + +check_num "planning costs less than probing both catalogs would" "$(margin "$((plan_probe - plan_default))" 1)" "1" + +# AND THE STRONG FORM, which is what "returns to its exact pre-#1198 number" +# means: a zero-page heap costs nothing to read, so planning should touch these +# two catalogs not at all. +check_num "and touches the empty catalogs not at all" "$plan_default" "0" + +# --------------------------------------------------------------------------- +# AND THE PROBE IS STILL TAKEN WHERE IT PAYS (#1217) +# +# Declining on an empty catalog is only half the claim: a size check that +# declined everything would pass every arm above. So populate one catalog past +# the threshold and require the default to beat reading it whole. +# +# THREE HUNDRED PROJECTIONS ON A NOISE TABLE, AND ONE ON THE MEASURED TABLE. +# `pgcolumnar.projection` takes a row per projection, so one table carries the +# catalog to seven pages in a loop; `pgcolumnar.options` takes one row per +# columnar TABLE and would need about four hundred of them to pass three pages. +# That asymmetry is why this phase drives `projection`. +# +# THE BULK GOES ON THE NOISE TABLE, AND THE FIRST DRAFT PUT IT ON THE MEASURED +# ONE. That fixture read default=1501 against read-whole=1050 -- the probe +# LOSING at seven pages -- and the fixture was what was wrong. A probe's cost +# scales with the number of rows MATCHING ITS KEY, not with the size of the +# catalog; three hundred projections on the measured table means every row +# matches and the probe must return all of them. Scanning seven pages then wins, +# and would have been recorded as "the threshold is wrong for the planner path". +# +# The shape this change is about is the opposite one: a catalog made large by +# OTHER tables' rows, where the probe returns one row and the scan walks +# everything. That is what the noise table builds. +# +# NAMING THE GAP RATHER THAN IMPLYING COVERAGE. The five `options_pkey` sites are +# covered by the arms above, which show the check DECLINING, and not by an arm +# showing it take the probe. They run the same helper as the two +# `projection_pkey` sites, which are covered both ways. +# --------------------------------------------------------------------------- + +q "DO \$do\$ BEGIN FOR i IN 1..300 LOOP + PERFORM pgcolumnar.add_projection('noise_a', 'pp' || i, ARRAY['id'], ARRAY['id']); + END LOOP; END \$do\$; + SELECT pgcolumnar.add_projection('plan_cat', 'own', ARRAY['id'], ARRAY['id']);" >/dev/null + +prj_pages="$(q "SELECT pg_relation_size('pgcolumnar.projection') / 8192;")" +min_blocks="$(q "SHOW pgcolumnar.index_min_blocks;")" +echo "-- projection now $(q "SELECT count(*) FROM pgcolumnar.projection;") rows, ${prj_pages} pages; threshold ${min_blocks}" + +# THE PREMISE THIS PHASE CANNOT DO WITHOUT, and it is derived from the setting +# rather than typed. Below the threshold the default declines the probe and +# reads the heap -- which is what the other reading does too, so both come back +# equal and the arm reports no difference. That reads as "the check is gone" and +# means "the fixture is too small". +check_num "premise: projection is larger than the threshold, so the two paths differ" \ + "$(margin "$((prj_pages - min_blocks))" 1)" "1" + +# AND THE MEASURED TABLE'S OWN SHARE OF IT MUST BE SMALL, or the probe returns +# most of the catalog and the comparison is about something else. This is the +# premise the first draft of this phase did not have, and it is the one that +# would have caught its fixture. +own_rows="$(q "SELECT count(*) FROM pgcolumnar.projection p + JOIN pgcolumnar.storage s USING (storage_id) + WHERE s.relation_oid = 'plan_cat'::regclass::oid;")" +all_rows="$(q "SELECT count(*) FROM pgcolumnar.projection;")" +echo "-- the measured table owns $own_rows of $all_rows projection rows" + +# A SHARE, NOT A COUNT. The first version of this asserted exactly 1 and read 2, +# because add_projection writes more than one row per projection -- which is a +# fact about the function, not about the claim. What the arm needs is that the +# probe returns a SMALL PART of the catalog; a tenth is far looser than the +# fixture and still refuses the shape that broke the first draft, where the +# measured table owned all of it. +check_num "premise: the measured table owns a small share of that catalog" \ + "$(margin "$((all_rows - own_rows * 10))" 1)" "1" + +pop_default="$(plan_work 50)" +pop_whole="$(plan_work 50 2147483647)" +echo "-- 50 plans, projection populated: default=$pop_default read-whole=$pop_whole" + +check_num "premise: reading the populated catalog whole costs something" \ + "$(margin "$pop_whole" 50)" "50" + +check_num "with the catalog populated, planning costs less than reading it whole" \ + "$(margin "$((pop_whole - pop_default))" 1)" "1" + pgc_summary diff --git a/test/check_ledger.tsv b/test/check_ledger.tsv index 1b984d9b..7bc1122b 100644 --- a/test/check_ledger.tsv +++ b/test/check_ledger.tsv @@ -51,15 +51,19 @@ catalog_delete_index catalog_delete_index premise: two vacuums of the same table catalog_delete_index catalog_delete_index the default's cost grows far less with the database than reading whole does 15;16;17;18;19 2026-09-24 pgcolumnar_index_min_blocks default 3 -> 2147483647, so no site probes;pgcolumnar_scan_index_oid stops asking the relation its size, so every site probes;the eight converted scan sites back to InvalidOid catalog_delete_index catalog_delete_index the vacuum's default does less row_group work than reading it whole 15;16;17;18;19 2026-09-24 pgcolumnar_index_min_blocks default 3 -> 2147483647, so no site probes;pgcolumnar_scan_index_oid stops asking the relation its size, so every site probes;the eight converted scan sites back to InvalidOid catalog_plan_index catalog_plan_index a no-qual count did not sequentially scan pgcolumnar.storage 15;16;17;18;19 2026-09-23 both storage_id scans reverted to InvalidOid, so the catalog is swept sequentially on its own primary key +catalog_plan_index catalog_plan_index and touches the empty catalogs not at all 15;16;17;18;19 2026-09-24 only the five options_pkey sites reverted, the two projection ones kept;pgcolumnar_scan_index_oid stops asking the relation its size;the seven planner sites back to the unconditional pgcolumnar_index_oid catalog_plan_index catalog_plan_index planning a join probed pgcolumnar.storage through storage_pkey 15;16;17;18;19 2026-09-23 both storage_id scans reverted to InvalidOid, so the catalog is swept sequentially on its own primary key -catalog_plan_index catalog_plan_index planning did not sequentially scan pgcolumnar.options 15;16;17;18;19 2026-09-22 InvalidOid on the planner options and projection scans -catalog_plan_index catalog_plan_index planning did not sequentially scan pgcolumnar.projection 15;16;17;18;19 2026-09-22 InvalidOid on the planner options and projection scans -catalog_plan_index catalog_plan_index planning probed pgcolumnar.options through options_pkey 15;16;17;18;19 2026-09-22 InvalidOid on the planner options and projection scans -catalog_plan_index catalog_plan_index planning probed pgcolumnar.projection through projection_pkey 15;16;17;18;19 2026-09-22 InvalidOid on the planner options and projection scans +catalog_plan_index catalog_plan_index planning costs less than probing both catalogs would 15;16;17;18;19 2026-09-24 pgcolumnar_scan_index_oid stops asking the relation its size;the seven planner sites back to the unconditional pgcolumnar_index_oid catalog_plan_index catalog_plan_index premise: a no-qual count over a columnar table touched storage at all 15;16;17;18;19 never - +catalog_plan_index catalog_plan_index premise: forcing the probe costs something, so the instrument measured 15;16;17;18;19 never - +catalog_plan_index catalog_plan_index premise: projection is larger than the threshold, so the two paths differ 15;16;17;18;19 never - +catalog_plan_index catalog_plan_index premise: reading the populated catalog whole costs something 15;16;17;18;19 never - catalog_plan_index catalog_plan_index premise: the filtered scan returned every row 15;16;17;18;19 never - catalog_plan_index catalog_plan_index premise: the join reached storage more than once 15;16;17;18;19 never - catalog_plan_index catalog_plan_index premise: the measured table holds its rows 15;16;17;18;19 never - +catalog_plan_index catalog_plan_index premise: the measured table owns a small share of that catalog 15;16;17;18;19 never - +catalog_plan_index catalog_plan_index premise: this fixture is the default configuration, both catalogs empty 15;16;17;18;19 never - +catalog_plan_index catalog_plan_index with the catalog populated, planning costs less than reading it whole 15;16;17;18;19 2026-09-24 pgcolumnar_scan_index_oid stops asking the relation its size;the seven planner sites back to the unconditional pgcolumnar_index_oid differential differential agg avg 15;16;17;18;19 never - differential differential agg count 15;16;17;18;19 never - differential differential agg minmax 15;16;17;18;19 never - @@ -1427,11 +1431,11 @@ harness_selftest 570-a-checkout-test-must-ask-git premise: the sweep sees .git m harness_selftest 570-a-checkout-test-must-ask-git the sweep matches every spelling that stats .git as a directory 15;16;17;18;19 2026-09-23 the same planted line, read by the pattern shipped in #1224;the sweep pattern shipped in #1224 is restored verbatim harness_selftest 570-a-checkout-test-must-ask-git without the trailing boundary the same pattern flags three of them 15;16;17;18;19 2026-09-23 the boundary in the sweep pattern drifts, so the strip becomes a no-op;the derived control stops stripping the boundary harness_selftest 580-a-hand-rebuild-must-record a log with content produces a diagnosis to print 15;16;17;18;19 2026-09-24 rebuild.sh forced to fail after its stamp AND _hr_diagnose gutted, so the failure prints no reason -harness_selftest 580-a-hand-rebuild-must-record a rebuild that failed skips them rather than reading a stamp it did not write 15;16;17;18;19 2026-09-24 the gate reverted so a failed rebuild still runs the arms beneath it; driven alone, with no forced rebuild failure, because these two arms call the gate with literals and the rebuild's status never reaches them +harness_selftest 580-a-hand-rebuild-must-record a rebuild that failed skips them rather than reading a stamp it did not write 15;16;17;18;19 2026-09-24 driven alone, with no forced rebuild failure, because these two arms call the gate with literals and the rebuild's status never reaches them;the gate reverted so a failed rebuild still runs the arms beneath it harness_selftest 580-a-hand-rebuild-must-record a rebuild that succeeded runs the arms that read its stamp 15;16;17;18;19 2026-09-24 the gate always skips, so a successful rebuild runs none of the arms that read its stamp harness_selftest 580-a-hand-rebuild-must-record an empty log produces nothing rather than an empty banner 15;16;17;18;19 2026-09-24 _hr_diagnose banners unconditionally, so an empty or absent log still prints harness_selftest 580-a-hand-rebuild-must-record and a log that is not there produces nothing rather than an error 15;16;17;18;19 2026-09-24 _hr_diagnose banners unconditionally, so an empty or absent log still prints -harness_selftest 580-a-hand-rebuild-must-record and any other status skips too, rather than being read as success 15;16;17;18;19 2026-09-24 the gate reverted so a failed rebuild still runs the arms beneath it; driven alone, with no forced rebuild failure, because these two arms call the gate with literals and the rebuild's status never reaches them +harness_selftest 580-a-hand-rebuild-must-record and any other status skips too, rather than being read as success 15;16;17;18;19 2026-09-24 driven alone, with no forced rebuild failure, because these two arms call the gate with literals and the rebuild's status never reaches them;the gate reverted so a failed rebuild still runs the arms beneath it harness_selftest 580-a-hand-rebuild-must-record and it reads a different source as stale rather than fresh 15;16;17;18;19 2026-09-23 the record call removed from rebuild.sh, so no stamp is written at all harness_selftest 580-a-hand-rebuild-must-record and it writes a reason, so there is something for the gate to print 15;16;17;18;19 2026-09-24 rebuild.sh exits without saying why, so the gate has nothing to print harness_selftest 580-a-hand-rebuild-must-record and the diagnosis carries the failing line, not just a status 15;16;17;18;19 2026-09-24 rebuild.sh forced to fail after its stamp AND _hr_diagnose gutted, so the failure prints no reason diff --git a/test/check_ledger_budget.txt b/test/check_ledger_budget.txt index 6fc2eb6c..d3d280c0 100644 --- a/test/check_ledger_budget.txt +++ b/test/check_ledger_budget.txt @@ -897,4 +897,50 @@ suites_not_covered 249 # not there holds none" (G, redundant) and "a preflight that built nothing says # how many it built", which predates this change -- K reddens its two siblings # but leaves the `built 0 of 3` line intact. -checks_never_observed_red 1534 +# +# 1534 -> 1539 for #1217, RE-ANCHORED after #1219 landed under this branch. +# It was written against 1532 and rebased onto a main that now declares 1534; +# the arithmetic below is redone against what main actually holds, not adjusted +# by the difference. Counted: +# +# awk -F'\t' '$5=="never"' test/check_ledger.tsv | wc -l -> 1539 +# 1694 rows total, 155 not `never`, 1539 + 155 == 1694 +# +# 4 rows REMOVED, all of them dated never unchanged, dated -4 +# 8 rows ADDED: 5 arrive `never`, 3 dated never +5, dated +3 +# 1534 + 5 == 1539 1690 - 4 + 8 == 1694 +# +# THE ROWS AUTO-MERGED AND ONLY THIS LINE CONFLICTED, which is the shape to be +# careful about rather than reassured by: a conflict on the total is a prompt to +# RECOUNT, not a choice between two numbers. Measured on a different pair the +# same day, composing two branches that touched DISJOINT parts: the rows +# conflicted too, and taking one side gave the right count while taking the +# other silently dropped three rows and reported a number nothing reads as +# wrong. Union the rows; recount the total. +# +# FOUR ROWS CARRYING HISTORY WERE DELETED, AND THE TOOL REFUSED TO DO IT FOR ME. +# `orphan-scan --prune` said: "the catalogue is what this ledger is for -- no run +# can recreate it. Reconcile them instead: rename the ledger row to the check's +# new name, or say in the commit why the history may go." This is the second +# route. +# +# planning probed pgcolumnar.options through options_pkey +# planning did not sequentially scan pgcolumnar.options +# planning probed pgcolumnar.projection through projection_pkey +# planning did not sequentially scan pgcolumnar.projection +# +# THEY ARE NOT RENAMES AND MUST NOT BE RECORDED AS ONE. Each asserted WHICH +# ACCESS PATH was taken; their replacements assert HOW MUCH WORK was done. Moving +# a path claim's history onto a work claim's row would say those arms had been +# observed red under a mutation they never ran, which is the one thing a ledger +# exists to prevent. The history is real and it is history of a check that should +# not exist: all four fail against a build that made planning strictly cheaper -- +# 300 buffers over 50 plans down to nothing -- which is a guard firing on correct +# code. +# +# What replaces them was observed red under three mutations, one of which is +# PARTIAL: reverting only the five options_pkey sites and keeping the two +# projection ones reddens exactly one arm, at exactly the residual cost. +# +# suites_not_covered does NOT move: catalog_plan_index is already covered. +checks_never_observed_red 1539 diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 03d0cc8f..3043d7ef 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -125,7 +125,7 @@ behaviour, the source of that number is named. - [77. test_projection_parallel.py: a covering projection can be a parallel scan](#77-test_projection_parallelpy-a-covering-projection-can-be-a-parallel-scan) - [78. test_ttl_expire.py: the one function that deletes rows, tested twice](#78-test_ttl_expirepy-the-one-function-that-deletes-rows-tested-twice) - [79. test_projection_scan_io.py: a covering projection is not priced from the base table's pages](#79-test_projection_scan_iopy-a-covering-projection-is-not-priced-from-the-base-tables-pages) -- [80. test_catalog_plan_index.py: planning uses the options and projection indexes](#80-test_catalog_plan_indexpy-planning-uses-the-options-and-projection-indexes) +- [80. test_catalog_plan_index.py: planning pays for these catalogs only when they hold something](#80-test_catalog_plan_indexpy-planning-pays-for-these-catalogs-only-when-they-hold-something) - [81. test_catalog_delete_index.py: retiring a row group costs no more for a bigger database](#81-test_catalog_delete_indexpy-retiring-a-row-group-costs-no-more-for-a-bigger-database) ## 1. How to read a test in here @@ -6170,11 +6170,19 @@ fixtures are `psio_e` and `pciot_e`. | `test_projection_scan_io` | the table and covering projection exist; the plan uses that projection; the covering scan has a positive run cost; the projection occupies a minority of the relation; the covering run is not priced from the base table's pages; a covering path whose storage cannot be found is not priced as one page | | `test_an_empty_covering_projection_is_priced_as_one_page` | the projection is found and its storage holds no row groups; an empty covering projection is still priced as one page, so it is still chosen | -## 80. test_catalog_plan_index.py: planning uses the options and projection indexes +## 80. test_catalog_plan_index.py: planning pays for these catalogs only when they hold something -Port of `catalog_plan_index.sh`. A plan that asks what one columnar table was written with, and whether it has a projection, sequentially scanned `pgcolumnar.options` and `pgcolumnar.projection`. Both catalogs already have a primary key on the column the scan key names. +Port of `catalog_plan_index.sh`. A plan that asks what one columnar table was written with, and whether it has a projection, sequentially scanned `pgcolumnar.options` and `pgcolumnar.projection`. Both catalogs already have a primary key on the column the scan key names, and #1198 made planning use it. -The measured statement runs on a second connection. The shell suite gets that by using a fresh `psql` for every statement. This file holds one connection for the writes and opens another for the scan, for the same reason `test_native_delete_vector_index.py` does: the session that just wrote is a different path. +THAT WAS A REGRESSION ON THE DEFAULT CONFIGURATION, AND THIS FILE NOW SAYS SO. A row reaches `options` only when `set_options` is called and `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, the scan is literally free, and the probe cannot win however large the database grows -- there is no crossover to be above. Measured on main before the fix: six index buffers per plan, flat at 10, 200 and 1000 columnar tables alike. #1217 routes those seven sites through the size check shipped in #1213, and the cost returns to nothing. + +FOUR ARMS WERE REMOVED RATHER THAN REPAIRED. They asserted `idx_scan >= 1` and `seq_scan == 0` on each catalog -- which path was taken, not how much work was done -- and they fail against the build that made planning strictly cheaper. That is a guard firing on correct code, the same defect #1213 removed from `catalog_delete_index.sh`. It was predicted before the change was written: the prediction named those four arms, the direction each would move, and the storage arms as the ones that must not move, and all three held. + +ON A PRIVATE DATABASE, for the two tests whose claims are about catalog size. `pgc_conn` gives a private schema and these catalogs are per database, so inside the corpus other files have already populated them; `pgc_own_db` in `conftest.py` gives a database of its own. That fixture exists because the same trap bit `test_catalog_delete_index.py` first -- see section 81. + +THE BULK OF THE PROJECTIONS GOES ON A TABLE THAT IS NOT THE MEASURED ONE. A probe's cost scales with the rows matching its key, not with the size of the catalog. The shell twin's first draft put 300 projections on the measured table, so every row matched, and it read the probe LOSING at seven pages -- which would have been recorded as "the threshold is wrong for the planner path". A premise now asserts the measured table owns a small share. + +The two halves share no fixture: the shell suite drives `projection` to 7 pages with 300 projections on a noise table and measures 50 plans; this one uses 260 and a different measured table, and reads the work per catalog rather than summing in SQL. `pg_stat_reset()` is database-wide. The corpus runs serially within a worker. @@ -6182,7 +6190,9 @@ The measured statement runs on a second connection. The shell suite gets that by | test | what it holds | | --- | --- | -| `test_catalog_plan_index` | the measured table's row count, that the filtered scan returned every row, and that `options` and `projection` were probed by index with `seq_scan` still 0 | +| `test_catalog_plan_index` | the measured table's row count, that the filtered scan returned every row, and that planning a `count(*)` and a two-relation join reach `pgcolumnar.storage` through `storage_pkey` | +| `test_planning_costs_nothing_on_the_default_configuration` | that this fixture really is the default configuration with both catalogs empty, that forcing the probe costs something so the instrument measured, that planning costs less than probing both, and that it touches the empty catalogs **not at all** | +| `test_planning_still_probes_a_populated_catalog` | that `projection` is larger than the threshold so the two paths differ, that the measured table owns a small share of it, that reading it whole costs something, and that planning costs less than reading it whole | ## 81. test_catalog_delete_index.py: retiring a row group costs no more for a bigger database diff --git a/test/pytest/conftest.py b/test/pytest/conftest.py index 859c88a3..2f352156 100644 --- a/test/pytest/conftest.py +++ b/test/pytest/conftest.py @@ -188,6 +188,54 @@ def _serve_cluster(cluster, root, verdict, worker_id): shutil.rmtree(root, ignore_errors=True) +@pytest.fixture +def pgc_own_db(pgc_cluster, request): + """A private DATABASE, where `pgc_conn` gives a private schema. + + `pgc_conn` is the right trade almost everywhere: isolation without paying an + initdb per test. IT IS THE WRONG ONE FOR ANY TEST WHOSE CLAIMS ARE ABOUT THE + SIZE OR EMPTINESS OF THE pgcolumnar CATALOGS, because those live in the + database and are shared by every test in the session. + + Measured, and it is why this exists (#1213): a file asserting a claim about + catalog size saw six catalog pages run alone and thirty-nine run after the + other fifty-one cluster files, and the claim fell from 223 parts per + thousand to 65 -- under its floor. The arm was not wrong and the code was not + wrong; the fixture's assumption was, and it held only in the arrangement + that had been run. CI found it and a single-file run could not. + + The extension is created on the raw connection before the wrapper goes on, + the way pgc_conn creates its schema: that is this fixture's own DDL, not the + test's writes, and DDL carries no row count anyway. + + A test using this must still not run concurrently with another that reads + database-wide statistics, because pg_stat_reset() is per database and this + one is private only to the test, not to the cluster. + """ + import psycopg # deferred: see the module docstring + + name = "pgc_own_" + "".join( + ch if ch.isalnum() else "_" for ch in request.node.name + )[:40] + + def admin(sql): + c = psycopg.connect(pgc_cluster.dsn(), autocommit=True) + try: + c.execute(sql) + finally: + c.close() + + admin(f'DROP DATABASE IF EXISTS "{name}"') + admin(f'CREATE DATABASE "{name}"') + conn = psycopg.connect(pgc_cluster.dsn(dbname=name), autocommit=True) + try: + conn.execute("CREATE EXTENSION pgcolumnar") + yield pgc_vacuity.watch_writes(conn, request.node.nodeid) + finally: + conn.close() + admin(f'DROP DATABASE IF EXISTS "{name}"') + + @pytest.fixture def pgc_conn(pgc_cluster, request): """A direct connection, in a schema private to this one test. diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index 632f703e..90fbf5dd 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -509,21 +509,14 @@ guard_tests 403 # main alone guard=403 cluster=476 (50 cluster files) # composed guard=403 cluster=477 (51 cluster files) # -# cluster_tests moved for #1207's new cluster file, test_catalog_delete_index.py, -# which holds two tests. RE-DERIVED BY COLLECTION against this branch's actual -# base, never by adding two to either side: this branch has carried 478, 479 and -# 480 against three different mains, and each described a tree that no longer -# exists. -# -# main 1bcfb92b guard=403 cluster=478 (51 cluster files) -# this tree guard=403 cluster=480 (52 cluster files) -# -# THE FIRST ATTEMPT AT THE MAIN FIGURE READ 480, AND IT WAS THE MEASUREMENT THAT -# WAS WRONG. It collected in a copy of this working tree with -# `git checkout origin/main -- .` applied, which restores the files main has and -# does not remove the ones it does not: the new test file was still sitting -# there, so "main" collected 52 files. `git checkout -f` plus `git clean -fd` -# gives 51 and 478, which is what main's own expected_tests.txt declares -- and -# agreeing with the committed number is how the corrected measurement was -# recognised as the right one. -cluster_tests 480 +# cluster_tests 480 -> 482 for #1217's two new tests in test_catalog_plan_index.py. +# RE-DERIVED BY COLLECTION on this branch, not by adding two: +# +# main 6c3a9510 guard=403 cluster=480 (52 cluster files) +# this tree guard=403 cluster=482 (52 cluster files) +# +# The file count does NOT move -- the tests join a file the corpus already had, +# which is the case the earlier note about 478/479/480 did not cover. Every one +# of those was re-derived against a different main; this one is re-derived +# against 6c3a9510 and against nothing else. +cluster_tests 482 diff --git a/test/pytest/test_catalog_delete_index.py b/test/pytest/test_catalog_delete_index.py index 38179eb5..4f6ab16b 100644 --- a/test/pytest/test_catalog_delete_index.py +++ b/test/pytest/test_catalog_delete_index.py @@ -97,9 +97,6 @@ # cannot measure, this file does not assert. FLOOR_PERMILLE = 100 -import pytest - -import pgc_vacuity CATALOGS = ( "bloom", @@ -111,49 +108,9 @@ ) -@pytest.fixture -def pgc_own_db(pgc_cluster, request): - """A private DATABASE, not merely a private schema. - - `pgc_conn` gives every test its own schema, which is the right trade almost - everywhere here: isolation without paying an initdb per test. IT IS THE - WRONG ONE FOR THIS FILE. The `pgcolumnar` metadata catalogs are per - DATABASE and shared by every test in the session, and every claim below is - about how big those catalogs are. - - MEASURED, AND IT IS WHY CI FOUND THIS AND A LOCAL RUN COULD NOT. Run alone, - this file saw six catalog pages at phase 0 and P1 was worth 223 parts per - thousand. Run after the other fifty-one cluster files, it saw thirty-nine - pages and P1 was worth 65 -- under the floor. The arm was not wrong and the - code was not wrong; the fixture's assumption was, and it held only in the - one arrangement I had run. - - The extension is created on the raw connection before the wrapper goes on, - the way conftest creates its schema: that is this fixture's own DDL, not the - test's writes, and DDL carries no row count anyway. - """ - import psycopg # deferred: see the module docstring - - name = "pgc_own_" + "".join( - ch if ch.isalnum() else "_" for ch in request.node.name - )[:40] - - def admin(sql): - c = psycopg.connect(pgc_cluster.dsn(), autocommit=True) - try: - c.execute(sql) - finally: - c.close() - - admin(f'DROP DATABASE IF EXISTS "{name}"') - admin(f'CREATE DATABASE "{name}"') - conn = psycopg.connect(pgc_cluster.dsn(dbname=name), autocommit=True) - try: - conn.execute("CREATE EXTENSION pgcolumnar") - yield pgc_vacuity.watch_writes(conn, request.node.nodeid) - finally: - conn.close() - admin(f'DROP DATABASE IF EXISTS "{name}"') +# `pgc_own_db` moved to conftest.py when a second file needed it (#1217). A +# guarantee kept in one suite has to be re-fitted to every other that needs it, +# and this one is about a trap that is invisible from inside the file it bites. def _columnar_relations(conn): diff --git a/test/pytest/test_catalog_plan_index.py b/test/pytest/test_catalog_plan_index.py index 68bb6714..97804509 100644 --- a/test/pytest/test_catalog_plan_index.py +++ b/test/pytest/test_catalog_plan_index.py @@ -75,32 +75,13 @@ def test_catalog_plan_index(pgc_cluster, pgc_conn, expect): "premise: the filtered scan returned every row", ) - opt_idx, opt_seq = _stats(pgc_conn, "options") - prj_idx, prj_seq = _stats(pgc_conn, "projection") - print( - f"-- options idx_scan={opt_idx} seq_scan={opt_seq} " - f"projection idx_scan={prj_idx} seq_scan={prj_seq}" - ) - expect.at_least( - opt_idx, - 1, - "planning probed pgcolumnar.options through options_pkey", - ) - expect.num( - opt_seq, - 0, - "planning did not sequentially scan pgcolumnar.options", - ) - expect.at_least( - prj_idx, - 1, - "planning probed pgcolumnar.projection through projection_pkey", - ) - expect.num( - prj_seq, - 0, - "planning did not sequentially scan pgcolumnar.projection", - ) + # THE FOUR ARMS THAT STOOD HERE ASSERTED THE ACCESS PATH (#1217): options + # and projection each had to show idx_scan >= 1 and seq_scan == 0. This + # fixture calls neither set_options nor add_projection, so both catalogs are + # EMPTY, the size check declines both probes, and those arms fail against a + # build that made planning strictly cheaper. A guard that fires on correct + # code gets switched off. Replaced below by the work, in its own test, on a + # database private to it. # ---- and pgcolumnar.storage, through storage_pkey (#1237) -------------- # @@ -176,3 +157,147 @@ def test_catalog_plan_index(pgc_cluster, pgc_conn, expect): 2, "planning a join probed pgcolumnar.storage through storage_pkey", ) + + +PLAN_CATS = ("options", "projection") + + +def _plan_work(conn, n, min_blocks=None): + """Buffers `options` and `projection` serve while one query is planned `n` times. + + `n` plans rather than one, because the effect is per-plan: six buffers is too + small a base to divide into, and fifty makes it three hundred against + nothing. + """ + with conn.cursor() as cur: + cur.execute("SELECT pg_stat_force_next_flush()") + cur.execute("SELECT pg_stat_reset()") + if min_blocks is not None: + cur.execute(f"SET pgcolumnar.index_min_blocks = {min_blocks}") + for _ in range(n): + cur.execute("EXPLAIN (COSTS OFF) SELECT count(*) FROM pc_main WHERE v > 3") + cur.execute("RESET pgcolumnar.index_min_blocks") + cur.execute("SELECT pg_stat_force_next_flush()") + cur.execute( + "SELECT relname, " + " coalesce(heap_blks_read,0) + coalesce(heap_blks_hit,0) " + "+ coalesce(idx_blks_read,0) + coalesce(idx_blks_hit,0) " + "FROM pg_statio_all_tables " + "WHERE schemaname = 'pgcolumnar' AND relname = ANY(%s)", + (list(PLAN_CATS),), + ) + per = {r[0]: int(r[1]) for r in cur.fetchall()} + return sum(per.values()), per + + +def test_planning_costs_nothing_on_the_default_configuration(pgc_own_db, expect): + """`options` and `projection` are empty unless you ask for them, and that is the default. + + A row reaches `pgcolumnar.options` only when `set_options` is called and + `pgcolumnar.projection` only when a projection is added. An installation + doing neither has both empty, the heap the probe replaces is zero pages, and + the scan it replaces is literally free -- so the probe cannot win however + large the database grows. There is no crossover to be above. Measured on + main before the fix: six index buffers per plan, flat at 10, 200 and 1000 + columnar tables alike. + + ON A PRIVATE DATABASE, because `pgc_conn` gives a private schema and these + catalogs are per database. Run inside the corpus with a shared one, other + files have already populated them and the premise below is simply false. + """ + conn = pgc_own_db + with conn.cursor() as cur: + for t in ("pc_side_a", "pc_side_b", "pc_main"): + cur.execute(f"CREATE TABLE {t} (n bigint, v bigint) USING pgcolumnar") + cur.execute(f"INSERT INTO {t} SELECT g, g % 11 FROM generate_series(1,120) g") + cur.execute("SELECT count(*) FROM pc_main") + expect.num(cur.fetchone()[0], 120, "premise: the measured table holds its rows") + cur.execute( + "SELECT (SELECT count(*) FROM pgcolumnar.options)" + " + (SELECT count(*) FROM pgcolumnar.projection)" + " + pg_relation_size('pgcolumnar.options') / 8192" + " + pg_relation_size('pgcolumnar.projection') / 8192" + ) + expect.num( + cur.fetchone()[0], + 0, + "premise: this fixture is the default configuration, both catalogs empty", + ) + + default, per = _plan_work(conn, 50) + probe, _ = _plan_work(conn, 50, 0) + print(f"-- 50 plans: default={default} probe-always={probe} by catalog {per}") + + # AN ARM EXPECTING ZERO IS OWED A PREMISE THAT ANYTHING WAS MEASURED: a run + # that planned nothing reports 0 exactly as loudly as one that planned fifty + # times for free. + expect.at_least( + probe, 50, "premise: forcing the probe costs something, so the instrument measured" + ) + expect.at_least( + probe - default, 1, "planning costs less than probing both catalogs would" + ) + expect.num(default, 0, "and touches the empty catalogs not at all") + + +def test_planning_still_probes_a_populated_catalog(pgc_own_db, expect): + """Declining everything would pass the test above, so require the probe where it pays. + + THE BULK OF THE PROJECTIONS GO ON A TABLE THAT IS NOT THE MEASURED ONE. A + probe's cost scales with the rows MATCHING ITS KEY, not with the size of the + catalog: putting them all on the measured table makes every row match, and + the shell twin's first draft read the probe LOSING at seven pages for + exactly that reason. The shape this change is about is a catalog made large + by OTHER tables' rows. + """ + conn = pgc_own_db + with conn.cursor() as cur: + cur.execute("CREATE TABLE pc_bulk (n bigint, v bigint) USING pgcolumnar") + cur.execute("INSERT INTO pc_bulk SELECT g, g FROM generate_series(1,60) g") + cur.execute("CREATE TABLE pc_main (n bigint, v bigint) USING pgcolumnar") + cur.execute("INSERT INTO pc_main SELECT g, g % 11 FROM generate_series(1,120) g") + for i in range(260): + cur.execute( + "SELECT pgcolumnar.add_projection('pc_bulk', %s, ARRAY['n'], ARRAY['n'])", + (f"bp{i}",), + ) + cur.execute("SELECT pgcolumnar.add_projection('pc_main','own',ARRAY['n'],ARRAY['n'])") + + cur.execute("SHOW pgcolumnar.index_min_blocks") + threshold = int(cur.fetchone()[0]) + cur.execute("SELECT pg_relation_size('pgcolumnar.projection') / 8192") + pages = int(cur.fetchone()[0]) + cur.execute("SELECT count(*) FROM pgcolumnar.projection") + total = cur.fetchone()[0] + cur.execute( + "SELECT count(*) FROM pgcolumnar.projection p " + "JOIN pgcolumnar.storage s USING (storage_id) " + "WHERE s.relation_oid = 'pc_main'::regclass::oid" + ) + own = cur.fetchone()[0] + print(f"-- projection {total} rows, {pages} pages, threshold {threshold}; " + f"the measured table owns {own}") + + # Below the threshold the default declines and reads the heap, which is what + # the other reading does too -- both come back equal and the arm reports no + # difference. That reads as "the check is gone" and means "the fixture is + # too small". Derived from the setting rather than typed. + expect.at_least( + pages - threshold, 1, + "premise: projection is larger than the threshold, so the two paths differ", + ) + # A SHARE, NOT A COUNT: add_projection writes more than one row per + # projection, which is a fact about the function and not about the claim. + expect.at_least( + total - own * 10, 1, + "premise: the measured table owns a small share of that catalog", + ) + + default, _ = _plan_work(conn, 50) + whole, _ = _plan_work(conn, 50, 2147483647) + print(f"-- 50 plans, projection populated: default={default} read-whole={whole}") + expect.at_least(whole, 50, "premise: reading the populated catalog whole costs something") + expect.at_least( + whole - default, 1, + "with the catalog populated, planning costs less than reading it whole", + ) From 944868424724a5a1904ea349effa3178dc328276 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Thu, 24 Sep 2026 13:06:02 -0600 Subject: [PATCH 2/2] test: repair two lossy arms part 540 cannot see, and stop quoting the 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) Claude-Session: https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n --- test/catalog_plan_index.sh | 36 +++++++++++++++++++++++++++++------- 1 file changed, 29 insertions(+), 7 deletions(-) diff --git a/test/catalog_plan_index.sh b/test/catalog_plan_index.sh index 54deb6b3..b42f0784 100755 --- a/test/catalog_plan_index.sh +++ b/test/catalog_plan_index.sh @@ -24,11 +24,21 @@ q "CREATE EXTENSION IF NOT EXISTS pgcolumnar;" >/dev/null # margin MARGIN FLOOR -- MARGIN when it falls short of FLOOR, else FLOOR. # -# AN ARM'S FAILURE MUST SAY WHAT IT MEASURED (#1164, selftest part 540). A bare -# `$([ A -lt B ] && echo 1 || echo 0)` reports `got [0] want [1]`, which is the -# word FAILED spelled twice: a reader cannot tell whether the two readings were -# one buffer apart the wrong way or a thousand. Comparing the MARGIN against its -# floor reports the margin itself when it falls short. +# AN ARM'S FAILURE MUST SAY WHAT IT MEASURED (#1164, selftest part 540). An arm +# that reduces two numbers to a bare one-or-nought before the comparison reports +# `got [0] want [1]`, which is the word FAILED spelled twice: a reader cannot +# tell whether the two readings were one buffer apart the wrong way or a +# thousand. Comparing the MARGIN against its floor reports the margin itself +# when it falls short. +# +# THE ANTI-PATTERN IS DESCRIBED HERE IN WORDS AND NOT QUOTED, DELIBERATELY. +# Part 540's sweep does not strip comments, and its bracket rule carries no +# "the recorder is on this line" conjunct -- the awk rule beside it does. So a +# comment QUOTING the shape is matched as though it were an arm, and then named +# after whichever recorder call precedes it. Reproduced: a file whose only +# offending text is such a comment yields one finding, named after an innocent +# arm; delete the comment and it goes. That is how an untouched, byte-identical +# storage arm in this file came to be reported as a new offender. # # The same helper appears in catalog_delete_index.sh. It belongs in lib.sh the # moment a third suite wants it; two copies is not yet a population. @@ -146,10 +156,22 @@ sj_idx="${sj%% *}" sj_seq="${sj##* }" echo "-- storage after a two-relation join idx_scan=$sj_idx seq_scan=$sj_seq" +# THESE TWO WERE LOSSY AND PART 540 COULD NOT SEE IT (#1255). Both branches were +# constants and `sj_idx` was discarded, so a failure printed `got [0] want [1]` +# and the count went with it -- the #1164 symptom exactly. 540 misses them +# because both of its matchers require the `&&` spelling and these used the +# shell `if/then/else` one, so the corpus tracked them as clean rather than as +# debt. Found by @OffgridwithJD while checking an arm I had called innocent +# because it was byte-identical to main's; it was, and that was not the reason +# it went untracked. +# +# The `count(*)` arm above is left alone on purpose: `-ge 1` is inside 540's own +# determinate() carve-out, because exactly one value fails and `got [0]` does +# say which state was reached. check_num "premise: the join reached storage more than once" \ - "$(if [ "$((sj_idx + sj_seq))" -ge 2 ]; then echo 1; else echo 0; fi)" "1" + "$(margin "$((sj_idx + sj_seq))" 2)" "2" check_num "planning a join probed pgcolumnar.storage through storage_pkey" \ - "$(if [ "$sj_idx" -ge 2 ]; then echo 1; else echo 0; fi)" "1" + "$(margin "$sj_idx" 2)" "2" # --------------------------------------------------------------------------- # THE DEFAULT CONFIGURATION, where the probe cannot win at any size (#1217)