Reject dropping columns used by projections

[linuxhikerpm]

Problem

ALTER TABLE ... DROP COLUMN did not know about extension-owned projections. Dropping a projected sort key succeeded, left its attnum pointing at a dropped pg_attribute row, and made the next insert fail in typcache with type with OID 0 does not exist.

Fix

Before core executes DROP COLUMN, inspect materialized projections on the table and its inheritance/partition descendants. Refuse a dependent drop with SQLSTATE 2BP01 and a direct pgcolumnar.drop_projection() recovery hint. Locks serialize the check against concurrent projection creation until core upgrades to its ALTER lock.

Verification

• New regression covers a direct table, post-refusal writes/fan-out, unrelated column drops, and a projection on a columnar partition reached through parent DDL.
• Relevant suites: projections, inheritance, fk_referencing.
• Warning-free build.
• Full PG18 matrix: 243 passed, 0 failed, 0 incomplete; 2 PG19-only skips (native_repack, pg19_vacuum_options).

Made with Cursor

[OffgridwithJD]

Reviewed at 6bbeb1e on pgcolumnar-audit, PG 18.4 assert build (/usr/local/pg18a). I reproduced
your motivating defect on unfixed main, ran your suite on both trees, and then probed the refusal
with five fixtures it does not have.

The defect is real, the fix works, and four of my five probes came back clean. One did not, and it
is the reason I am requesting changes: a user with no rights to the table can tell which of its
columns a projection covers.

Your premise reproduces exactly

On main 9628414, .so 3e37c0af7b85, fix present = 0 asserted at source:

DROP COLUMN sort_key   -> accepted, column gone
next INSERT            -> ERROR:  type with OID 0 does not exist
rows                   -> stuck at 1000, table unwritable

Word for word what your header describes.

Your suite, same fixtures, two trees:

tree	.so	result
main 9628414	3e37c0af7b85	0 passed, 6 failed
your 6bbeb1e	7d4257a1f330	6 passed, 0 failed

What holds under probing

probe	result
DROP COLUMN IF EXISTS <projected>	refused, 2BP01 — the IF EXISTS spelling does not slip past
DROP COLUMN IF EXISTS <missing>	no error — correctly not your business
ADD COLUMN z, DROP COLUMN <projected> in one ALTER	refused, and the ADD did not leak: whole statement rolled back
ALTER TABLE ip DROP COLUMN sk on an inheritance child (not a partition)	refused, 2BP01, child column preserved
ALTER TABLE ONLY pp DROP COLUMN sk	42P16 from core — the !inh branch is unreachable for a partitioned parent, which is fine

And the invariant your check leans on is enforced: add_projection with a sort_key outside
columns raises 22023, so testing columns alone really is sufficient. That is worth one line in
the comment, because the sufficiency is not local to the code that relies on it.

The finding: the refusal answers before core checks permissions

pgcolumnar_reject_drop_projected_column runs in the pre-statement half of the hook, before
standard_ProcessUtility, so it reports on a table the caller may have no rights to. Measured, as
role nobody (LOGIN, no grants), same table, two columns:

non-owner DROP of a PROJECTED   column -> 2BP01   (your dependency error)
non-owner DROP of an UNPROJECTED column -> 42501   (core's permission denied)

The error code differs by whether a projection covers the column, so a stranger can enumerate the
projected columns of a table they cannot read, one statement at a time. It also means such a caller
takes ShareUpdateExclusiveLock on that table before any privilege check — held to end of
transaction by design, per your comment — which is a lock a non-owner should probably not be able to
take at will.

This repo already treats that boundary as load-bearing: there is a projection_privilege suite, and
#562 revoked EXECUTE from PUBLIC on exactly this metadata precisely so the C check is not the only
barrier. The disclosure is modest and I am not calling it a vulnerability. It is a regression in a
boundary the project has already decided to defend, it has no arm, and it is much cheaper to fix now
than after release.

Two shapes of fix, your call: check ownership before reporting (pg_class_ownercheck /
PgColumnarRequireTableOwnerByOid, which add_projection already uses), or move the check to where
core has already decided the caller may proceed. The second is a bigger change and I would not ask
for it.

One comment that is factually wrong, and it is the dangerous kind

 * Serialize against add_projection(), which takes ShareUpdateExclusiveLock,

pgcolumnar_add_projection takes ShareLock — columnar_projection.c:198, with its own comment
explaining the choice as "the same lock non-concurrent CREATE INDEX takes". Your conclusion is still
right, because ShareUpdateExclusiveLock and ShareLock do conflict, so the serialization you want
does happen. But the stated reason is wrong, and a later reader who trusts it could "simplify" your
lock level against a premise that was never true. Naming the real conflict pair is the fix.

A scope note, not a defect

Your rationale is the crash, and the refusal is broader than the crash. Measured on main:

drop the projection's SORT KEY column      -> next INSERT dies, type with OID 0
drop a projected NON-sort-key column       -> DROP and INSERT both SUCCEED; the projection
                                              silently reads empty

I think refusing both is the right call — a projection reading empty is worse than a loud refusal —
but the header explains only the first, and the second is the case a reader is more likely to hit.
Worth a sentence so nobody later narrows the check to sort keys on the strength of the rationale.

For completeness, one thing I checked and am not reporting: after DROP COLUMN, the base
projection's columns still names the dropped attnum ({1,2,3} with payload gone), and you skip
projection_id 0 deliberately. I could not demonstrate any consequence — read_projection(t,'base')
raises 42704 with no drop at all, so that is by design and not a symptom. Mentioning it only so
the next reviewer does not spend the same hour on it.

Also: your CI has never run

Both your PRs sit at completed/action_required. You are a first-time contributor pushing from
linuxhikerpm/pgcolumnar with read permission, so GitHub is holding the workflows for a maintainer.
gh pr checks says "no checks reported", which reads like "queued" and is not.
@jdatcmd — both #888 and #891 need that click before either has a gate.

Requesting changes for the privilege ordering and the two comment corrections. The fix itself is
sound, the tests are honest, and your partition arm is better than most in this tree. Once the
ownership check is in and CI is green I expect to approve.

One heads-up, since we are in the same function: I am implementing #887, which adds a post-statement
block to pgcolumnar_process_utility for AlterTableStmt and TruncateStmt. It will conflict with
your hunk at :2588 textually but not semantically — yours refuses before the statement, mine repairs
after it. Whoever lands second rebases; I am happy for that to be me.

[jdatcmd]

CI is now released on this PR. The workflow run has moved from action_required to queued
and will produce a real result.

Worth stating plainly for the record, because the failure mode here is quiet: this PR had no
gate at all until now, and the way that presents is misleading. gh pr checks reported

no checks reported on the '<branch>' branch

which reads like "not configured" or "queued" and actually meant "held pending a maintainer's
approval". A fork PR from an account with read permission does not run workflows until someone
with write releases them, and it must be released again on every push. So if you push a new
commit here, the gate goes back to held and needs another approval — ping @jdatcmd rather than
reading the absence of checks as a pass.

For what it is worth on the safety question, since running a contributor's code is the reason the
hold exists: .github/workflows/ci.yml triggers on pull_request rather than
pull_request_target, declares permissions: contents: read, references no secrets anywhere, and
uses only GitHub-hosted runners. So these runs execute in an ephemeral VM with a read-only token
and nothing to reach. That is why approving was routine rather than a judgement call about the
contributor.

[linuxhikerpm]

Addressed the requested changes in 6718048:

• check table ownership before taking the retained lock or reading projection metadata, then recheck after locking to cover an ALTER OWNER race;
• added the adversarial role regression: projected and unprojected DROP attempts now both return 42501 to a non-owner;
• corrected the lock comment: add_projection() takes ShareLock, which conflicts with this path's ShareUpdateExclusiveLock;
• documented why scanning columns covers sort keys and why non-sort projected columns must also be refused.

The updated regression and projection_privilege pass. An uncontaminated full PG18 matrix reports 243 passed, 0 failed, 0 incomplete, with the two PG19-only skips.

@OffgridwithJD please re-review when convenient. @jdatcmd this push will need CI released again.

[OffgridwithJD]

Re-reviewed at 6718048. The leak is closed and I measured it rather than reading the diff. My
blocking finding is resolved.

I re-ran the same probe from the first review, unchanged, against your new tree
(.so 08622af8ad84):

                                          before (6bbeb1ee)   now (6718048)
non-owner DROP of a PROJECTED   column          2BP01             42501
non-owner DROP of an UNPROJECTED column         42501             42501

Identical codes for both column kinds, so the error no longer distinguishes a projected column from
an unprojected one and a stranger learns nothing about the projection set. Your two new arms assert
exactly that, and they pass.

The fix did not cost the feature, which was the thing I most wanted to check: an over-eager
ownership check could easily have made the refusal stop firing. All four of my other probes still
behave, on your new tree:

probe	result
owner, DROP COLUMN IF EXISTS <projected>	refused, 2BP01
owner, DROP COLUMN IF EXISTS <missing>	no error, correctly not your business
owner, ADD COLUMN z, DROP COLUMN <projected>	refused, and the ADD did not leak
owner, ALTER TABLE ip DROP COLUMN sk on an inheritance child	refused, 2BP01, child column preserved
ALTER TABLE ONLY pp DROP COLUMN sk	42P16 from core, as before

Your suite is 8 for 8, and the two arms I asked about last time still pass.

Two details I want to credit, because both are better than what I asked for:

You check ownership twice, at :2514 before taking the lock and again at :2526 after. I asked
only for a check. The recheck closes a window I had not named: ALTER TABLE ... OWNER TO could
commit between the first test and the lock, and without the second test the caller would then be
acting on a table they no longer own. Checking before the lock is also the right order for the
disclosure itself, since it means a non-owner never reaches the metadata read at all.

The lock comment now names the real conflict: "add_projection() takes ShareLock, which conflicts
with our ShareUpdateExclusiveLock". That is the correct pair. The old wording asserted
add_projection took ShareUpdateExclusiveLock, and a later reader trusting it could have
"simplified" the lock level against a premise that was never true.

Where this leaves the PR

Your CI is held again. Run 34289398470 on 6718048 is completed/action_required, while the
run on your previous head, 34283785885, finished green. Releasing it is a decision a person makes
each time a fork PR from a read-permission account is pushed, not a formality, so it may take a
little while. I have asked @jdatcmd.

I am leaving my earlier CHANGES_REQUESTED standing only because this repository does not dismiss
stale reviews and I would rather not imply a green I have not seen. To be unambiguous: nothing in
the code is blocking from my side any more. The remaining gap is a green full CI, and I have only
run PG18 locally. When CI is green I will approve.

One coordination note, unchanged from last time. My #892 adds a post-statement block to
pgcolumnar_process_utility for AlterTableStmt and TruncateStmt, so it conflicts textually with
your hunk in that function and not semantically: yours refuses before the statement, mine repairs
after it. Whoever lands second rebases, and I am content for that to be me.

[OffgridwithJD]

One more pass at 6718048, on the question an added privilege check most often gets wrong: does it
reject someone who legitimately may run the statement?

Four callers, each on its own table, each with the fixture's premise asserted before the arm ran:

caller                        want    got
owner                         2BP01   2BP01
superuser, NOT the owner      2BP01   2BP01
stranger with no grants       42501   42501
member of the owning ROLE     2BP01   2BP01

The column survives all four refusals. So a superuser reaches the dependency refusal rather than
being told they lack permission, and ownership held through role membership counts as ownership,
which is what pg_class_ownercheck semantics should give and what a hand-rolled comparison against
relowner would have got wrong. No false positives on either case.

Worth recording how the first version of this probe failed, because it is a trap for anyone extending
your suite. I built the fixture as the target role, and add_projection silently did nothing:
#562 revoked EXECUTE on the projection functions from PUBLIC, so a plain role cannot declare a
projection. Every arm then measured a table with no projection to protect, the DROP succeeded, and the
codes I got back were 42703 and no error rather than anything meaningful. The premise check caught
it. If you add privilege arms, build the fixture as superuser and hand the table over with
ALTER TABLE ... OWNER TO, and assert the projection row exists before trusting the arm.

I have nothing further on the code. The only thing standing between this and my approval is CI,
which is held again on this head: run 34289398470 is completed/action_required. Releasing it is a
decision @jdatcmd's side makes per push rather than a formality, so it may take a little while. I have
asked.

I have approved #888, which is green at 12 checks. Once yours is released and green I will approve
this one the same way.

[OffgridwithJD]

Status, since #888 landed and moved main underneath this PR.

GitHub currently shows this PR's mergeability as UNKNOWN. That is GitHub recomputing after the
base moved, not a problem with your branch. I merged it locally to answer it now:

main d22d006 (carries #888)  +  #891 6718048   ->  MERGES CLEANLY
   Auto-merging src/columnar_tableam.c
   Auto-merging test/run_all_versions.sh

Both changes touch pgcolumnar_process_utility and git resolved them without help, because they sit
in different halves of it: #888 adds a call inside the rename block, yours adds the refusal to the
pre-statement block.

And the merged tree is green both ways, which is the part worth having:

projection_drop_column.sh     8 passed, 0 failed   (yours, on the merged tree)
projection_rename_restore.sh  8 passed, 0 failed   (#888's, on the merged tree)

So nothing about #888 landing has cost you anything, and you do not need to rebase.

What is actually blocking

Only CI, and it has been held for about an hour. Run 34289398470 on 6718048 is still
completed/action_required. A fork PR from a read-permission account needs the workflows released
per push, and that is a decision a person makes each time rather than a formality, so please read the
delay as a queue rather than as anything about your code. I have asked again.

This PR is also still marked draft, which is the same thing that briefly held up #888 after I
approved it: a draft cannot be merged even with an approval and a green gate. Worth marking it ready
whenever you are happy with it, so it is not the next thing in the way.

Where my review stands

My CHANGES_REQUESTED is still the recorded state, and I want to be unambiguous that it no longer
reflects an outstanding code problem. Everything I raised is addressed and verified:

• the non-owner disclosure is closed, 42501 for both projected and unprojected columns
• superusers and members of the owning role still reach the dependency refusal, 2BP01, so the new
  ownership check has no false positives on either
• the lock comment names the real conflict pair
• your suite is 8 for 8, on your branch and on the merged tree

I am leaving the review state as it is only because this repository does not dismiss stale reviews
and I will not post an approval against a gate I have not seen. The moment CI is released and green I
will approve, the same way I did on #888.

[OffgridwithJD]

Approving at 6718048. CI is 12 for 12, which was the only thing left, and this also clears the
CHANGES_REQUESTED I submitted at 6bbeb1ee. Sorry that stood as long as it did: it was accurate when
I filed it and stale from the moment you addressed it, and a stale review decision reads to everyone
else as a reviewer still asking for something.

Everything I raised is fixed and measured rather than read:

The disclosure is closed. Same probe as the first review, unchanged, on your tree:

                                          before (6bbeb1ee)   now (6718048)
non-owner DROP of a PROJECTED   column          2BP01             42501
non-owner DROP of an UNPROJECTED column         42501             42501

And the fix did not cost the feature, which is what an over-eager ownership check usually does.
Four callers, each on its own table, each with the fixture's premise asserted first:

caller	want	got
owner	2BP01	2BP01
superuser, not the owner	2BP01	2BP01
stranger with no grants	42501	42501
member of the owning ROLE	2BP01	2BP01

The column survives all four refusals. Superusers and role members reach the dependency refusal
rather than being told they lack permission, which is what pg_class_ownercheck semantics should give
and what a hand-rolled comparison against relowner would have got wrong.

Checking ownership twice is better than what I asked for. I asked for a check; the recheck after
the lock closes a window I had not named, where ALTER TABLE ... OWNER TO commits between the first
test and the lock. And testing before the lock is the right order for the disclosure itself, since a
non-owner never reaches the metadata read.

The lock comment now names the real conflict pair, add_projection's ShareLock against your
ShareUpdateExclusiveLock. The old wording was the dangerous kind of wrong: right conclusion, false
premise, and a later reader could have "simplified" the lock level on the strength of it.

Your suite is 8 for 8, on your branch and on the tree composed with main. I merged the two locally
rather than trusting GitHub's mergeability, which read UNKNOWN after #888 landed:

main d22d006 + #891 6718048  ->  MERGES CLEANLY
  projection_drop_column.sh     8 passed, 0 failed   (yours)
  projection_rename_restore.sh  8 passed, 0 failed   (#888's)

One thing left and it is not mine: this PR is still marked draft. @jdatcmd cannot merge it in that
state even with an approval and a green gate, which is exactly what briefly held up #888. Mark it
ready when you are happy.

Thank you for the privilege fix in particular. The disclosure was modest and easy to argue away, and
you closed it properly instead.