opt: one parameterised scan was the whole free_space cost (#1207)

src/columnar_metadata.c

@@ -1923,11 +1923,28 @@ delete_rows_by_storage_id(const char *tableName, AttrNumber storageAttno,
 	ScanKeyData key[1];
 	SysScanDesc scan;
 	HeapTuple	tuple;
+	Oid			idx;
 
 	ScanKeyInit(&key[0], storageAttno, BTEqualStrategyNumber,
 				F_INT8EQ, Int64GetDatum((int64) storageId));
 
-	scan = systable_beginscan(rel, InvalidOid, false, NULL, 1, key);
+	/*
+	 * ONE LINE THAT IS SEVEN SITES (#1207). The catalog name and the key
+	 * attnum both arrive as parameters, so the issue's stated method -- trace
+	 * the handle to its own open_columnar_table -- cannot classify this scan,
+	 * and it was left out of the population. Its call sites resolve it:
+	 * delete_vector, column_chunk, zone_map, bloom, free_space, row_group and
+	 * storage, every one keyed on storage_id and every one with a primary key
+	 * whose first column is storage_id. Each catalog's index follows the
+	 * <name>_pkey convention, checked for all seven.
+	 *
+	 * Size-aware like every other converted site: below the threshold this
+	 * resolves to InvalidOid and the scan stays sequential, which is what
+	 * keeps a small catalog from paying for a probe it cannot use (#1213).
+	 */
+	idx = pgcolumnar_scan_index_oid(rel, psprintf("%s_pkey", tableName));
+
+	scan = systable_beginscan(rel, idx, OidIsValid(idx), NULL, 1, key);
 	while (HeapTupleIsValid(tuple = systable_getnext(scan)))
 		CatalogTupleDelete(rel, &tuple->t_self);
 	systable_endscan(scan);

test/catalog_delete_index.sh

@@ -470,4 +470,89 @@ check_num "the vacuum's default does less row_group work than reading it whole"
 	"$(margin "$(permille "$((v_scan - v_default))" "$v_default")" $FLOOR_PERMILLE)" \
 	"$FLOOR_PERMILLE"
 
+# ---- the DROP path: delete_rows_by_storage_id, seven catalogs (#1207) -------
+#
+# THIS FILE DID NOT COVER THE CONVERSION IT LOOKS LIKE IT COVERS. Everything
+# above drives `delete_group_rows` (columnar_metadata.c:745), the retire path.
+# The seven-catalog sweep is `delete_rows_by_storage_id` (:1919), reached only
+# from PgColumnarDeleteMetadata on DROP and TRUNCATE -- and this suite contained
+# no DROP TABLE at all. Across the whole corpus no suite paired a catalog-work
+# assertion with a DROP, so seq_scan 2 -> 0 lived only in a PR body, which
+# nothing executes. Reported by @jdatcmd.
+#
+# WORK, NOT THE ACCESS PATH, for the reason at the top of this file: an arm
+# asserting seq_scan=0 fails against a build that makes the drop cheaper some
+# other way. And an arm asserting only that the rows were deleted passes with
+# InvalidOid, since a sequential scan deletes them just as correctly -- that is
+# the vacuous version this one exists instead of.
+#
+# drop_work TABLE [INDEX_MIN_BLOCKS] -- buffers the seven catalogs served while
+# TABLE was dropped. A DROP cannot be repeated, so each reading gets its own
+# identically built table.
+DCATS="$CATS,'storage'"
+drop_work() {
+	local set_clause=""
+	[ $# -ge 2 ] && set_clause="SET pgcolumnar.index_min_blocks = $2; "
+	q "SELECT pg_stat_reset();" >/dev/null
+	q "${set_clause}DROP TABLE $1;" >/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 ($DCATS);"
+}
+
+# Enough neighbours that the catalogs are worth an index. The conversion is
+# size-aware, so with a few pages the default DECLINES the probe and the two
+# readings converge -- which is the second arm, not a failure of the first.
+for i in $(seq 1 24); do make_target "drp_fill_$i" 6; done
+for t in drp_default drp_whole; do make_target "$t" 6; done
+
+# `margin`, not a 1-or-0: this arm reports the page count it saw. Part 540
+# refused the first version by name, and its rule is why -- a `-ge` against
+# anything but 0 or 1 discards a real number, where `-gt 0` below is an honest
+# presence check and is excused. `got [0] want [1]` would read the same at two
+# pages and at zero, and those are a thin fixture and a broken one.
+check_num "premise: the drop fixture grew the catalogs it is there to grow" \
+	"$(margin "$(catpages)" 3)" "3"
+check_num "premise: the table about to be dropped owns catalog rows" \
+	"$([ "$(groups_of drp_default)" -gt 0 ] && echo 1 || echo 0)" "1"
+
+d_default="$(drop_work drp_default)";               echo "--   drop default:    $d_default"
+d_whole="$(drop_work drp_whole 2147483647)";        echo "--   drop read-whole: $d_whole"
+echo "-- drop  catalog pages=$(catpages)  default=$d_default read-whole=$d_whole"
+
+check_num "the drop's default does less catalog work than reading them whole" \
+	"$(margin "$(permille "$((d_whole - d_default))" "$d_default")" $FLOOR_PERMILLE)" \
+	"$FLOOR_PERMILLE"
+
+# THE OTHER SIDE, and the arm that says the size check still protects a small
+# database: with the catalogs below the threshold the default declines the probe,
+# so forcing a probe cannot beat it.
+q "DROP TABLE IF EXISTS drp_small_a, drp_small_b;" >/dev/null 2>&1
+for t in drp_small_a drp_small_b; do
+	q "CREATE TABLE $t (id int) USING pgcolumnar;
+	   INSERT INTO $t SELECT g FROM generate_series(1,50) g;" >/dev/null
+done
+s_default="$(drop_work drp_small_a)"
+s_probe="$(drop_work drp_small_b 0)"
+echo "-- drop small  default=$s_default probe-always=$s_probe"
+# STRICT, NOT `-le`, AND THAT IS THE WHOLE ARM. The first version asked only
+# that the default do no MORE work than a forced probe. A build with the size
+# check REMOVED -- probing unconditionally -- makes the two readings equal, and
+# `-le` passes on it: the arm could not tell "the check is present and
+# declining" from "there is no check". That is the same vacuity as an arm
+# asserting only that the rows were deleted, which is what this section exists
+# instead of. Reported by @jdatcmd.
+#
+# Requiring a MARGIN reddens under both mutations, so this arm witnesses the
+# conversion rather than merely guarding a future one:
+#
+#     conversion present, declining      24 vs 27     125 permille   PASS
+#     size check removed, always probe   27 vs 27       0            FAIL
+#     conversion reverted, sequential   101 vs 101      0            FAIL
+check_num "with few catalog pages the drop's default does less work than probing every one" \
+	"$(margin "$(permille "$((s_probe - s_default))" "$s_default")" $FLOOR_PERMILLE)" \
+	"$FLOOR_PERMILLE"
+
 pgc_summary

test/check_ledger.tsv

@@ -66,6 +66,7 @@ catalog_delete_index	catalog_delete_index	B1 with large catalogs the default doe
 catalog_delete_index	catalog_delete_index	P1 with a few catalog pages the default does less work than probing every one	15;16;17;18;19	2026-09-24	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	premise: row_group is larger than the threshold, so the two paths differ	15;16;17;18;19	2026-09-24	pgcolumnar_index_min_blocks default 3 -> 2147483647, so no site probes
 catalog_delete_index	catalog_delete_index	premise: the deep table grew the catalogs it is there to grow	15;16;17;18;19	never	-
+catalog_delete_index	catalog_delete_index	premise: the drop fixture grew the catalogs it is there to grow	15;16;17;18;19	never	-
 catalog_delete_index	catalog_delete_index	premise: the phase 0 catalogs hold only this file's tables	15;16;17;18;19	never	-
 catalog_delete_index	catalog_delete_index	premise: the phase 0 compaction kept every surviving row	15;16;17;18;19	never	-
 catalog_delete_index	catalog_delete_index	premise: the phase 0 compaction retired the emptied groups	15;16;17;18;19	never	-
@@ -81,6 +82,7 @@ catalog_delete_index	catalog_delete_index	premise: the phase B compaction kept e
 catalog_delete_index	catalog_delete_index	premise: the phase B compaction retired the emptied groups	15;16;17;18;19	never	-
 catalog_delete_index	catalog_delete_index	premise: the phase B reading covers every catalog the arms name	15;16;17;18;19	never	-
 catalog_delete_index	catalog_delete_index	premise: the phase B reading measured something	15;16;17;18;19	never	-
+catalog_delete_index	catalog_delete_index	premise: the table about to be dropped owns catalog rows	15;16;17;18;19	never	-
 catalog_delete_index	catalog_delete_index	premise: the three phase 0 targets are the same fixture	15;16;17;18;19	never	-
 catalog_delete_index	catalog_delete_index	premise: the three phase A targets are the same fixture	15;16;17;18;19	never	-
 catalog_delete_index	catalog_delete_index	premise: the three phase B targets are the same fixture	15;16;17;18;19	never	-
@@ -90,7 +92,9 @@ catalog_delete_index	catalog_delete_index	premise: two phase A compactions at th
 catalog_delete_index	catalog_delete_index	premise: two phase B compactions at the same setting agree well inside the floor	15;16;17;18;19	never	-
 catalog_delete_index	catalog_delete_index	premise: two vacuums of the same table at the same setting agree well inside the floor	15;16;17;18;19	never	-
 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 drop's default does less catalog work than reading them whole	15;16;17;18;19	never	-
 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_delete_index	catalog_delete_index	with few catalog pages the drop's default does less work than probing every one	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	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

test/check_ledger_budget.txt

@@ -1177,4 +1177,4 @@ suites_not_covered 247
 # REBASED FROM 1bcfb92b ONTO 81ec3ddc, 32 commits, by the maintainer rather than
 # the author. The branch predates #1261, and three of its five CI reds were
 # fixed by the rebase alone rather than by any change here -- see the PR.
-checks_never_observed_red 1610
+checks_never_observed_red 1614

test/pytest/TESTS.md

@@ -6233,6 +6233,7 @@ THE ONE THING THIS HALF HAS TO DO THAT THE SHELL HALF DOES NOT is flush the stat
 | --- | --- |
 | `test_retiring_a_group_costs_no_more_for_a_bigger_database` | per phase: three identical targets, the emptied groups retired, every survivor kept, all six catalogs read, and two compactions at the same setting agreeing well inside the floor. Then that the default does less work than probing every catalog at six pages, less than reading every one whole at twenty-two and at seventy-six, and that its cost grows far less with the database than reading whole does |
 | `test_vacuum_reads_less_of_row_group_than_reading_it_whole` | that the vacuum walked this table's groups, that two vacuums at the same setting agree, and that its `row_group` work is below what reading that catalog whole costs -- the `PgColumnarComputeAllVisibleGroups` read, which the compaction path never reaches |
+| `test_dropping_a_table_reads_less_of_the_catalogs_than_reading_them_whole` | that a DROP costs less catalog work than reading the seven catalogs whole, and -- below the threshold -- less than forcing a probe. The subject is `delete_rows_by_storage_id`, one parameterised line serving seven catalogs on the DROP/TRUNCATE path, which nothing covered before (#1207). Measured in buffers, never the access path, and on a fixture sixty tables wide because `pgc_own_db` gives this file a private database where the shell twin shares a cluster |
 
 ## 82. test_rewrite_storage_oid.py: a type rewrite keeps the storage row on the live table
 

test/pytest/expected_tests.txt

@@ -529,4 +529,4 @@ guard_tests 403
 # The branch was 32 commits behind and declared 479, a number derived against
 # 1bcfb92b. Carrying that forward would have been a count measured on a tree
 # that no longer exists; 483 is measured on this one.
-cluster_tests 483
+cluster_tests 484

test/pytest/test_catalog_delete_index.py

@@ -506,3 +506,106 @@ def vacuum_work(min_blocks=None):
         FLOOR_PERMILLE,
         "the vacuum's default does less row_group work than reading it whole",
     )
+
+
+# ---- the DROP path: delete_rows_by_storage_id, seven catalogs (#1207) -------
+#
+# The shell twin's section explains the subject: everything above drives
+# `delete_group_rows` (columnar_metadata.c:745), the retire path, while the
+# seven-catalog sweep is `delete_rows_by_storage_id` (:1919), reached only from
+# PgColumnarDeleteMetadata on DROP and TRUNCATE. Neither half covered it.
+#
+# MEASURED HERE, NOT BORROWED. This file opens its own database, builds its own
+# fixture and reads pg_statio itself. Parallel in what it asserts, independent
+# in what it calls -- lifting the shell helpers so both could drive them would
+# make the extraction the dependency.
+DROP_CATALOGS = tuple(CATALOGS) + ("storage",)
+
+
+def _drop_work(conn, table, min_blocks=None):
+    """Buffers the seven catalogs served while `table` was dropped.
+
+    A DROP cannot be repeated, so each reading needs its own identically built
+    table.
+    """
+    with conn.cursor() as cur:
+        # FLUSH BEFORE THE RESET, for the reason _compact_work records: this
+        # harness holds ONE connection, so the writes that built the fixture
+        # leave pending statistics that pg_stat_reset() does not clear and that
+        # land on top of the reading. Omitting it here read -988 permille --
+        # the first DROP absorbing the whole fixture build -- against +125 for
+        # the shell twin, which gets a fresh backend per statement.
+        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}")
+        cur.execute(f"DROP TABLE {table}")
+        cur.execute("RESET pgcolumnar.index_min_blocks")
+        cur.execute("SELECT pg_stat_force_next_flush()")
+        cur.execute(
+            "SELECT coalesce(sum("
+            "  coalesce(heap_blks_read,0) + coalesce(heap_blks_hit,0) "
+            "+ coalesce(idx_blks_read,0) + coalesce(idx_blks_hit,0)), 0) "
+            "FROM pg_statio_all_tables "
+            "WHERE schemaname = 'pgcolumnar' AND relname = ANY(%s)",
+            (list(DROP_CATALOGS),),
+        )
+        return int(cur.fetchone()[0])
+
+
+def test_dropping_a_table_reads_less_of_the_catalogs_than_reading_them_whole(
+    pgc_own_db, expect
+):
+    """The DROP sweep must use the index once the catalogs are worth one.
+
+    WORK, NOT THE ACCESS PATH, for the reason in this file's header: an arm
+    asserting `seq_scan = 0` fails against a build that makes the drop cheaper
+    some other way. And an arm asserting only that the rows were deleted passes
+    with InvalidOid, because a sequential scan deletes them just as correctly --
+    that is the vacuous version this test exists instead of.
+    """
+    conn = pgc_own_db
+    # MORE NEIGHBOURS THAN THE SHELL TWIN NEEDS, and the reason is the fixture
+    # rather than the property. `pgc_own_db` gives this file a private database,
+    # so the catalogs hold only what this test builds -- where the shell twin
+    # shares a cluster with everything before it. At 24 fill tables the margin
+    # read 53 permille here against 901 there, both correct measurements of
+    # different databases.
+    for i in range(1, 61):
+        _make_target(conn, f"drp_fill_{i}", 6)
+    for t in ("drp_default", "drp_whole"):
+        _make_target(conn, t, 6)
+
+    expect.at_least(
+        _catpages(conn), 3,
+        "premise: the drop fixture grew the catalogs it is there to grow",
+    )
+    expect.at_least(
+        _groups_of(conn, "drp_default"), 1,
+        "premise: the table about to be dropped owns catalog rows",
+    )
+
+    d_default = _drop_work(conn, "drp_default")
+    d_whole = _drop_work(conn, "drp_whole", 2147483647)
+    print(f"-- drop  catalog pages={_catpages(conn)} "
+          f"default={d_default} read-whole={d_whole}")
+    expect.at_least(
+        _permille(d_whole - d_default, d_default), FLOOR_PERMILLE,
+        "the drop's default does less catalog work than reading them whole",
+    )
+
+    # THE OTHER SIDE. Below the threshold the default declines the probe, so
+    # forcing one must cost MORE. A `<=` form would pass on a build with the
+    # size check removed -- the two readings are then equal -- which is the same
+    # vacuity as asserting only that the rows were deleted.
+    for t in ("drp_small_a", "drp_small_b"):
+        with conn.cursor() as cur:
+            cur.execute(f"CREATE TABLE {t} (id int) USING pgcolumnar")
+            cur.execute(f"INSERT INTO {t} SELECT g FROM generate_series(1,50) g")
+    s_default = _drop_work(conn, "drp_small_a")
+    s_probe = _drop_work(conn, "drp_small_b", 0)
+    print(f"-- drop small  default={s_default} probe-always={s_probe}")
+    expect.at_least(
+        _permille(s_probe - s_default, s_default), FLOOR_PERMILLE,
+        "with few catalog pages the drop's default does less work than probing every one",
+    )