Re: DROP OWNED BY fails to clean out pg_init_privs grants

Daniel Gustafsson <daniel(at)yesql(dot)se> writes:
> On 15 Jun 2024, at 01:46, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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