Re: DROP OWNED BY fails to clean out pg_init_privs grants - Mailing list pgsql-hackers

From Tom Lane
Subject Re: DROP OWNED BY fails to clean out pg_init_privs grants
Date
Msg-id 3178658.1718636216@sss.pgh.pa.us
Whole thread Raw
In response to Re: DROP OWNED BY fails to clean out pg_init_privs grants  (Daniel Gustafsson <daniel@yesql.se>)
Responses Re: DROP OWNED BY fails to clean out pg_init_privs grants
List pgsql-hackers
Daniel Gustafsson <daniel@yesql.se> writes:
> On 15 Jun 2024, at 01:46, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The core change is to install SHARED_DEPENDENCY_INITACL entries in
>> pg_shdepend for all references to non-pinned roles in pg_init_privs,
>> whether they are the object's owner or not.  Doing that ensures that
>> we can't drop a role that is still referenced there, and it provides
>> a basis for shdepDropOwned and shdepReassignOwned to take some kind
>> of action for such references.

> I wonder if this will break any tools/scripts in prod which relies on the
> previous (faulty) behaviour.  It will be interesting to see if anything shows
> up on -bugs.  Off the cuff it seems like a good idea judging by where we are
> and what we can fix with it.

Considering that SHARED_DEPENDENCY_INITACL has existed for less than
two months, it's hard to believe that any outside code has grown any
dependencies on it, much less that it couldn't be adjusted readily.

> I wonder if it's worth reverting passing the owner ID for v17 and revisiting
> that in 18 if we work on recording the ID.  Shaving a few catalog lookups is
> generally worthwhile, doing them without needing the result for the next five
> years might bite us.

Yeah, that was the direction I was leaning in, too.  I'll commit the
revert of that separately, so that un-reverting it shouldn't be too
painful if we eventually decide to do so.

> Re-reading 534287403 I wonder about this hunk in RemoveRoleFromInitPriv:

> +       if (!isNull)
> +               old_acl = DatumGetAclPCopy(oldAclDatum);
> +       else
> +               old_acl = NULL;                 /* this case shouldn't happen, probably */

> I wonder if we should Assert() on old_acl being NULL?  I can't imagine a case
> where it should legitimately be that and catching such in development might be
> useful for catching stray bugs?

Hmm, yeah.  I was trying to be agnostic about whether it's okay for a
pg_init_privs ACL to be NULL ... but I can't imagine a real use for
that either, and the new patch does add some code that's effectively
assuming it isn't.  Agreed, let's be uniform about insisting !isNull.

> +1 on going ahead with this patch.  There is more work to do but I agree that
> this about all that makes sense in v17 at this point.

Thanks for reviewing!

            regards, tom lane



pgsql-hackers by date:

Previous
From: Melanie Plageman
Date:
Subject: Re: Show WAL write and fsync stats in pg_stat_io
Next
From: Melanie Plageman
Date:
Subject: Re: Separate HEAP WAL replay logic into its own file