fix: a partitioned table carrying relam=pgcolumnar aborts the backend (#1259)

CHANGELOG.md

@@ -458,6 +458,36 @@ true until the next version shipped.
 
 ### Fixed
 
+- A partitioned table carrying `relam = pgcolumnar` no longer aborts the backend
+  (#1259).
+
+  `PgColumnarIsColumnarRelation` decided "is this ours?" from `relam` alone.
+  From PostgreSQL 17 a partitioned table may carry `relam = pgcolumnar` as the
+  default for its future partitions, and it has no storage: `relfilenode` is 0.
+  Every caller that took the true branch and then read storage handed
+  `smgropen` a zero relfilenumber, which on an assert build aborts the backend
+  and restarts the cluster:
+
+  ```
+    TRAP: failed Assert("RelFileNumberIsValid(rlocator.relNumber)"), smgr.c:204
+    LOG:  server process was terminated by signal 6: Aborted
+  ```
+
+  On a production build it does not crash, it answers wrong: `get_storage_id`
+  returns NULL and `stats` and `sort_status` build on that.
+
+  The predicate now also requires storage. **`RELKIND_HAS_STORAGE` rather than
+  `== RELKIND_RELATION`, and the difference is load-bearing:** a materialized
+  view can be columnar -- measured, `relkind` `m`, `relam` `pgcolumnar`, rows
+  readable -- and narrowing to `RELKIND_RELATION` would have stopped
+  recognising one at all 31 call sites, silently. A test arm drives exactly that
+  wrong fix.
+
+  All 31 call sites were classified before the change: none legitimately wants
+  the partitioned parent. `set_options` refuses it and `add_projection` aborted
+  on it, so the two catalog renames in `pgcolumnar_process_utility` have nothing
+  to maintain there.
+
 - The ANALYZE cap's input guard defends a class, and the comment named one
   member of it (#1252).
 

src/columnar_metadata.c

@@ -22,6 +22,7 @@
 #include "access/xact.h"
 #include "catalog/indexing.h"
 #include "catalog/namespace.h"
+#include "catalog/pg_class.h"
 #include "catalog/pg_type.h"
 #include "commands/defrem.h"
 #include "commands/sequence.h"
@@ -3761,8 +3762,31 @@ PgColumnarDeleteOptions(Oid relid)
 
 /*
  * PgColumnarIsColumnarRelation
- *		Whether a relation uses the columnar table access method. The access
- *		method oid is resolved once and cached.
+ *		Whether a relation uses the columnar table access method AND has
+ *		storage of its own. The access method oid is resolved once and cached.
+ *
+ *		THE RELKIND TEST IS NOT DECORATION (#1259). From PostgreSQL 17 a
+ *		PARTITIONED table may carry relam = pgcolumnar, as the default access
+ *		method for its future partitions, and it has no storage: relfilenode is
+ *		0. Every caller that took the true branch and then read storage handed
+ *		smgropen a zero relfilenumber, which on an assert build aborts the
+ *		backend and restarts the cluster:
+ *
+ *		    TRAP: failed Assert("RelFileNumberIsValid(rlocator.relNumber)"),
+ *		          File: "smgr.c", Line: 204
+ *
+ *		Ten entry points were measured reaching it. On a production build it
+ *		does not crash, it answers wrong -- get_storage_id returns NULL and
+ *		stats and sort_status build on that -- which is the harder half to
+ *		notice.
+ *
+ *		RELKIND_HAS_STORAGE RATHER THAN == RELKIND_RELATION, and the difference
+ *		is load-bearing: a MATERIALIZED VIEW can be columnar. Measured --
+ *		`CREATE MATERIALIZED VIEW mv USING pgcolumnar AS ...` succeeds, and the
+ *		result has relkind 'm', relam 'pgcolumnar' and readable rows. Narrowing
+ *		to RELKIND_RELATION would have stopped recognising it at all 31 call
+ *		sites, silently. The macro admits it and excludes the partitioned
+ *		parent, which is exactly the property the callers need.
  */
 bool
 PgColumnarIsColumnarRelation(Oid relid)
@@ -3772,7 +3796,10 @@ PgColumnarIsColumnarRelation(Oid relid)
 	if (columnarAmOid == InvalidOid)
 		columnarAmOid = get_am_oid("pgcolumnar", true);
 
-	return OidIsValid(columnarAmOid) && get_rel_relam(relid) == columnarAmOid;
+	if (!OidIsValid(columnarAmOid) || get_rel_relam(relid) != columnarAmOid)
+		return false;
+
+	return RELKIND_HAS_STORAGE(get_rel_relkind(relid));
 }
 
 /* -------------------------------------------------------------------------

test/check_ledger.tsv

@@ -1545,6 +1545,32 @@ index_fetch_penalty_crossover	index_fetch_penalty_crossover	premise: the 50000-r
 index_fetch_penalty_crossover	index_fetch_penalty_crossover	premise: the btree on id exists	15;16;17;18;19	never	-
 index_fetch_penalty_crossover	index_fetch_penalty_crossover	premise: the table holds all 1000000 rows	15;16;17;18;19	never	-
 index_fetch_penalty_crossover	index_fetch_penalty_crossover	the fetch penalty leaves a clustered ORDER BY on its index	15;16;17;18;19	never	-
+inheritance	inheritance	INHERITS parent includes the child (columnar)	15;16;17;18;19	never	-
+inheritance	inheritance	INHERITS parent includes the child (heap oracle)	15;16;17;18;19	never	-
+inheritance	inheritance	ONLY parent is still just the parent	15;16;17;18;19	never	-
+inheritance	inheritance	SELECT * from an INHERITS parent plans an Append	15;16;17;18;19	never	-
+inheritance	inheritance	a columnar materialized view is still recognised as columnar	15;16;17;18;19	2026-09-24	the predicate narrowed to `relkind == RELKIND_RELATION` instead of RELKIND_HAS_STORAGE -- the obvious wrong fix, which also excludes RELKIND_MATVIEW and stops a columnar materialized view being recognised at all 31 call sites
+inheritance	inheritance	a large child is still visible through the parent	15;16;17;18;19	never	-
+inheritance	inheritance	a partitioned parent still returns every row	15;16;17;18;19	never	-
+inheritance	inheritance	add_projection on the parent is refused rather than aborting the backend	15;16;17;18;19	never	-
+inheritance	inheritance	and it still returns its rows	15;16;17;18;19	never	-
+inheritance	inheritance	and the cluster is still up after it	15;16;17;18;19	never	-
+inheritance	inheritance	and the cluster is still up after that too	15;16;17;18;19	never	-
+inheritance	inheritance	and the partition is still a PgColumnarScan (#436)	15;16;17;18;19	never	-
+inheritance	inheritance	control: add_projection on the LEAF still works	15;16;17;18;19	never	-
+inheritance	inheritance	control: get_storage_id on the LEAF still returns an id	15;16;17;18;19	never	-
+inheritance	inheritance	count(*) from an INHERITS parent plans an Append	15;16;17;18;19	never	-
+inheritance	inheritance	filtered rows match the heap	15;16;17;18;19	never	-
+inheritance	inheritance	get_storage_id on the parent is refused rather than reading storage	15;16;17;18;19	never	-
+inheritance	inheritance	premise: a materialized view is relkind m	15;16;17;18;19	never	-
+inheritance	inheritance	premise: and it carries the columnar access method	15;16;17;18;19	never	-
+inheritance	inheritance	premise: and it is partitioned	15;16;17;18;19	never	-
+inheritance	inheritance	premise: so it has no storage of its own	15;16;17;18;19	never	-
+inheritance	inheritance	premise: the parent carries the columnar access method	15;16;17;18;19	never	-
+inheritance	inheritance	premise: while its leaf is an ordinary relation	15;16;17;18;19	never	-
+inheritance	inheritance	row identities match the heap	15;16;17;18;19	never	-
+inheritance	inheritance	the child is visible through the parent	15;16;17;18;19	never	-
+inheritance	inheritance	ungrouped vector aggregation also sees the child rows	15;16;17;18;19	never	-
 native_chunk_length_bound	native_chunk_length_bound	a sequential scan of the same poisoned chunk is refused (XX001)	15;16;17;18;19	never	-
 native_chunk_length_bound	native_chunk_length_bound	an index fetch of a chunk whose page_length is 2^32 too large is refused (XX001)	15;16;17;18;19	never	-
 native_chunk_length_bound	native_chunk_length_bound	backend survived the sequential refusal	15;16;17;18;19	never	-

test/check_ledger_budget.txt

@@ -15,7 +15,7 @@
 #
 #   Adding a check to a suite that is already covered does not move it, which is
 #   what makes it safe to bound.
-suites_not_covered 248
+suites_not_covered 247
 #
 # checks_never_observed_red -- A CENSUS. NOT a ceiling, and it must not become
 #   one.
@@ -1121,4 +1121,47 @@ suites_not_covered 248
 # was meaningful in the old form where `scan_ms` really could be 0. By the rule
 # used for the input guard one line above -- a condition either gets covered or
 # gets removed -- this one cannot be covered, so it goes.
-checks_never_observed_red 1574
+
+# #1259: 1574 -> 1599, AND suites_not_covered 248 -> 247. `inheritance` enters
+# the ledger with all 26 of its checks, for the same mechanical reason
+# analyze_stats did: recording that the new arm reddens means merging a RUN, and
+# the tool refuses partial ones.
+#
+#     ledger rows   1741 + 26 = 1767
+#     never         1574 + 25 = 1599
+#     dated          167 +  1 =  168
+#     1599 + 168 == 1767
+#
+# ONE ARM IS DATED AND IT IS THE ONE THAT DISCRIMINATES AGAINST THE WRONG FIX.
+# Mutation B narrows the predicate to `relkind == RELKIND_RELATION` -- the
+# obvious fix, and the one a reader reaches for. It reddens EXACTLY `a columnar
+# materialized view is still recognised as columnar`, on all five majors and
+# nothing else. A materialized view CAN be columnar: measured, relkind 'm',
+# relam 'pgcolumnar', rows readable. RELKIND_RELATION would have stopped
+# recognising it at all 31 call sites, silently.
+#
+# MUTATION A -- THE REVERT -- IS A REMOVAL PROOF AND NOT A LEDGER SOURCE, and
+# the reason is worth keeping. Reverting the relkind test restores the crash, so
+# what fails AFTER it depends on when the postmaster finishes restarting:
+#
+#     major 15, 16   0 reds   the arms are skipped; the bug is PG17+
+#     major 17       4 reds
+#     major 18, 19   8 reds   the same 4, plus 4 downstream of the restart
+#
+# Same mutation, same tree, different counts. Dating arms from that would record
+# the matview arms as killed by a crash rather than by a test, so A is reported
+# and not merged. The two arms it reddens on every one of 17/18/19 are `and the
+# cluster is still up after it` and `and the cluster is still up after that too`.
+#
+# AND THOSE TWO ARE WHY THE `is refused` ARMS ARE NOT ENOUGH. Under the revert
+# `get_storage_id` on a parent CRASHES, and a crashed connection also returns
+# non-zero -- so `refused` is satisfied by the bug itself and those arms pass
+# vacuously. The liveness check taken from a NEW backend is what distinguishes a
+# refusal from an abort.
+#
+# THE PG17 FLOOR IS LOAD-BEARING, NOT TIDINESS. PostgreSQL 15 and 16 refuse
+# `PARTITION BY ... USING pgcolumnar` outright, so the fixture never exists and
+# the two `is refused` arms PASS for the wrong reason -- the relation is not
+# there. A vacuous green, caught by the premises, which is what premises are
+# for. Ten arms are skipped by name below PG17 rather than left to pass.
+checks_never_observed_red 1599

test/inheritance.sh

@@ -86,4 +86,110 @@ prt_plan="$(plan "EXPLAIN (COSTS OFF) SELECT * FROM inh_prt;")"
 check "and the partition is still a PgColumnarScan (#436)" \
 	"$(printf '%s' "$prt_plan" | grep -c 'Custom Scan (PgColumnarScan)')" "1"
 
+# ---- a PARTITIONED parent carrying relam=pgcolumnar has no storage (#1259) ----
+#
+# From PostgreSQL 17 a partitioned table may carry relam = pgcolumnar as the
+# default for its future partitions. It has no storage of its own -- relfilenode
+# is 0 -- and PgColumnarIsColumnarRelation used to answer "yes" from relam
+# alone. Every caller that then read storage handed smgropen a zero
+# relfilenumber, which on an ASSERT build aborts the backend and restarts the
+# cluster. Ten entry points were measured reaching it.
+#
+# THE ARMS BELOW ASSERT AN ERROR, NOT A CRASH. On a production build the old
+# behaviour was not an error at all -- get_storage_id returned NULL and stats
+# and sort_status built on it -- so "returns something" is not the property.
+# The property is that the call is REFUSED and the cluster is still up
+# afterwards, which is why each arm is followed by a liveness check rather than
+# trusting the return alone.
+# PG17 IS THE FLOOR AND THE ARMS BELOW ARE VACUOUS WITHOUT IT. PostgreSQL 15 and
+# 16 refuse `PARTITION BY ... USING pgcolumnar` outright --
+# "specifying a table access method is not supported on a partitioned table" --
+# so the fixture never exists, and the two "is refused" arms then PASS for the
+# wrong reason: the call is refused because the relation is not there. That is a
+# vacuous green, which is worse than a red, and the premises below are what
+# caught it. So they are skipped by name rather than left to pass.
+if ! pgc_is_number "${PGC_MAJOR:-}"; then
+	pgc_fail "premise: the server major is known, so the PG17 floor can be applied" \
+		"got [${PGC_MAJOR:-<none>}]"
+fi
+if [ "$PGC_MAJOR" -lt 17 ]; then
+	for _inh_arm in \
+		"premise: the parent carries the columnar access method" \
+		"premise: and it is partitioned" \
+		"premise: so it has no storage of its own" \
+		"premise: while its leaf is an ordinary relation" \
+		"get_storage_id on the parent is refused rather than reading storage" \
+		"and the cluster is still up after it" \
+		"add_projection on the parent is refused rather than aborting the backend" \
+		"and the cluster is still up after that too" \
+		"control: get_storage_id on the LEAF still returns an id" \
+		"control: add_projection on the LEAF still works"; do
+		check_skip "$_inh_arm" \
+			"SKIP  $_inh_arm (a partitioned table cannot carry an access method before PG17; this server is $PGC_MAJOR)" \
+			"partitioned relam needs PG17+"
+	done
+else
+psql_run "DROP TABLE IF EXISTS inh_pp CASCADE;
+          CREATE TABLE inh_pp (a int, b text) PARTITION BY RANGE (a) USING pgcolumnar;
+          CREATE TABLE inh_pp1 PARTITION OF inh_pp FOR VALUES FROM (0) TO (100);
+          INSERT INTO inh_pp SELECT g, 'x'||g FROM generate_series(0,99) g;"
+check "premise: the parent carries the columnar access method" \
+	"$(q "SELECT am.amname FROM pg_class c JOIN pg_am am ON am.oid = c.relam
+	      WHERE c.relname = 'inh_pp'")" "pgcolumnar"
+check "premise: and it is partitioned" \
+	"$(q "SELECT relkind::text FROM pg_class WHERE relname = 'inh_pp'")" "p"
+check "premise: so it has no storage of its own" \
+	"$(q "SELECT relfilenode FROM pg_class WHERE relname = 'inh_pp'")" "0"
+check "premise: while its leaf is an ordinary relation" \
+	"$(q "SELECT relkind::text FROM pg_class WHERE relname = 'inh_pp1'")" "r"
+
+# EACH CALL IS FOLLOWED BY A LIVENESS CHECK taken from a new backend, because a
+# crashed cluster restarts and a later query succeeds -- a delayed check reads
+# "up" for a backend that aborted.
+inh_up() { psql_run "SELECT 1;" >/dev/null 2>&1 && echo up || echo DOWN; }
+
+check "get_storage_id on the parent is refused rather than reading storage" \
+	"$(psql_run "SELECT pgcolumnar.get_storage_id('inh_pp'::regclass);" >/dev/null 2>&1 \
+		&& echo accepted || echo refused)" "refused"
+check "and the cluster is still up after it" "$(inh_up)" "up"
+
+check "add_projection on the parent is refused rather than aborting the backend" \
+	"$(psql_run "SELECT pgcolumnar.add_projection('inh_pp'::regclass, 'p1', ARRAY['a']);" \
+		>/dev/null 2>&1 && echo accepted || echo refused)" "refused"
+check "and the cluster is still up after that too" "$(inh_up)" "up"
+
+# THE CONTROL THAT THE CALLS WORK AT ALL. Without it "refused" is satisfied by a
+# function that refuses everything, and the arms above would pass on a build
+# where nothing works.
+check "control: get_storage_id on the LEAF still returns an id" \
+	"$(psql_run "SELECT pgcolumnar.get_storage_id('inh_pp1'::regclass);" >/dev/null 2>&1 \
+		&& echo accepted || echo refused)" "accepted"
+check "control: add_projection on the LEAF still works" \
+	"$(psql_run "SELECT pgcolumnar.add_projection('inh_pp1'::regclass, 'p1', ARRAY['a']);" \
+		>/dev/null 2>&1 && echo accepted || echo refused)" "accepted"
+fi
+
+# ---- AND A MATERIALIZED VIEW IS STILL COLUMNAR (#1259) ----
+#
+# THIS IS THE ARM THAT DISCRIMINATES AGAINST THE OBVIOUS WRONG FIX. Narrowing
+# the predicate to `relkind == RELKIND_RELATION` also excludes RELKIND_MATVIEW,
+# and a materialized view CAN be columnar -- so that fix would stop recognising
+# one at all 31 call sites, silently and on every path. RELKIND_HAS_STORAGE
+# admits it and excludes the partitioned parent, which is the property the
+# callers actually need.
+psql_run "DROP MATERIALIZED VIEW IF EXISTS inh_mv;
+          DROP TABLE IF EXISTS inh_src;
+          CREATE TABLE inh_src (a int);
+          INSERT INTO inh_src SELECT generate_series(1,10);
+          CREATE MATERIALIZED VIEW inh_mv USING pgcolumnar AS SELECT * FROM inh_src;"
+check "premise: a materialized view is relkind m" \
+	"$(q "SELECT relkind::text FROM pg_class WHERE relname = 'inh_mv'")" "m"
+check "premise: and it carries the columnar access method" \
+	"$(q "SELECT am.amname FROM pg_class c JOIN pg_am am ON am.oid = c.relam
+	      WHERE c.relname = 'inh_mv'")" "pgcolumnar"
+check "a columnar materialized view is still recognised as columnar" \
+	"$(psql_run "SELECT pgcolumnar.get_storage_id('inh_mv'::regclass);" >/dev/null 2>&1 \
+		&& echo accepted || echo refused)" "accepted"
+check "and it still returns its rows" "$(q "SELECT count(*) FROM inh_mv")" "10"
+
 pgc_summary