Re: DROP OWNED BY fails to clean out pg_init_privs grants

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Daniel Gustafsson <daniel(at)yesql(dot)se>
Cc: Hannu Krosing <hannuk(at)google(dot)com>, "David G(dot) Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Stephen Frost <sfrost(at)snowman(dot)net>, Andres Freund <andres(at)anarazel(dot)de>, Noah Misch <nmisch(at)google(dot)com>
Subject: Re: DROP OWNED BY fails to clean out pg_init_privs grants
Date: 2024-05-24 14:20:00
Message-ID: 651419.1716560400@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Daniel Gustafsson <daniel(at)yesql(dot)se> writes:
> I had a look, but I didn't beat you to a fix since it's not immediately clear
> to me how this should work for REASSING OWNED (DROP OWNED seems a simpler
> case). Should REASSIGN OWNED alter the rows in pg_shdepend matching init privs
> from SHARED_DEPENDENCY_OWNER to SHARED_DEPENDENCY_INITACL, so that these can be
> mopped up with a later DROP OWNED? Trying this in a POC patch it fails with
> RemoveRoleFromInitPriv not removing the rows, shortcircuiting that for a test
> seems to make it work but is it the right approach?

I've tentatively concluded that I shouldn't have modeled
SHARED_DEPENDENCY_INITACL so closely on SHARED_DEPENDENCY_ACL,
in particular the decision that we don't need such an entry if
there's also SHARED_DEPENDENCY_OWNER. I think one reason we
can get away with omitting a SHARED_DEPENDENCY_ACL entry for the
owner is that the object's normal ACL is part of its primary
catalog row, so it goes away automatically if the object is
dropped. But obviously that's not true for a pg_init_privs
entry. I can see two routes to a solution:

1. Create SHARED_DEPENDENCY_INITACL, if applicable, whether the
role is the object's owner or not. Then, clearing out the
pg_shdepend entry cues us to go delete the pg_init_privs entry.

2. Just always search pg_init_privs for relevant entries
when dropping an object.

I don't especially like #2 on performance grounds, but it has
a lot fewer moving parts than #1. In particular, there's some
handwaving in changeDependencyOnOwner() about why we should
drop SHARED_DEPENDENCY_ACL when changing owner, and I've not
wrapped my head around how those concerns map to INITACL
if we treat it in this different way.

Another point: shdepReassignOwned explicitly does not touch grants
or default ACLs. It feels like the same should be true of
pg_init_privs entries, or at least if not, why not? In that case
there's nothing to be done in shdepReassignOwned (although maybe its
comments should be adjusted to mention this explicitly). The bug is
just that DROP OWNED isn't getting rid of the entries because there's
no INITACL entry to cue it to do so.

Another thing I'm wondering about right now is privileges on global
objects (roles, databases, tablespaces). The fine manual says
"Although an extension script is not prohibited from creating such
objects, if it does so they will not be tracked as part of the
extension". Presumably, that also means that no pg_init_privs
entries are made; but do we do that correctly?

Anyway, -ENOCAFFEINE for the moment. I'll look more later.

regards, tom lane

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2024-05-24 14:39:49 Re: Convert node test compile-time settings into run-time parameters
Previous Message Peter Eisentraut 2024-05-24 14:17:37 Re: Upgrade Debian CI images to Bookworm