fix: a partitioned table carrying relam=pgcolumnar aborts the backend (#1259) - #1261
Conversation
…commandprompt#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: 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, 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, relkind 'm', relam 'pgcolumnar', rows readable. Narrowing to RELKIND_RELATION would have stopped recognising one at all 31 call sites, silently and on every path. The macro admits it and excludes the partitioned parent. ALL 31 CALL SITES WERE CLASSIFIED BEFORE THE CHANGE and none wants the parent. set_options refuses it cleanly and add_projection aborted on it, so the two catalog renames in pgcolumnar_process_utility have nothing to maintain there. (The issue says 32; that counts the definition line as a call site.) TWO MUTATIONS, AND ONLY ONE IS A LEDGER SOURCE: B the predicate narrowed to == RELKIND_RELATION -> reddens EXACTLY `a columnar materialized view is still recognised as columnar`, on all five majors and nothing else A the relkind test reverted entirely -- a removal proof, NOT merged -> 0 reds on 15/16 (skipped), 4 on 17, 8 on 18/19. Same mutation, same tree, different counts: what fails after the abort depends on when the postmaster finishes restarting. Dating arms from that would record the matview arms as killed by a crash rather than by a test. THE LIVENESS ARMS ARE WHY `is refused` IS 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. A liveness check from a NEW backend distinguishes a refusal from an abort. THE PG17 FLOOR IS LOAD-BEARING. PostgreSQL 15 and 16 refuse `PARTITION BY ... USING pgcolumnar`, so the fixture never exists and the two `is refused` arms PASS for the wrong reason. Ten arms are skipped by name below PG17 rather than left green. Caught by the premises. ledger rows 1741 + 26 = 1767 never 1574 + 25 = 1599 dated 167 + 1 = 168 suites_not_covered 248 -> 247 Verified on pg17a and pg18a, rc=0 and zero FAIL rows throughout: inheritance 26, harness_selftest 1198, docs_style 55, alter_am_cleanup 45, fk_referencing 26. Unmutated control green on all five majors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP
jdatcmd
left a comment
jdatcmd
left a comment
There was a problem hiding this comment.
Approving. 15 of 15 green, RELKIND_HAS_STORAGE is the right predicate, and the
matview arm is the one that makes this reviewable rather than plausible.
I verified the load-bearing fact rather than taking it
A columnar materialized view is real, measured here on PostgreSQL 17.10:
CREATE MATERIALIZED VIEW mv USING pgcolumnar AS SELECT * FROM src; -> SELECT 10
mv relkind = [m] relam = [pgcolumnar] relfilenode nonzero = [true]
get_storage_id(mv) = [10000000001]
and the macro, from catalog/pg_class.h:
#define RELKIND_HAS_STORAGE(relkind) \
((relkind) == RELKIND_RELATION || (relkind) == RELKIND_INDEX || \
(relkind) == RELKIND_SEQUENCE || (relkind) == RELKIND_TOASTVALUE || \
(relkind) == RELKIND_MATVIEW)This was my error and the arm is aimed at it. I posted relkind != RELKIND_RELATION on #1250 as the fix; it excludes matviews, and at 31 call
sites that is a silent loss of recognition on every path. I have corrected that
comment. An arm that reddens for exactly the wrong fix somebody actually
proposed is worth more than one aimed at a hypothesis.
The predicate also fixes #1250, measured
I composed #1250's head 03983390 with this change and no per-call-site
guard:
fk_referencing rc=0 26 passed + 0 failed crash signatures: 0
rewrite_storage_oid rc=0 5 passed + 0 failed
Same head without it aborts with the TRAP. So this closes that call site too,
and I have told @linuxhikerpm their guard becomes redundant behind this.
The arms hold the two properties that matter
refused alone would have been satisfied by the bug — a crashed connection also
returns non-zero — and the liveness check from a fresh backend is what separates
them. The leaf controls stop refused being satisfied by a build where nothing
works. Both of those are the difference between an arm and a sentence.
Skipping ten arms by name below PG17 rather than letting them pass is the
right call and the premises are what found it. A fixture that cannot exist makes
"is refused" true for the wrong reason, and that green is worse than a red.
One thing to settle before merging, and it is not a defect in this PR
#1261 and #1262 both rewrite check_ledger.tsv and check_ledger_budget.txt,
so they will conflict, and a ledger conflict cannot be resolved by taking a
side. This PR adds 26 rows and declares never 1599; mine adds 2 and declares
a different number, both computed against c096eee6.
Taking either side of that conflict silently drops the other's rows — I measured
that composition on #1255 and got 1679 one way, 1678 the other with three rows
lost. Whichever of these merges first, the second must rebase and recount from
a run, not re-add arithmetic. Mine is the smaller one, so merge this first and
I will recount against the new main.
Not blocking: your mutation A decision. Reporting 0/4/8 reds across majors from
one mutation, and declining to date arms from a run where an abort decides what
fails next, is the harder and more honest call.
🤖 Generated with Claude Code
`pgcolumnar_import_parquet` checked the server-file role, INSERT and row
security, and never checked that the target has storage. Handed a PARTITIONED
parent carrying relam = pgcolumnar, which has relfilenode 0, it reached
table_slot_create and the insert sink on a relation with no storage:
LOG: server process was terminated by signal 11: Segmentation fault
SIGNAL 11, NOT AN ASSERT, which separates this from #1259's family. Those
compile out to a wrong answer on a production build; this one takes the cluster
down there too. Reproduced on main 00d3529, which already carries #1261.
Found by @OffgridwithJD.
THE PREDICATE IS RELKIND_HAS_STORAGE, NOT "IS IT COLUMNAR", and the first
version of this fix got that wrong. import_parquet accepts a HEAP target on
purpose, and the two functions' own documentation is where that is settled:
import_arrow 'insert rows ... into a COLUMNAR TABLE'
import_parquet 'insert rows ... into A TABLE'
They differ on exactly that word. Guarding on PgColumnarIsColumnarRelation
rejected heap targets with 42809 and reddened three suites that have imported
into heaps all along -- native_parquet_flba, native_parquet_hardening and
native_parquet_units. Being the odd corner is import_parquet's contract, so the
symmetry argument does not reach it: the crash is about STORAGE, and only that
question is this function's to ask. Same macro and same reasoning as #1261 one
level up; it admits heap, columnar and matview and excludes relkind 'p'.
THE REMOVAL PROOF RAN ONE SUITE ALONE AND THE REGRESSION WAS IN THREE OTHERS.
The narrow run could not have found it and the matrix did -- the same lesson
#1262 taught from the other direction. The arms now carry a HEAP control
alongside the columnar one, which is the arm whose absence let the over-broad
guard through.
Removal proof, corrected guard:
guard present parquet_import 25 passed + 0 failed rc=0 signal 11 x0
native_parquet_units 19/19 rc=0
native_parquet_flba 15/15 rc=0
native_parquet_multifile 32/32 rc=0
native_parquet_hardening 18/18 rc=0
guard removed parquet_import 24 passed + 1 failed rc=1 signal 11 x2
native_parquet_units 19/19 rc=0 <- unaffected
AND "IS REFUSED" IS NOT THE PROPERTY. `importing into the partitioned parent is
refused` PASSES on both arms, because a crashed connection also returns
non-zero. Only the liveness check from a fresh backend moves.
That liveness arm was itself wrong first: written with `psql_run`, which runs -q
with no -At and prints nothing, it returned empty on a LIVE cluster and could
never pass -- red against the unfixed build for the wrong reason and still red
against the fix. A discriminator that discriminates nothing, and worse than a
cannot-fail arm here specifically, because a removal proof reads red as success.
It uses `q` now.
THE ARM HAS TO BUILD THE FILE FIRST. A sweep reusing one fixture across entry
points recorded this call as "survived rc=1", because export had been refused on
the parent and the import failed at the OPEN before reaching the crash.
parquet_import is not a ledger-covered suite (0 rows), so the new checks need no
ledger entries.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n
2 participants
Closes #1259.
What
PgColumnarIsColumnarRelationdecided "is this ours?" fromrelamalone. Apartitioned table carrying
relam = pgcolumnarhas no storage —relfilenodeis 0 — so every caller that took the true branch and then read storage handed
smgropena zero relfilenumber:One line, in one predicate. @jdatcmd measured ten entry points reaching it.
RELKIND_HAS_STORAGE, not== RELKIND_RELATIONThis is the part worth reviewing hardest, because the obvious fix is wrong.
A materialized view can be columnar. Measured before choosing the
predicate:
relkind == RELKIND_RELATIONalso excludesRELKIND_MATVIEW, so it would stoprecognising a columnar materialized view at all 31 call sites, silently and
on every path.
RELKIND_HAS_STORAGEadmits it and excludes the partitionedparent, which is the property the callers actually need. It is present on all
five majors (path read from
pg_config --includedir-server, not guessed).An arm drives exactly that wrong fix — see mutation B below.
Every call site was classified first, and none wants the parent
The one site that looked like a genuine exception —
pgcolumnar_process_utilitywalking
find_all_inheritors, which includes the parent — does three things perrelation: one storage rename and two catalog renames a parent could
plausibly own. Probed:
A parent can hold neither row, so those renames have nothing to maintain there.
Full working on #1259.
Two mutations, and only one is a ledger source
A is deliberately not merged into the ledger. Same mutation, same tree,
different counts: what fails after the abort depends on when the postmaster
finishes restarting. Dating arms from that would record the matview arms as
killed by a crash rather than by a test. The two it reddens on every one of
17/18/19 are the liveness arms.
Two design points the arms exist to hold
is refusedis not enough on its own. Under the revert,get_storage_idona parent crashes, and a crashed connection also returns non-zero — so
refusedis satisfied by the bug itself and that arm passes vacuously. Aliveness check from a new backend distinguishes a refusal from an abort.
That is why each call is followed by one.
The PG17 floor is load-bearing, not tidiness. PostgreSQL 15 and 16 refuse
PARTITION BY ... USING pgcolumnaroutright, so the fixture never exists andthe two
is refusedarms pass for the wrong reason — the relation is not there.A vacuous green. Ten arms are skipped by name below PG17 rather than left to
pass, and the premises are what caught it.
Counted
inheritanceenters the ledger with all 26 checks, for the same mechanicalreason
analyze_statsdid: the tool refuses partial runs. Published before thecount and it agrees.
Verified
One thing for @linuxhikerpm and #1250
PgColumnarRetargetStorageRelationis gated onPgColumnarIsColumnarRelation,so this change may remove the need for #1250's per-call-site
relkindguardentirely. I have not tested that — the function does not exist on
main—and #1250 is not mine to change. Worth checking before that guard is carried
forward.
🤖 Generated with Claude Code
https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP