From bc5ad0c7b1b4ce58e85d619657be277d5853b25c Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 23 Sep 2026 13:18:01 -0600 Subject: [PATCH] fix: name storage_pkey on both storage_id scans (#1237) PgColumnarGetSortedInfo and PgColumnarCheckNativeFormatVersion key on storage_id and both passed InvalidOid, so both swept pgcolumnar.storage sequentially on the one column storage_pkey is a UNIQUE btree over. PgColumnarGetSortedInfo planning, via pgcolumnar_sorted_pathkeys PgColumnarCheckNativeFormatVersion execution, once per relation scanned Unlike #1210 nothing has to be built and no decision about #1211 is involved: the index exists and the key is exact. unfixed count(*) idx=0 seq=1 join idx=0 seq=5 8 passed + 2 failed fixed count(*) idx=1 seq=0 join idx=4 seq=1 10 passed + 0 failed The four converted are GetSortedInfo twice at planning and CheckNativeFormatVersion twice at execution. THE RESIDUE IS pgcolumnar_written_stripe_row_limit, ONCE PER COLUMNAR RELATION THAT REACHES IT (#1210, #1211), which keys on relation_oid and has no index to name. One here because this query's qual is on one side of the join only; @OffgridwithJD measures two where both sides reach it. `seq` becoming 1 is a property of the query, not of the change. Which is why the join arm asserts idx_scan >= 2 rather than seq_scan == 0. Asserting zero would fail for a defect this change does not fix, and asserting one would pin a number #1210 is expected to move. The two shapes are separate arms because they reach different code. A no-qual count never reaches the row-group-limit lookup, so its only storage access is the format-version one. FOUND BY TWO INSTRUMENTS DISAGREEING AND BOTH BEING RIGHT. An elog inside pgcolumnar_written_stripe_row_limit counted 2 scans for a join; seq_scan on the catalog counted 4. An elog at one function counts arrivals at that function; a counter on the relation counts scans from any caller. The gap was the second reader. A first hypothesis -- that a projection was present in one fixture and not the other -- was driven with the projection count printed as a premise and died: four calls with zero projections. Both harnesses, ten checks each, identical assertion names. The pytest twin got its own removal proof rather than inheriting the shell one, because the twins' regimes diverge: it aborts at its first failure where the shell suite runs on, reporting 8 checks against 10. Majors from five runs against DEFAULT_CONFIGS: rc=0, 10 passed + 0 failed on pg15, pg16, pg17, pgsql and pg19. Census: 1519 -> 1521 by counting, 1606 rows, 85 not never, 1521 + 85 == 1606. suites_not_covered does not move; catalog_plan_index is already covered. PgColumnarSetSortedExtent has the same shape on the same key and is NOT included: it is a write path holding RowExclusiveLock, nothing has measured its cost or frequency, and the InvalidOid sweep of this file belongs to #1207/#1213. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n --- CHANGELOG.md | 41 ++++++++++++++ src/columnar_metadata.c | 35 +++++++++++- test/catalog_plan_index.sh | 62 +++++++++++++++++++++ test/check_ledger.tsv | 4 ++ test/check_ledger_budget.txt | 50 ++++++++++++++++- test/pytest/test_catalog_plan_index.py | 75 ++++++++++++++++++++++++++ 6 files changed, 264 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b32fdee..8f3c65df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -336,6 +336,47 @@ true until the next version shipped. ### Fixed +- Planning a columnar query sequentially scanned `pgcolumnar.storage` on its own + primary key (#1237). + + Two readers key on `storage_id` and both passed `InvalidOid`, so both swept the + catalog sequentially on the one column `storage_pkey` is a UNIQUE btree over: + + | reader | phase | reached from | + | --- | --- | --- | + | `PgColumnarGetSortedInfo` | planning | `pgcolumnar_sorted_pathkeys` | + | `PgColumnarCheckNativeFormatVersion` | execution, once per relation scanned | `columnar_reader.c:644` | + + Unlike #1210 nothing had to be built and no decision about #1211 was involved: + the index exists and the key is exact. + + ``` + unfixed count(*) idx=0 seq=1 join idx=0 seq=5 + fixed count(*) idx=1 seq=0 join idx=4 seq=1 + ``` + + The four converted scans are `GetSortedInfo` twice at planning and + `CheckNativeFormatVersion` twice at execution. **THE RESIDUE IS + `pgcolumnar_written_stripe_row_limit`, ONCE PER COLUMNAR RELATION THAT REACHES + IT** (#1210, #1211), which keys on `relation_oid` and has no index to name. + One here because this query's qual is on one side of the join only; + @OffgridwithJD measures two where both sides reach it. `seq` becoming 1 is a + property of the query, not of the change. + + WHICH IS WHY THE JOIN ARM ASSERTS `idx_scan >= 2` RATHER THAN `seq_scan == 0`. + Asserting zero would fail for a defect this change does not fix, and asserting + one would pin a number #1210 is expected to move. + + The two shapes are separate arms because they reach different code. A no-qual + count never reaches the row-group-limit lookup, so its only storage access is + the format-version one and `seq_scan == 0` reads cleanly for that site alone. + + HOW IT WAS FOUND: two instruments disagreeing, and both being right. An `elog` + inside `pgcolumnar_written_stripe_row_limit` counted 2 scans for a join; + `pg_stat_all_tables.seq_scan` on the catalog counted 4. An elog at one + function counts arrivals at that function; a counter on the relation counts + scans from any caller. The gap was the second reader. + - Eleven suites built with their own `make`, so neither the build stamp (#536) nor the cross-major object check (#1219) protected them (#1220). They now route through `pgc_build_and_install`. diff --git a/src/columnar_metadata.c b/src/columnar_metadata.c index 6080216e..dd1bcb91 100644 --- a/src/columnar_metadata.c +++ b/src/columnar_metadata.c @@ -2315,6 +2315,7 @@ PgColumnarGetSortedInfo(uint64 storageId, int64 *firstGroup, int64 *lastGroup, ScanKeyData key[1]; SysScanDesc scan; HeapTuple tuple; + Oid storIdx; *firstGroup = -1; *lastGroup = -1; @@ -2325,7 +2326,23 @@ PgColumnarGetSortedInfo(uint64 storageId, int64 *firstGroup, int64 *lastGroup, tupdesc = RelationGetDescr(rel); ScanKeyInit(&key[0], Anum_native_storage_storage_id, BTEqualStrategyNumber, F_INT8EQ, Int64GetDatum((int64) storageId)); - scan = systable_beginscan(rel, InvalidOid, false, NULL, 1, key); + + /* + * NAME THE INDEX (#1237). The key is storage_id, which storage_pkey is a + * UNIQUE btree on, so this was a sequential scan of the catalog on the one + * column it is indexed by -- 2 * pages per call, growing with the number of + * columnar relations in the database. + * + * It matters because this is on the PLANNING path, not a maintenance one: + * pgcolumnar_sorted_pathkeys reaches it at columnar_customscan.c:1542. The + * comment above says "used by recluster's self-gate", which was true when + * it was written and is no longer the whole truth. + * + * Unlike #1210's lookup on relation_oid, nothing has to be built and no + * decision about #1211 is involved: the index exists and the key is exact. + */ + storIdx = pgcolumnar_index_oid("storage_pkey"); + scan = systable_beginscan(rel, storIdx, OidIsValid(storIdx), NULL, 1, key); tuple = systable_getnext(scan); if (HeapTupleIsValid(tuple)) { @@ -2392,11 +2409,25 @@ PgColumnarCheckNativeFormatVersion(uint64 storageId, const char *relName) HeapTuple tuple; bool found = false; int32 formatVersion = 0; + Oid storIdx; ScanKeyInit(&key[0], Anum_native_storage_storage_id, BTEqualStrategyNumber, F_INT8EQ, Int64GetDatum((int64) storageId)); /* NULL snapshot -> catalog snapshot, same as the other read-side scans. */ - scan = systable_beginscan(rel, InvalidOid, false, NULL, 1, key); + /* + * NAME THE INDEX (#1237), for the same reason as PgColumnarGetSortedInfo + * above: storage_pkey is a UNIQUE btree on storage_id. This one is NOT on + * the planning path -- measured by @OffgridwithJD, fmtver=0 in all five + * planned shapes -- but it is reached ONCE PER COLUMNAR RELATION SCANNED at + * execution, from columnar_reader.c:644, and a `count(*)` that pays nothing + * at planning still pays this. + * + * Whether that cost MATTERS against actually reading the relation's data is + * not claimed here and has not been measured. The scan is wrong either way: + * it is sequential on an indexed primary key. + */ + storIdx = pgcolumnar_index_oid("storage_pkey"); + scan = systable_beginscan(rel, storIdx, OidIsValid(storIdx), NULL, 1, key); tuple = systable_getnext(scan); if (HeapTupleIsValid(tuple)) diff --git a/test/catalog_plan_index.sh b/test/catalog_plan_index.sh index 63913a40..2c0d60f6 100755 --- a/test/catalog_plan_index.sh +++ b/test/catalog_plan_index.sh @@ -73,4 +73,66 @@ check_num "planning probed pgcolumnar.projection through projection_pkey" \ check_num "planning did not sequentially scan pgcolumnar.projection" \ "$prj_seq" "0" +# ---- and pgcolumnar.storage, through storage_pkey (#1237) ------------------- +# +# Two readers key on storage_id and both passed InvalidOid, so both scanned the +# catalog sequentially on the one column storage_pkey is a UNIQUE btree over: +# +# PgColumnarGetSortedInfo PLANNING, via pgcolumnar_sorted_pathkeys +# PgColumnarCheckNativeFormatVersion EXECUTION, once per relation scanned +# +# THE TWO SHAPES ARE DIFFERENT ARMS BECAUSE THEY REACH DIFFERENT CODE, and a +# single shape cannot tell them apart. Measured on the unfixed tree, scans of +# pgcolumnar.storage per planned query by shape: +# +# count(*), no qual 0 at planning, 1 at execution +# qual on a plain column 1 +# qual on a column with a projection 4 +# two columnar relations, one qual 4 <- 2 limit lookups + 2 sorted lookups +# +# A `count(*)` never reaches the row-group-limit lookup, so the ONLY storage +# access it makes is the format-version one. That makes seq_scan == 0 a clean +# reading for that site and nothing else. +# +# THE JOIN IS MEASURED ON idx_scan RATHER THAN seq_scan, deliberately. Its two +# remaining sequential scans come from pgcolumnar_written_stripe_row_limit, +# which keys on relation_oid and has NO index to name -- that is #1210 and +# #1211 and is not this change. Asserting seq_scan == 0 there would fail for a +# defect this change does not claim to fix, and asserting seq_scan == 2 would +# pin a number that #1210 is expected to move. +q "CREATE TABLE plan_cat_j (id int) USING pgcolumnar; + INSERT INTO plan_cat_j SELECT g FROM generate_series(1,800) g;" >/dev/null + +storage_stat() { # -> "idx_scan seq_scan" + q "SELECT coalesce(idx_scan,0)::text || ' ' || coalesce(seq_scan,0)::text + FROM pg_stat_all_tables + WHERE schemaname = 'pgcolumnar' AND relname = 'storage';" +} + +q "SELECT pg_stat_reset();" >/dev/null +q "SELECT count(*) FROM plan_cat;" >/dev/null +q "SELECT pg_stat_force_next_flush();" >/dev/null +st="$(storage_stat)" +st_idx="${st%% *}" +st_seq="${st##* }" +echo "-- storage after count(*) idx_scan=$st_idx seq_scan=$st_seq" + +check_num "premise: a no-qual count over a columnar table touched storage at all" \ + "$(if [ "$((st_idx + st_seq))" -ge 1 ]; then echo 1; else echo 0; fi)" "1" +check_num "a no-qual count did not sequentially scan pgcolumnar.storage" \ + "$st_seq" "0" + +q "SELECT pg_stat_reset();" >/dev/null +q "SELECT count(*) FROM plan_cat a JOIN plan_cat_j b ON a.id = b.id WHERE a.id > 0;" >/dev/null +q "SELECT pg_stat_force_next_flush();" >/dev/null +sj="$(storage_stat)" +sj_idx="${sj%% *}" +sj_seq="${sj##* }" +echo "-- storage after a two-relation join idx_scan=$sj_idx seq_scan=$sj_seq" + +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" +check_num "planning a join probed pgcolumnar.storage through storage_pkey" \ + "$(if [ "$sj_idx" -ge 2 ]; then echo 1; else echo 0; fi)" "1" + pgc_summary diff --git a/test/check_ledger.tsv b/test/check_ledger.tsv index 1c5ebb69..97ebc37a 100644 --- a/test/check_ledger.tsv +++ b/test/check_ledger.tsv @@ -20,11 +20,15 @@ capability_sweep capability_sweep premise: the shim makes pyarrow unimportable 1 capability_sweep capability_sweep premise: the sweep is not in the population it sweeps 15;16;17;18;19 2026-09-23 the self-exclusion removed from pgc_cap_population, so the sweep enters its own population capability_sweep capability_sweep premise: the two derivations of the population name the same suites 15;16;17;18;19 never - capability_sweep capability_sweep premise: while python itself still runs under the shim 15;16;17;18;19 never - +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 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 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: 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 - differential differential agg avg 15;16;17;18;19 never - differential differential agg count 15;16;17;18;19 never - diff --git a/test/check_ledger_budget.txt b/test/check_ledger_budget.txt index 8a8c7170..74ebd000 100644 --- a/test/check_ledger_budget.txt +++ b/test/check_ledger_budget.txt @@ -546,4 +546,52 @@ suites_not_covered 249 # this shows; they are unattacked, and they need mutations aimed at plan shape, # the NULL handling and the baseline join rather than at the filter's verdict. # Saying which is the next reader's job, not an entry in this file. -checks_never_observed_red 1519 +# 1519 -> 1521 for #1237's four new catalog_plan_index checks. Re-derived by +# COUNTING on this tree: +# +# awk -F'\t' '$5=="never"' test/check_ledger.tsv | wc -l -> 1521 +# +# with the premise beside it, because a pattern matching nothing counts 0 and +# reads like a clean answer: 1606 rows total, 85 not `never`, 1521 + 85 == 1606. +# +# FOUR ROWS AND THE CENSUS MOVES BY TWO. Two carry a measured last-red from one +# mutation, both `storage_id` scans reverted to `InvalidOid`: +# +# a no-qual count did not sequentially scan pgcolumnar.storage got [1] want [0] +# planning a join probed pgcolumnar.storage through storage_pkey got [0] want [1] +# +# The two that stay `never` are fixture premises: that a no-qual count reached +# the catalog at all, and that the join reached it more than once. They exist so +# the two arms above cannot pass over a measurement that never happened. +# +# RED BEFORE GREEN, WITH SOURCE AND BINARY IN STEP EACH TIME: +# +# unfixed count(*) idx=0 seq=1 join idx=0 seq=5 8 passed + 2 failed +# fixed count(*) idx=1 seq=0 join idx=4 seq=1 10 passed + 0 failed +# +# and the pytest twin separately, because the twins' regimes diverge: unfixed +# rc=1 with `storage after count(*) idx_scan=0 seq_scan=1`, fixed rc=0 with 10 +# of 10. The twin aborts at its first failure where the shell suite runs on, so +# it reports 8 checks against the shell's 10 -- a divergence in the FAILURE +# output, not in the names. +# +# THE MAJOR SET WAS DERIVED FROM FIVE RUNS against DEFAULT_CONFIGS read out of +# run_all_versions.sh: rc=0 and 10 passed + 0 failed on pg15, pg16, pg17, pgsql +# (the runner's PG18) and pg19. +# +# FIVE INSTRUMENT FAILURES BEFORE THE FIRST MEASUREMENT, none of them about the +# change, and one defect behind all five. Each time the run produced no verdict +# and each time a grep written for the expected answer hid the cause: +# +# a FATAL matching none of PASS|FAIL|accounting, read as "ran, printed nothing" +# a raw `make install`, which does NOT write the source stamp -- only +# pgc_build_and_install does -- so the freshness gate refused (#1230) +# a harness build as `postgres` into a tree a root-run `make` had left +# root-owned, dying on Permission denied writing a .d file +# a grep for `storage` matching the TEST FILE'S OWN COMMENTS echoed back +# +# `head -12` on the log settled it in one go. READ THE TAIL, THEN FILTER ONCE +# YOU KNOW WHAT IS THERE. +# +# suites_not_covered does NOT move: catalog_plan_index is already covered. +checks_never_observed_red 1521 diff --git a/test/pytest/test_catalog_plan_index.py b/test/pytest/test_catalog_plan_index.py index c5455701..68bb6714 100644 --- a/test/pytest/test_catalog_plan_index.py +++ b/test/pytest/test_catalog_plan_index.py @@ -101,3 +101,78 @@ def test_catalog_plan_index(pgc_cluster, pgc_conn, expect): 0, "planning did not sequentially scan pgcolumnar.projection", ) + + # ---- and pgcolumnar.storage, through storage_pkey (#1237) -------------- + # + # Two readers key on storage_id and both passed InvalidOid, so both scanned + # the catalog sequentially on the one column storage_pkey is a UNIQUE btree + # over: PgColumnarGetSortedInfo at PLANNING (via pgcolumnar_sorted_pathkeys) + # and PgColumnarCheckNativeFormatVersion once per relation scanned at + # EXECUTION. + # + # TWO SHAPES BECAUSE THEY REACH DIFFERENT CODE. A no-qual count never + # reaches the row-group-limit lookup, so its only storage access is the + # format-version one and seq_scan == 0 reads cleanly. The join is measured + # on idx_scan instead: its remaining sequential scan comes from + # pgcolumnar_written_stripe_row_limit, which keys on relation_oid and has + # no index to name -- that is #1210 and #1211, not this change. Measured + # unfixed, join idx=0 seq=5; fixed, idx=4 seq=1, and the 1 is that lookup. + # + # Its own cursor and its own reader connection, like the arms above: the + # session that wrote the rows must not be the session being measured. + with pgc_conn.cursor() as cur: + cur.execute("CREATE TABLE planner_join (n bigint) USING pgcolumnar") + cur.execute( + f"INSERT INTO planner_join SELECT g FROM generate_series(1,{ROWS}) g" + ) + cur.execute("SELECT pg_stat_reset()") + + reader = psycopg.connect(pgc_cluster.dsn(), autocommit=True) + try: + with reader.cursor() as cur: + cur.execute(f'SET search_path TO "{schema}", public') + cur.execute("SELECT count(*) FROM planner_opts") + cur.execute("SELECT pg_stat_force_next_flush()") + finally: + reader.close() + + st_idx, st_seq = _stats(pgc_conn, "storage") + print(f"-- storage after count(*) idx_scan={st_idx} seq_scan={st_seq}") + expect.at_least( + st_idx + st_seq, + 1, + "premise: a no-qual count over a columnar table touched storage at all", + ) + expect.num( + st_seq, + 0, + "a no-qual count did not sequentially scan pgcolumnar.storage", + ) + + with pgc_conn.cursor() as cur: + cur.execute("SELECT pg_stat_reset()") + + reader = psycopg.connect(pgc_cluster.dsn(), autocommit=True) + try: + with reader.cursor() as cur: + cur.execute(f'SET search_path TO "{schema}", public') + cur.execute( + "SELECT count(*) FROM planner_opts a " + "JOIN planner_join b ON a.n = b.n WHERE a.n >= 1" + ) + cur.execute("SELECT pg_stat_force_next_flush()") + finally: + reader.close() + + sj_idx, sj_seq = _stats(pgc_conn, "storage") + print(f"-- storage after a two-relation join idx_scan={sj_idx} seq_scan={sj_seq}") + expect.at_least( + sj_idx + sj_seq, + 2, + "premise: the join reached storage more than once", + ) + expect.at_least( + sj_idx, + 2, + "planning a join probed pgcolumnar.storage through storage_pkey", + )