diff --git a/CHANGELOG.md b/CHANGELOG.md index 41f1f6af..5d6686b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -390,6 +390,19 @@ true until the next version shipped. and cuts sentences with `sed`; the pytest half splits on a lookbehind and collects with `re`, so a parsing mistake in one is not a parsing mistake in the other. +- Planning a columnar query sequentially scanned `pgcolumnar.options` and `pgcolumnar.projection` (#1198). + + `options_pkey` is `(regclass)` and `projection_pkey` leads with `storage_id`. The planner looks those catalogs up by those columns, and the scans passed `InvalidOid`. One filtered scan of one table then walked every columnar table's options row and every projection row. Measured on PG18 before the change, after `pg_stat_reset()`: + + | catalog | idx_scan | seq_scan | + | --- | ---: | ---: | + | options | 0 | 2 | + | projection | 0 | 2 | + + Both readers now pass their primary key, as `row_group` and `delete_vector` already did. The same lookup on `pgcolumnar.storage` stays a sequential scan: `storage_pkey` is on `storage_id` and that reader looks up by `relation_oid`. + + After the change the same scan reports `idx_scan=2` and `seq_scan=0` on both catalogs, and the filtered count is still every row. Putting `InvalidOid` back on the two planner scans returns `idx_scan=0` and `seq_scan=2`. + - `native_reclaim_cycles` could not reach the defect it guards (#1138). It is the declared regression guard for #84. Deleting the #84 fix left it reporting diff --git a/src/columnar_metadata.c b/src/columnar_metadata.c index efe0daa7..6080216e 100644 --- a/src/columnar_metadata.c +++ b/src/columnar_metadata.c @@ -2031,7 +2031,49 @@ PgColumnarRenameDeclaredSortByColumn(Oid relid, const char *oldName, tupdesc = RelationGetDescr(rel); ScanKeyInit(&key[0], Anum_options_regclass, BTEqualStrategyNumber, F_OIDEQ, ObjectIdGetDatum(relid)); - scan = systable_beginscan(rel, InvalidOid, false, NULL, 1, key); + + /* + * options_pkey, like every other scan whose key is that column. The sixth + * site: five moved when the others did and this one did not, and the + * population was a list of function names rather than the property + * "the key column IS the index's column" -- which is what a list silently + * regrows past. + * + * AND THE PROPERTY IS WIDER THAN THIS CHANGE. Counted on both trees rather + * than on one: + * + * main 44 systable_beginscan calls, 31 InvalidOid, 13 indexed + * this branch 44 , 24 , 20 + * + * so this change resolves SEVEN sites, and 21 of the remaining 24 have a scan + * key that is a prefix of an existing index. None of the 21 is called a defect + * here, because none has been measured the way these seven were. #1207 carries + * them, with line numbers. + * + * NO PER-CATALOG TABLE HERE, DELIBERATELY. Three independent sweeps agreed on + * 21 and disagreed on how it splits, because a sweep that searches BACKWARDS + * for the nearest ScanKeyInit mis-assigns any function that scans two + * catalogs -- PgColumnarCheckFreeSpaceNoOverlap scans row_group at 1020 and + * free_space at 1041, and a backward search gives both to whichever key it + * meets first. The relation HANDLE passed to systable_beginscan is the ground + * truth; tracing it to its open_columnar_table("") settles it. A table + * of counts in a comment is the thing this comment is warning about. + * + * THE COUNTS ABOVE ARE OF CALLS, NOT OF THE STRING. A first draft said 46 and + * "six", from a `grep -c systable_beginscan` that counted this very + * paragraph's own sentence about systable_beginscan. A comment describing a + * sweep is input to that sweep, which is the same trap one level down from the + * one it is describing. + * + * NO MEASUREMENT WILL SHOW THIS ONE. It runs on ALTER TABLE ... RENAME + * COLUMN, not on a plan, so the planner-path probe that found the others + * cannot reach it. That is the reason to fix it rather than a reason not to. + */ + { + Oid optIdx = pgcolumnar_index_oid("options_pkey"); + + scan = systable_beginscan(rel, optIdx, OidIsValid(optIdx), NULL, 1, key); + } if (HeapTupleIsValid(tuple = systable_getnext(scan))) { bool isnull; @@ -3183,7 +3225,16 @@ PgColumnarReadOptions(Oid relid, PgColumnarOptions *opts) ScanKeyInit(&key[0], Anum_options_regclass, BTEqualStrategyNumber, F_OIDEQ, ObjectIdGetDatum(relid)); - scan = systable_beginscan(rel, InvalidOid, false, snapshot, 1, key); + /* + * options_pkey is (regclass), the same column this key names. Passing + * InvalidOid walked every columnar table's options row on a plan that + * asked about one of them. + */ + { + Oid optIdx = pgcolumnar_index_oid("options_pkey"); + + scan = systable_beginscan(rel, optIdx, OidIsValid(optIdx), snapshot, 1, key); + } if (HeapTupleIsValid(tuple = systable_getnext(scan))) { bool isnull; @@ -3298,8 +3349,9 @@ pgcolumnar_effective_stripe_row_limit(Oid relid) * of groups on every plan, which is too much to spend refining a term that * is approximate by construction. * - * Scanned without an index, like PgColumnarReadOptions immediately above: - * the storage index is on storage_id and this looks up by relation_oid. + * Scanned without an index: storage_pkey is on storage_id and this + * looks up by relation_oid. options_pkey does match its lookup, and + * PgColumnarReadOptions uses it; this one cannot. */ int pgcolumnar_written_stripe_row_limit(Oid relid) @@ -3400,7 +3452,11 @@ PgColumnarReadTtl(Oid relid, char **column, Interval **interval) ScanKeyInit(&key[0], Anum_options_regclass, BTEqualStrategyNumber, F_OIDEQ, ObjectIdGetDatum(relid)); - scan = systable_beginscan(rel, InvalidOid, false, snapshot, 1, key); + { + Oid optIdx = pgcolumnar_index_oid("options_pkey"); + + scan = systable_beginscan(rel, optIdx, OidIsValid(optIdx), snapshot, 1, key); + } tuple = systable_getnext(scan); if (HeapTupleIsValid(tuple)) { @@ -3443,7 +3499,11 @@ PgColumnarReadSortBy(Oid relid) ScanKeyInit(&key[0], Anum_options_regclass, BTEqualStrategyNumber, F_OIDEQ, ObjectIdGetDatum(relid)); - scan = systable_beginscan(rel, InvalidOid, false, snapshot, 1, key); + { + Oid optIdx = pgcolumnar_index_oid("options_pkey"); + + scan = systable_beginscan(rel, optIdx, OidIsValid(optIdx), snapshot, 1, key); + } if (HeapTupleIsValid(tuple = systable_getnext(scan))) { bool isnull; @@ -3493,7 +3553,11 @@ PgColumnarDeleteOptions(Oid relid) ScanKeyInit(&key[0], Anum_options_regclass, BTEqualStrategyNumber, F_OIDEQ, ObjectIdGetDatum(relid)); - scan = systable_beginscan(rel, InvalidOid, false, NULL, 1, key); + { + Oid optIdx = pgcolumnar_index_oid("options_pkey"); + + scan = systable_beginscan(rel, optIdx, OidIsValid(optIdx), NULL, 1, key); + } while (HeapTupleIsValid(tuple = systable_getnext(scan))) CatalogTupleDelete(rel, &tuple->t_self); systable_endscan(scan); @@ -3910,9 +3974,18 @@ PgColumnarListProjections(uint64 storageId) ScanKeyInit(&key[0], Anum_projection_storage_id, BTEqualStrategyNumber, F_INT8EQ, Int64GetDatum((int64) storageId)); - /* NULL snapshot -> catalog snapshot: sees committed rows plus this - * transaction's own writes after a CommandCounterIncrement (DDL semantics). */ - scan = systable_beginscan(rel, InvalidOid, false, NULL, 1, key); + /* + * NULL snapshot -> catalog snapshot: sees committed rows plus this + * transaction's own writes after a CommandCounterIncrement (DDL + * semantics). projection_pkey leads with storage_id, so the index + * answers this key. The snapshot is what makes the index scan see + * those writes; dropping it would not. + */ + { + Oid projIdx = pgcolumnar_index_oid("projection_pkey"); + + scan = systable_beginscan(rel, projIdx, OidIsValid(projIdx), NULL, 1, key); + } while (HeapTupleIsValid(tuple = systable_getnext(scan))) { PgColumnarProjection *p = palloc0(sizeof(PgColumnarProjection)); @@ -3953,7 +4026,11 @@ PgColumnarDeleteProjectionRow(uint64 storageId, int projectionId) ScanKeyInit(&key[1], Anum_projection_projection_id, BTEqualStrategyNumber, F_INT4EQ, Int32GetDatum(projectionId)); - scan = systable_beginscan(rel, InvalidOid, false, NULL, 2, key); + { + Oid projIdx = pgcolumnar_index_oid("projection_pkey"); + + scan = systable_beginscan(rel, projIdx, OidIsValid(projIdx), NULL, 2, key); + } while (HeapTupleIsValid(tuple = systable_getnext(scan))) CatalogTupleDelete(rel, &tuple->t_self); systable_endscan(scan); diff --git a/test/catalog_plan_index.sh b/test/catalog_plan_index.sh new file mode 100755 index 00000000..63913a40 --- /dev/null +++ b/test/catalog_plan_index.sh @@ -0,0 +1,76 @@ +#!/usr/bin/env bash +# +# Planning a columnar query must probe pgcolumnar.options and +# pgcolumnar.projection through their primary keys. +# +# options_pkey is (regclass) and projection_pkey is (storage_id, +# projection_id). The planner looks options up by regclass and projections +# up by storage_id, and both scans passed InvalidOid, so every plan +# sequentially scanned those catalogs. A database with many columnar +# tables pays that on a query that touches one of them. +# +# After one filtered scan of this suite's own table, pg_stat_all_tables +# must show idx_scan > 0 and seq_scan = 0 for both catalogs. The filtered +# scan is its own psql, so the session that wrote the rows is not the +# session being measured. +# +# Usage: test/catalog_plan_index.sh [PG_CONFIG] + +set -uo pipefail +. "$(dirname "${BASH_SOURCE[0]}")/lib.sh" +pgc_setup "${1:-/usr/local/pg17/bin/pg_config}" + +q "CREATE EXTENSION IF NOT EXISTS pgcolumnar;" >/dev/null + +# 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; + CREATE TABLE noise_b (id int) USING pgcolumnar; + INSERT INTO noise_a SELECT g FROM generate_series(1,40) g; + INSERT INTO noise_b SELECT g FROM generate_series(1,60) g; + CREATE TABLE plan_cat (id int) USING pgcolumnar; + INSERT INTO plan_cat SELECT g FROM generate_series(1,800) g;" >/dev/null + +check_num "premise: the measured table holds its rows" \ + "$(q "SELECT count(*) FROM plan_cat;")" "800" + +q "SELECT pg_stat_reset();" >/dev/null +q "SELECT count(*) FROM plan_cat WHERE id > 0;" >/dev/null +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" + +pgc_summary diff --git a/test/check_ledger.tsv b/test/check_ledger.tsv index c72cb09b..011d396a 100644 --- a/test/check_ledger.tsv +++ b/test/check_ledger.tsv @@ -7,6 +7,12 @@ base_scan_io base_scan_io premise: the later plan is still a base columnar scan base_scan_io base_scan_io premise: the later plan still does not name a covering projection 15;16;17;18;19 never - base_scan_io base_scan_io premise: the plan is a base columnar scan 15;16;17;18;19 never - base_scan_io base_scan_io premise: the table holds every inserted row 15;16;17;18;19 never - +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: the filtered scan returned every row 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 - differential differential agg minmax 15;16;17;18;19 never - diff --git a/test/check_ledger_budget.txt b/test/check_ledger_budget.txt index ad89a958..95e19303 100644 --- a/test/check_ledger_budget.txt +++ b/test/check_ledger_budget.txt @@ -211,4 +211,19 @@ suites_not_covered 249 # sat, so neither the branch's previous value nor main's is the composed one. # 1499 -> 1506 for #1155's own nine rows, re-counted after this branch was # rebuilt on main rather than added to either side. -checks_never_observed_red 1506 +# REBUILT ON MAIN CARRYING #1155, and the number is neither side's. This branch +# derived 1501 against the previous main; main now holds 1506. 1501 is not 1506 +# minus anything and 1506 is not 1501 plus anything -- each was correct for a tree +# that no longer exists. Re-derived by COUNTING on this tree: +# +# awk -F'\t' '$5=="never"' test/check_ledger.tsv | wc -l -> 1508 +# +# with the premise printed beside it, because a pattern that matches nothing +# counts 0 and reads like a clean answer: 1541 rows total, 33 not `never`. +# +# suites_not_covered does NOT move. catalog_plan_index is a new registered suite +# that arrives WITH ledger rows, so it raises the registered count and the covered +# count by one each and leaves the difference alone. Measured on both trees rather +# than argued: not-covered identical on main and on this tree, ledger-only suites 0, +# and registered == covered + not-covered on each. +checks_never_observed_red 1508 diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 15bdace7..32546c40 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -125,6 +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) ## 1. How to read a test in here @@ -6144,3 +6145,17 @@ this file uses `pciot` / `onck` / 36000 rows. Assertion names match. | test | what it holds | | --- | --- | | `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 | + +## 80. test_catalog_plan_index.py: planning uses the options and projection indexes + +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. + +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. + +`pg_stat_reset()` is database-wide. The corpus runs serially within a worker. + +### Every test + +| 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 | diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index 8cc05b49..8941b1ce 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -500,4 +500,13 @@ guard_tests 403 # after a THIRD rebase. This value has read 464, 468, 469 and now 476 as main # went 463, 467, 468 and 475 under it; every one was correct for the main of # its hour and none survived the next merge. `476 tests collected`. -cluster_tests 476 +# cluster_tests moved 476 -> 477 for #1198's one new cluster file, +# test_catalog_plan_index.py. RE-DERIVED BY COLLECTION on the composed tree, not +# by adding one to either side. Both this branch and main carried 476 and the +# merge was silent, because the branch's 476 was measured against the previous +# main and main's 476 was measured without this branch: +# +# main alone guard=403 cluster=476 (50 cluster files) +# composed guard=403 cluster=477 (51 cluster files) +# +cluster_tests 477 diff --git a/test/pytest/test_catalog_plan_index.py b/test/pytest/test_catalog_plan_index.py new file mode 100644 index 00000000..c5455701 --- /dev/null +++ b/test/pytest/test_catalog_plan_index.py @@ -0,0 +1,103 @@ +"""Planning a columnar query uses the options and projection primary keys. + +`options_pkey` is `(regclass)` and `projection_pkey` leads with `storage_id`. +The planner looks those catalogs up by exactly those columns, and both scans +passed `InvalidOid`, so a plan sequentially scanned every columnar table's +options row and every projection row. + +Independent of `test/catalog_plan_index.sh`: same public seam +(`pg_stat_all_tables` after one filtered scan), own tables, own row counts, +own observations. The measured scan runs on a second connection. A session +that just wrote can take a different catalog path; this arm is about the +session that only plans and reads. + +`pg_stat_reset()` is database-wide. Tests run serially within a worker, so +nothing else is counting during this test, and this file must not be run +concurrently with another that reads statistics. +""" + +import psycopg + +ROWS = 1200 +# sum(1..1200) +FULL_SUM = ROWS * (ROWS + 1) // 2 + + +def _stats(conn, relname): + with conn.cursor() as cur: + cur.execute( + "SELECT coalesce(idx_scan,0), coalesce(seq_scan,0) " + "FROM pg_stat_all_tables " + "WHERE schemaname = 'pgcolumnar' AND relname = %s", + (relname,), + ) + row = cur.fetchone() + if row is None: + return 0, 0 + return int(row[0]), int(row[1]) + + +def test_catalog_plan_index(pgc_cluster, pgc_conn, expect): + with pgc_conn.cursor() as cur: + cur.execute("CREATE TABLE side_m (n bigint) USING pgcolumnar") + cur.execute("CREATE TABLE side_n (n bigint) USING pgcolumnar") + cur.execute("CREATE TABLE side_o (n bigint) USING pgcolumnar") + cur.execute("INSERT INTO side_m SELECT g FROM generate_series(1,17) g") + cur.execute("INSERT INTO side_n SELECT g FROM generate_series(1,19) g") + cur.execute("INSERT INTO side_o SELECT g FROM generate_series(1,23) g") + cur.execute("CREATE TABLE planner_opts (n bigint) USING pgcolumnar") + cur.execute( + f"INSERT INTO planner_opts SELECT g FROM generate_series(1,{ROWS}) g" + ) + cur.execute("SELECT count(*) FROM planner_opts") + expect.num( + cur.fetchone()[0], + ROWS, + "premise: the measured table holds its rows", + ) + cur.execute("SELECT current_schema()") + schema = cur.fetchone()[0] + 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 sum(n) FROM planner_opts WHERE n >= 1") + scanned = cur.fetchone()[0] + cur.execute("SELECT pg_stat_force_next_flush()") + finally: + reader.close() + + expect.num( + scanned, + FULL_SUM, + "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", + ) diff --git a/test/pytest/test_compare_to_bash.py b/test/pytest/test_compare_to_bash.py index e34b3d1b..03317a4c 100644 --- a/test/pytest/test_compare_to_bash.py +++ b/test/pytest/test_compare_to_bash.py @@ -90,6 +90,7 @@ "analyze_function", "analyze_reltuples", "base_scan_io", + "catalog_plan_index", "differential", "encode_post_codec", "hilbert_cluster", diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 2b3fb129..ad49e665 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -62,6 +62,7 @@ SUITES=( bloom_sizing cancel_decode catalog_natts + catalog_plan_index column_projection concurrency concurrent_diff