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", + )