fix: set_options must refuse a relation that is not columnar

[OffgridwithJD]

pgcolumnar.set_options accepted any relation, and silently recorded options for it.

What it did

=> CREATE TABLE h_only (id int) USING heap;
=> SELECT pgcolumnar.set_options('h_only', chunk_group_row_limit => 1234);
 set_options
-------------          <- no error, no warning, void
=> SELECT * FROM pgcolumnar.options;
 regclass | chunk_group_row_limit | ...
----------+-----------------------+
 h_only   |                  1234 |

docs/configuration.md documents the argument as "The columnar table to change", and
options are read by the columnar writer, so the row can never be used by a heap table.

Why it is worth an error rather than a comment

The row is not merely useless. It leaks. The object access hook that clears
pgcolumnar.options fires only for columnar relations, so the row outlives the table.
Measured, arm and control on one cluster:

	options row before DROP	after DROP
USING heap (arm)	1	1
USING pgcolumnar (control)	1	0

After the drop, regclass renders as the bare oid (16554), because the reference is
dangling. A later relation that reuses that oid inherits the stale options.

The one workflow this could have broken, and why it does not

"Set options first, convert the table second" is the only sensible reason to call this on a
heap table. ALTER TABLE ... SET ACCESS METHOD pgcolumnar keeps the relation's oid
(measured: 16560 before, 16560 after), so setting the options after the conversion reaches
the same relation. The hint says so:

ERROR:  relation "h_only" is not a columnar table
HINT:   Per-table options are read by the columnar writer and apply only to a table
        using the pgcolumnar access method. Convert it first with
        ALTER TABLE ... SET ACCESS METHOD pgcolumnar, then set the options.

The wording matches what the C paths already raise (columnar_arrow.c:1014,
columnar_parquet.c:1174, and two more).

Nothing in the 215-suite corpus calls set_options on a non-columnar relation, so no
existing test changes behaviour. I checked every call site, including the three suites that
build partitioned tables: all of them target a table created USING pgcolumnar.

The upgrade script

native_upgrade_converge compares md5(pg_get_functiondef(p.oid)), so a body change in the
base script that is not mirrored into 1.0-alpha--1.0-alpha2 makes an upgraded catalog
diverge from a fresh install. The new definition is mirrored, generated from the base script
text so the two are byte-identical (verified: 4,995 bytes each, identical: True).

Removal proof:

mutation	result
drop the CREATE OR REPLACE from the upgrade script	RED — native_upgrade_converge fails on exactly one row, the set_options definition: edcf1ff8... against 62f03e87...

Test

audit.sh section 3 already owns per-table option validation. The new arm sits beside the
existing bounds rejections, with two things the surrounding checks did not have:

• The message is asserted, not just the failure. expect_error passes on any error,
  including a typo in the suite's own SQL, so the error text is captured and matched for
  is not a columnar table. Captured then matched rather than piped, because an erroring
  psql into grep reports on the pipeline.
• A positive control. The same call on a columnar table is asserted to be accepted and
  recorded, so the deny arm cannot pass because set_options rejects everything.

Not in this PR

Rows already orphaned by the old behaviour are left alone. Deleting user rows in an upgrade
script is your call, not mine, and the guard stops new ones. Say the word and I will add it.

Gate

Container pgcolumnar-audit, assert builds.

gate	PG18	PG19
audit	PASS	PASS
native_upgrade_converge	PASS, 5 checks	PASS, 5 checks
extension_upgrade		rc=0
harness_selftest	PASSED, 126 checks
docs_style	PASSED, 9 checks
shellcheck -S error -s bash	clean

The four new assertions, identical on both majors:

PASS  reject set_options on a heap table (rejected)
PASS  the refusal says the relation is not columnar: yes
PASS  nothing was recorded for the heap table: 0
PASS  the same call on a columnar table is recorded: 1

The suite aborted the first time I ran it: audit.sh runs under set -e, and capturing the
error text from a call that is expected to fail takes the non-zero status and ends the
script. expect_error survives only because it sits inside an if. Fixed with || true
and a comment saying why, since the next person to capture an error here will hit it too.

CHANGELOG and docs/configuration.md included.

🤖 Generated with Claude Code

[jdatcmd]

Reviewed on the real merged tree (your base is 93a2eda, main is now 014c2db after
#745), pg17 in my container. The diagnosis is right, the reasoning about the oid-preserving
conversion is right, and I verified the parts I could break rather than the parts I agreed
with. One finding: the guard does not close the leak on PG17 and later.

Verified

claim	result
all four new assertions	PASS, and audit.sh overall PASSED, 0 FAIL
native_upgrade_converge on the merged tree	PASS, 5 checks
base and upgrade definitions are the same text	identical: True (4979 by my extraction, which differs from your 4995 only in where I drew the boundaries)
nothing in the corpus calls set_options on a non-columnar relation	holds

I checked that last one the way you cannot check it from the call sites alone: the four
suites that build partitioned hierarchies (fk_referencing, parallel_copy, native_skip,
parallel_export_parquet) create their parents without USING, and every set_options
target in them is a leaf (nskp_1, nskp_2, t_col, t_tx, t_big, t_wit). So no
existing test changes behaviour, as you said.

I also confirmed the premise your whole argument rests on, that a row for a non-columnar
relation can never be read: the options lookup is a ScanKeyInit on
Anum_options_regclass with plain equality (columnar_metadata.c:2717, :2837, :2887)
and there is no parent walk anywhere. Refusing is therefore right, and it converts a silent
no-op into a diagnosable error.

---

Finding: on PG17+, a partitioned parent passes the guard and leaks exactly as before

The guard tests a.amname = 'pgcolumnar' and nothing else. From PG17, a partitioned
table can carry an access method, so a parent created USING pgcolumnar satisfies it.
Measured on 17.10, merged tree, your guard in place:

-- parent relam=pgcolumnar relkind=p
-- set_options('pp')   rc=0
-- options rows for pp: 1
-- rows keyed to the dropped parent oid (16506) AFTER DROP TABLE: 1

That is the defect this PR exists to fix, reproduced through the front door with the fix
applied: accepted, recorded, and the row outlives the table keyed to a dangling oid.

The mechanism is in the code you already reasoned about. The drop hook returns before it
looks at the access method:

/* columnar_tableam.c:2330 */
if (get_rel_relkind(objectId) != RELKIND_RELATION)
    return;

So it cleans up 'r' and nothing else, while the guard admits any relkind whose relam
matches. A partitioned parent has no storage, the writer only ever writes leaves, and the
hook will never clear its row.

PG16 and earlier cannot reach it -- CREATE TABLE ... PARTITION BY ... USING pgcolumnar
fails outright there, which I checked rather than assumed, so the parent's relam is 0 and
your guard refuses it correctly. That makes this PG17, 18 and 19: three of the five majors
this project supports.

The principled form of the fix is to admit exactly what the cleanup path can clean, so the
two cannot drift:

JOIN pg_am a ON a.oid = c.relam
WHERE c.oid = table_name
  AND a.amname = 'pgcolumnar'
  AND c.relkind = 'r'          -- what the drop hook at columnar_tableam.c:2330 clears

A partitioned parent then gets the same clear error, and the hint is already almost right for
it -- worth a clause saying options are per-partition, since "convert it first" is not the
action a user with a partitioned hierarchy needs.

Two consequences worth stating: the new arm in audit.sh should have a partitioned case
beside the heap one, or this comes back; and the guard has to be mirrored into the upgrade
script again, with native_upgrade_converge re-run.

Not blocking

The CHANGELOG will conflict. #745 and #746 both add entries in ### Fixed; yours is
further down and merges clean against main today, but #746 is ahead of you in the queue and
whichever lands second will need a resolution.

Leaving already-orphaned rows alone is the right call and I would not take you up on the
offer to delete them in an upgrade script. Deleting rows a user could have inserted, on
upgrade, to clean up after a bug, is a bigger decision than the bug.

The || true note is worth keeping. You are right that the next person capturing an
expected error under set -e hits the same thing, and I hit an equivalent trap in this
review: my first probe of the partitioned case borrowed run_pg/$PSQL from audit.sh,
which defines them itself rather than getting them from lib.sh, so it silently produced
empty output and I nearly read "0 rows recorded" as the guard working. Recording it because
it is the same class as the one you documented.

---

Requesting changes on the relkind gap alone. Everything else here is sound and well
measured, and the ALTER TABLE ... SET ACCESS METHOD oid argument is the part that makes
the refusal safe rather than merely strict.

[OffgridwithJD]

Fixed, and this was the right call to block on. My guard converted a silent no-op into a
diagnosable error for the door I looked at, and left the identical leak open on the one I did
not.

Reproduced before fixing

Your report, on my own branch with my own guard in place, PG17.6:

CREATE ... PARTITION BY ... USING pgcolumnar:  ACCEPTED
set_options('pp'):                             ACCEPTED   <- guard admits a partitioned parent
options rows for pp:                           1
options rows for oid 16506 AFTER DROP TABLE:   1          <- leaked
CONTROL, ordinary columnar table, same run:    0          <- cleaned up

And your PG16 claim holds: CREATE TABLE ... PARTITION BY ... USING pgcolumnar is refused
outright on 16.14, so relam is 0 and the guard rejects it correctly. PG17, 18 and 19 only.

Your drop-hook cite is exact — if (get_rel_relkind(objectId) != RELKIND_RELATION) return;
sits above everything that looks at the access method.

The fix is the form you proposed

  AND a.amname = 'pgcolumnar'
  AND c.relkind = 'r'          -- what the drop hook can actually clear

I took the framing with it, because it is the part worth keeping: the guard should admit
exactly what the cleanup can clean, so the two cannot drift. That reasoning is now in the
comment beside it, with the measurement and the file it depends on.

The hint says what a partitioned user should do rather than the conversion advice, which as
you noted was not their action:

HINT: ... A partitioned table has no storage of its own: set the options on each
      partition. Otherwise convert the table first with ALTER TABLE ... SET
      ACCESS METHOD pgcolumnar, then set the options.

Both consequences you named are in

The audit.sh arm has a partitioned case, gated on server_version_num >= 170000 with
an explicit else-branch, so PG15 and PG16 say why it does not apply rather than skipping in
silence. It carries a premise asserting the parent really is p/pgcolumnar on this major,
and a fourth check that the partition itself is still accepted — the guard has to refuse
the parent without refusing the thing that stores rows.

The mirror is regenerated from the base script text: byte-identical, 6,116 bytes each.

gate	PG17	PG18	PG19
audit	PASS	PASS	PASS
native_upgrade_converge	PASS, 5	PASS, 5	PASS, 5

Removal proof, drop relkind and keep everything else, PG17:

PASS  premise: this major really lets a partitioned parent carry the AM: p/pgcolumnar
FAIL  reject set_options on a partitioned parent: statement unexpectedly succeeded
FAIL  nothing was recorded for the partitioned parent: got [1] want [0]

The premise stays green while the arm goes red, which is the shape that says the arm is
measuring rather than passing.

Two traps this cost me, both worth recording next to yours

My premise check was broken and it is what saved the arm. First run:
FAIL premise: this major really lets a partitioned parent carry the AM: got [] want [p/pgcolumnar]. Cause was c.relkind||'/'||a.amname raising operator is not unique: "char" || unknown — relkind is "char" and needs the cast. The deny arm beside it was PASSing
the whole time. Had I not written the premise I would have shipped an arm I could not
distinguish from a vacuous one.

audit.sh builds in-tree with no cross-major clean. Running it for 17 then 18 in one
tree left PG17 objects, and PG18 died at pg_ctl: could not start server before a single
statement ran. I read that as a defect in my own change for a while. A clean tree per major
passes on all three. Not a PR issue — CI gives each major its own builddir — but it is a trap
for anyone gating audit.sh across majors by hand, which is exactly what I was doing.

Agreed on the rest

Leaving already-orphaned rows alone stays. Your reason is better than mine: deleting rows a
user could have inserted, on upgrade, to clean up after a bug, is a bigger decision than the
bug.

And your run_pg/$PSQL trap is the same class as the || true one — audit.sh defines
its own helpers rather than taking them from lib.sh, so borrowing them out of context
produces empty output that reads as a passing result. Worth both of us remembering that this
suite is not a lib.sh suite.

— OffgridwithJD

[OffgridwithJD]

Self-review while waiting on yours. Your relkind finding raised a question I had not asked:
relkind = 'r' also excludes a materialized view, and a matview can carry an access
method. So does this refuse something that works?

It works for reads, and its options are inert

CREATE MATERIALIZED VIEW ... USING pgcolumnar is accepted on 18.4, comes out m/pgcolumnar,
and its rows read back. That much made me think I had removed a capability.

Then I measured whether the writer honours options recorded for one. Options row inserted
straight into the catalog, REFRESH, count the column-0 zone maps over 20,000 rows:

	chunk-group zone maps
matview, chunk_group_row_limit => 1000	3
matview, instance default (10000)	3

Identical, so no effect. But two arms agreeing is a signal, not a result, so I ran the
positive control that decides whether the fixture can show the effect at all — the same
catalog insert on an ordinary columnar table:

	chunk-group zone maps
ordinary table, chunk_group_row_limit => 1000	21
ordinary table, instance default	3

The fixture is live. So the matview null is real: a matview's options are as inert as a heap
table's, and the guard removes nothing that worked. And its row leaks on DROP the same way,
which I checked too (1 row surviving, keyed to the dropped oid).

Refusing it is therefore right for the same reason as the partitioned parent, and by the same
principle you gave me: admit exactly what the cleanup can clean. Recorded in the CHANGELOG,
because it is still a user-visible change for anyone who has a columnar matview.

Nothing in the corpus creates one — no test uses MATERIALIZED VIEW at all, and src/ has no
RELKIND_MATVIEW — so no existing coverage changes.

Unchanged from the last round

audit and native_upgrade_converge PASS on 17, 18 and 19; removal proof still reddens
both directions with the premise green.

— OffgridwithJD

[jdatcmd]

Approving. Re-verified on the merged tree at 62db78a, which already has main in it.

The relkind gap is closed, and the positive controls say it is not closed by refusing

everything

Same probe as my finding, four relkinds in one run on 17.10:

relation	relkind	set_options	rows
partitioned parent	p	refused	0
its leaf partition	r	accepted	1
ordinary columnar table	r	accepted	1
materialized view	m	refused	0

-- rows keyed to the dropped parent oid: 0

The leak I measured last round is gone, and the two r rows are the control that matters: a
guard that had simply become stricter would have refused those too.

The matview question was yours to ask and you answered it correctly

I would have accepted relkind = 'r' without noticing it also excludes a matview. You caught
it, and then did the thing that makes the answer trustworthy: two arms agreeing that options
had no effect is not a result, so you ran the positive control on an ordinary table and got
21 against 3 zone maps. That is what turns "no effect" into evidence rather than a null
from a dead fixture. Same shape as pgc_check_ordered_oracle on #746, and the same reason it
is needed.

Verified

gate	result
audit, PG17	PASSED, all 8 arms incl. the new partitioned ones
audit, PG16	PASSED
native_upgrade_converge, PG16 and PG17	PASSED
base vs upgrade set_options body	identical: True, 6100 bytes, relkind = 'r' present in both

The version gate on the partitioned arm behaves: on PG17 the premise reports p/pgcolumnar
and the three partitioned checks run; on PG16 they are correctly absent, because
CREATE TABLE ... PARTITION BY ... USING pgcolumnar fails outright there, which I confirmed
rather than assumed.

A red I produced and had to clear before reporting it

audit first came back rc=1 on both majors with pg_ctl: could not start server, which
would have read as this PR breaking the suite. It was mine, twice over. Leaked clusters first:
the probes I broke mid-run earlier left three postmasters behind because pgc_finish never
ran. Then, with those cleared, pg17 passed and pg16 still failed -- because I ran two majors
against one tree and audit.sh has its own harness with no equivalent of lib.sh's #536
clean, so pg16 loaded a pg17-built .so. make clean and pg16 passes.

Not a finding against this PR, and I am not asking you to fix audit.sh. Recording it because
the failure mode is indistinguishable from a real red in the log, and the next person who runs
audit.sh across majors in one tree will see it.

Left as agreed

Orphaned rows from the old behaviour stay. Your reasoning holds and I would not want an
upgrade script deleting user rows to clean up after a bug.

Nothing outstanding. With #746 approved as well, whichever of the two lands second will need
the CHANGELOG resolved.