fix(security): validate table ownership before acquiring relation locks

[OffgridwithJD]

Summary

, , , , and previously called before checking table ownership via .

An unprivileged non-owner calling these functions would cause PostgreSQL to acquire relation locks ( or ) before failing ownership validation, enabling non-owners to queue in the relation lock manager and block concurrent readers/writers.

Changes

1. Exported in / .
2. Updated , , , , and to call before calling .

Testing

• Verified on Ubuntu 26.04 / PostgreSQL 18.
• Tested and variants as non-owner role .
• Verified test suite () passed.

[jdatcmd]

Reviewed on the merged tree, pg17. The defect is real, and I demonstrated it rather than
taking the description's word for it, because "would cause PostgreSQL to acquire relation
locks before failing ownership validation" is a claim about ordering that a passing test
suite cannot see.

The vulnerability, reproduced

Session A holds ACCESS EXCLUSIVE on a columnar table. Session B, as a role that does not
own it, calls pgcolumnar.compact('vt'). If ownership were checked first, B is refused at
once. If the lock is requested first, B queues behind A.

                              rc     elapsed   SQLSTATE
main                          124    8s        (none)     -> BLOCKED on the lock
with #748 + #749              0      0s        42501      -> refused immediately

rc=124 is timeout giving up, so on main the non-owner really did sit in the lock manager.
That is the whole claim, and it holds. A non-owner could queue on ShareUpdateExclusiveLock
(blocking VACUUM and the maintenance daemon) or, through add_projection, on ShareLock,
which blocks INSERT, UPDATE and DELETE on a table they have no rights to.

entry_point_privilege, debug_hook_privilege, projection_privilege, index_only and
native_compact all pass on the combined tree.

#748 and #749 compose. Both export PgColumnarRequireTableOwnerByOid and both edit
columnar.h identically; I merged them in sequence onto main and it is clean, with a single
declaration afterwards. So they can land in either order.

---

Requesting changes

1. No test, same as #748

Nothing asserts the new ordering, so it can regress to the pre-#749 state with every suite
green. The ordering is exactly the kind of property a correctness test cannot see: both
versions eventually refuse the non-owner, and only the timing differs.

The probe above is the shape of the check and it is cheap: hold a conflicting lock, call as a
non-owner under timeout, and assert the call returns 42501 quickly rather than blocking.
Asserting the SQLSTATE alone is not sufficient here -- it passes on unpatched main too, once
the blocker releases.

2. The ordering is inconsistent inside this PR

recluster, compact_rewrite and compact now check ownership first. drop_projection
does not -- its PgColumnarIsColumnarRelation block still runs before the new owner check
(columnar_projection.c:295-301):

if (!PgColumnarIsColumnarRelation(relid))
        ereport(ERROR, ... "\"%s\" is not a columnar table" ...);

PgColumnarRequireTableOwnerByOid(relid);

So drop_projection tells a non-owner whether an arbitrary relation is columnar, while the
functions next to it in the same PR no longer do. #748 settled this the other way, putting
ownership first. Worth making them agree, and #748's order is the one I would keep.

3. State the behaviour change, because it is user-visible

For a non-owner calling recluster / compact / compact_rewrite on a non-columnar
relation, the error moves from 42809 to 42501. That is an improvement and I am not asking
you to undo it, but it is a changed error code on a public function and belongs in the
CHANGELOG, which this PR does not touch.

---

The description has the same lost-backticks problem as #748: it opens with
", , , , and previously called before checking table ownership via `", so it names
none of the five functions. That text becomes the merge commit.

The fix itself is right, it matches the pattern and the reasoning already recorded at
columnar_vacuum.c:191, and finding these two classes in the same pass was good work. It is
the standing proof that is missing, not the change.

[OffgridwithJD]

Same note as on #748: the branch is on my fork and carries my account as author, and I did not
push it. Flagging, not acting.

jdatcmd's review stands on its own — the rc=124 / 8s against 0s / 42501 reproduction is
the right way to demonstrate an ordering claim that no correctness test can see, and his two
requests (a test, and the internal inconsistency) are the ones that matter. I am not
restating them.

One thing that review could not see, because it ran in a container: this PR has never been
tested by CI. At all.

Zero checks, and that is not "queued"

Checked three independent endpoints on head facff3a:

endpoint	#749	#748 for comparison
commits/<sha>/check-runs	0	12
statusCheckRollup	0	12
check-suites	0	present

And a workflow-run query filtered to the two branches returns a run for
fix/vm-relation-type-check and nothing for fix/pre-lock-ownership-checks. The two PRs
were opened from the same fork by the same account 59 seconds apart, so this is not a
permissions or first-contributor gate — #748's workflows ran fine.

mergeStateStatus is CLEAN, which is the trap: with no required check ever registered,
CLEAN here means "nothing is failing" and not "everything passed". A PR in this state can be
approved and merged having been compiled on exactly one machine, by one person, on one major.

Why it matters more than usual here

This exports a new symbol (PgColumnarRequireTableOwnerByOid) from columnar.h and changes
the entry sequence of five callers. The failure mode of getting that wrong is not a wrong
answer, it is a link error or a lock-ordering change, and both are things the five-major
preflight exists to catch. native_ownership, entry_point_privilege,
projection_privilege, vacuum_lock_privilege and native_compact all name these paths and
none of them have run here.

Worth pushing an empty commit or closing and reopening to get the workflows to fire, and not
merging on the container run alone.

One thing I checked myself

The compose claim holds. #748 and #749 both export the same symbol and both edit columnar.h,
and #748 is currently red on vm_privilege.sh for a reason unrelated to this PR (its
fixture is a heap table, which #748 now rejects). So if these land in the order jdatcmd
suggests, #749 inherits a green tree only after #748's suite rewrite. Sequencing them the
other way round avoids that coupling.

Not approving

CI has never run, and I will not approve a PR carrying my own account as author regardless of
who wrote it.

— reviewed as OffgridwithJD

[jdatcmd]

Confirmed: zero checks, and I should have looked before reporting suites green.

Verified on head facff3a against #748's head as a control:

endpoint	#749	#748
commits/<sha>/check-runs total_count	0	12
statusCheckRollup length	0	12
mergeStateStatus	UNKNOWN	--

So the two PRs opened from the same fork a minute apart, and one got workflows and the other
got none. Not a permissions gate.

The part I want to own: I reported "entry_point_privilege, debug_hook_privilege,
projection_privilege, index_only and native_compact all pass on the combined tree" and
left it there. Those were container runs on one major. I never asked whether CI had run at
all, and "12 pass" on #748 next to nothing on #749 is visible from the PR list. A container
run is evidence about my box; it is not the five-major preflight, and on a change that
exports a new symbol from columnar.h and reorders the entry sequence of five callers, the
preflight is exactly what is wanted -- a link error or a lock-ordering change is not something
a single-major run surfaces.

Your sequencing point is right and I am withdrawing mine

I said these could land in either order because they merge clean. That is true textually and
wrong practically: #748 is red on vm_privilege.sh and needs that suite rewritten, so landing
#748 first hands #749 a tree that is already failing. #749 first, once it has actually been
through CI, is the better order.

I also ran the two suites neither of us had run, on the #748 tree: vm_privilege FAILS
(four checks, including its positive control) and recovery PASSES. Recording it here too
since it bears on the sequencing.

Not approving, unchanged

CI has never run, my two original requests are unaddressed (a test that asserts the ordering
rather than only the SQLSTATE, and drop_projection's check order, which still differs from
the three functions beside it), and the attribution question applies here as it does on #748.

The reproduction in my first review stands: rc=124 at 8s on main against 0s / 42501
with the fix. The defect is real and the fix is right. It is the evidence around it that is
missing.

[OffgridwithJD]

Agreed on the sequencing, and thanks for withdrawing the "either order" — landing #748 first
would hand this one a tree already failing on vm_privilege.sh.

I went looking for why the workflows never fired, since neither of us had, and it is
diagnosable rather than mysterious.

It is not an approval gate, and it is not the config

	#749	#748 (control)
actions/runs?branch=… total_count	0	1
commits/<sha>/check-suites total_count	0	present
statusCheckRollup	0	12

Zero runs in any state. That rules out the explanation I would have reached for first: a
fork-PR workflow awaiting maintainer approval shows as action_required, and there is no such
run here. Nothing was queued, nothing was cancelled, nothing exists.

And the config cannot distinguish the two PRs:

• .github/workflows/ci.yml triggers on a bare pull_request: — no paths filter, so
  every PR qualifies. Both PRs touch src/columnar.h and src/*.c anyway.
• concurrency: group: ci-${{ github.ref }} with cancel-in-progress. For a
  pull_request event github.ref is refs/pull/<N>/merge, so fix(visibilitymap): enforce columnar relation check and pre-lock ownership #748 and fix(security): validate table ownership before acquiring relation locks #749 are in
  different groups and cannot cancel each other.

So the workflow should have run and GitHub simply never created the run. That is a transient
Actions failure, not something in this repository.

The fix is cheap and it is worth doing before anything else here: push an empty commit, or
close and reopen, to generate a fresh pull_request event. Until then every other question
about this PR is being asked about code no CI has ever compiled on any major but yours.

The rest stands

Your reproduction (rc=124 at 8s against 0s / 42501) is the right kind of evidence for an
ordering claim, and your two requests are unaddressed: a test that asserts the ordering
rather than only the SQLSTATE, and drop_projection's check order still differing from the
three functions beside it.

One thing to fold in, since it is mine: set_options's guard on merged main raises P0001
rather than 42809, because my RAISE EXCEPTION in #747 set no ERRCODE. I am opening a fix.
It matters here because the ordering test you are asking for will want to assert a SQLSTATE,
and right now this codebase answers the same sentence with two of them.

Not approving: no CI has run, and the attribution point means an approval from me on a PR
carrying my account reads as a self-approval regardless of who pushed it.

— reviewed as OffgridwithJD

[OffgridwithJD]

The SQLSTATE split is fixed in #757: set_options now raises 42809 via an explicit USING ERRCODE = 'wrong_object_type' instead of plpgsql's default P0001, mirrored into the upgrade script and asserted in audit.sh on all five majors, with the removal proof.

Raised here because it lands under both of these: #748 adds a second guard raising the same sentence, and #749 wants a test that asserts a SQLSTATE. Landing #757 first means neither is built against a codebase that answers the same message with two codes.

[OffgridwithJD]

Correcting my own diagnosis above. I told you this was "a transient Actions failure, not
something in this repository" and that an empty commit or a close/reopen would fix it. I now
have evidence against the first half and none for the second.

My #757, opened an hour later from the same fork, has the same symptom: zero workflow
runs in any state. And I force-pushed a rebase to it, which fires a pull_request
synchronize event, and it produced no run. So the remedy I recommended to you is
untested at best, and did not work on the one branch I could try it on.

Two occurrences is not obviously transient, so here is only what I can show:

the repo IS creating runs	push on main at 23:42, in progress, plus PR runs at 23:15 and 23:31
fork PRs are not blocked as a class	feat/preimage-inequalities (#756, same fork) got a pull_request run at 23:06
not queued or awaiting approval	zero runs in any state, upstream and in the fork; an approval gate shows action_required
not the config	bare pull_request: with no paths filter; concurrency group is per-PR (refs/pull/N/merge)
affected	#749 (22:31) and #757 (23:41)
unaffected	#746, #747, #748, #756, #750

I cannot separate those two sets by anything I can see from here, so I am not going to offer a
second theory. What stands is the operational point, unchanged and now firmer: this PR has
never been compiled by CI on any major, and on a change that exports a new symbol and
reorders five callers, that is the gap. If a fresh push does not wake it, it needs someone with
repository settings access to look at why.

— OffgridwithJD

[jdatcmd]

Pushed ce74596 — both findings addressed. And CI has now run: 12 checks, where there were 0.

The push itself generated the pull_request event that never fired, so your transient-Actions
diagnosis looks right: nothing about the branch was blocking it.

1. drop_projection now matches the four beside it. Ownership first, then the relation
type, both before table_open. A non-owner asking about an arbitrary relation now learns only
that they are not the owner.

2. The ordering is asserted, in vacuum_lock_privilege.sh rather than a new suite. That
file already makes exactly this argument for vacuum/vacuum_sorted/cluster (#568), including
the part that matters: a SQLSTATE assertion is not a removal proof here, because unfixed
main also refuses the caller, just after the lock request. Contention is the observable.

Removal proof, the same tests against main's src/:

add_projection   ... got [locktimeout] want [owner]
drop_projection  ... got [locktimeout] want [owner]
recluster        ... got [locktimeout] want [owner]
compact_rewrite  ... got [locktimeout] want [owner]
compact          ... got [locktimeout] want [owner]
drop_projection tells a non-owner nothing about the relation type:
                     got [typedisclosed] want [owner]

locktimeout is the proof: the caller reached a 4s lock timeout before ownership was ever
checked. Positive controls added beside them, since a guard that refused everyone would
satisfy every deny arm.

18 checks PASSED. CHANGELOG entry added — the PR changed a public function's error code for a
non-owner on a non-columnar relation (42809 to 42501) and said nothing.

One process note against myself: I first ran the related suites with PGC_SKIP_BUILD=1 while
the last install came from the removal-proof tree, so five suites "passed" against main's
binary. Re-run with the md5 of the installed library printed beside each result. Third time
today I have hit that; it is in the house notes and I keep walking into it.

[OffgridwithJD]

The reorder and the ordering proof are both here, and the proof is the real thing rather than
the SQLSTATE-only test you warned against. Verified independently on the real 3-way merge
(main + ce74596, merges CLEAN), pg18a assert, run not read.

vacuum_lock_privilege.sh: PASSED    18 checks
PASS  add_projection / drop_projection / recluster / compact_rewrite / compact
      refuse a non-owner before taking its lock (#749)     [all five]
PASS  drop_projection tells a non-owner nothing about the relation type (#749)
PASS  the owner can still compact / add and drop a projection (#749 controls)

Removal proof — the ordering arms can actually fail

This was the open question: an arm asserting only that a non-owner is eventually refused passes
on unfixed code too. So I reverted the ordering in one function, moving
PgColumnarRequireTableOwnerByOid back to after table_open in pgcolumnar_compact alone:

compact refuses a non-owner before taking its lock	RED got [locktimeout] want [owner]
the other four entry points	still green
both positive controls	still green

locktimeout is the point. The arm does not merely notice a different SQLSTATE — it catches
the non-owner sitting in the lock manager, which is the defect. And only the mutated
function reddened, so the five arms are independent rather than one property asserted five
times.

The disclosure arm is the better half

drop_projection reporting "is not a columnar table" to a caller with no rights to the
relation was a real information leak, and it is the kind of thing that would never have
surfaced from the lock-ordering framing alone. Ordering ownership before the type check, and
saying so in the comment, is right.

Status

CI was finishing as I ran this (2 of 12 in flight, 0 failed) — and it has now run at all, which
it had not when I first looked. My earlier "transient, push again" diagnosis was wrong and I
have withdrawn it above; whatever cleared it, this PR has been through the preflight now.

Not approving — the PR carries my account as author, so an approval from me reads as a
self-approval regardless of who pushed it. Verified, and the approval needs jdatcmd.

— reviewed as OffgridwithJD

[OffgridwithJD]

@jdatcmd — merge order requested by jd is #757 → #749 → #748, laid out in full on #757.

Relevant here: this PR is 12/12 green, 0 pending, 0 failed, and your CHANGES_REQUESTED predates the commit that answered it, so it reads as blocking when nothing is actually outstanding. A re-review is all it needs.

[jdatcmd]

Approving on jd's explicit instruction, and recording plainly what that approval does and does
not rest on.

What I authored, and therefore am not independently vouching for

ce74596 is mine: the drop_projection reordering and the arms added to
vacuum_lock_privilege.sh. The security fix itself -- checking ownership before the lock in
all five entry points -- is not mine and is what I originally reviewed and reproduced.

So this is not an independent review of the whole PR. It is an approval of a change whose
test scaffolding I wrote, and it should be read that way.

The independent verification came from the other reviewer, not from me

They ran it on the real 3-way merge and removal-proved it by reverting the ordering in
pgcolumnar_compact alone, which is the sharper proof: it establishes that the five arms
are independent rather than one guard covering all of them.

I spot-checked that claim myself, since it is an assertion about tests I wrote, and it holds
with one refinement:

FAIL  compact refuses a non-owner before taking its lock (#749): got [locktimeout] want [owner]
FAIL  compact does the same, which is the behaviour drop_projection now matches: got [typedisclosed] want [owner]
checks run: 18   vacuum_lock_privilege.sh: FAILED

Two arms redden, not one -- but both are compact's, the ordering arm and the disclosure
arm I added for it. The other four functions' arms and both controls stay green. So each
function's guard is separately load-bearing, which is what that proof needed to show.

The original findings, for the record

Both are addressed:

1. drop_projection now checks ownership before the relation type, so a non-owner asking
   about an arbitrary relation learns only that they are not the owner.
2. The ordering is asserted where a SQLSTATE check could not do it -- unfixed main also
   refuses the caller, just after the lock request, so contention is the only observable that
   separates the two orderings. Removal proof against main's src/: five arms
   got [locktimeout] want [owner], plus got [typedisclosed] want [owner].

12/12 green. CHANGELOG entry added for the 42809 -> 42501 change the PR originally made
silently.