Re: DROP OWNED BY fails to clean out pg_init_privs grants

> On 15 Jun 2024, at 01:46, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Robert Haas <robertmhaas@gmail.com> writes:
>> I think the only thing we absolutely have to fix here is the dangling
>> ACL references.
>
> Here's a draft patch that takes care of Hannu's example, and I think
> it fixes other potential dangling-reference scenarios too.
>
> I'm not sure whether I like the direction this is going, but I do
> think we may not be able to do a lot more than this for v17.

Agreed.

> 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.

> I'm not terribly thrilled with this, because it's still possible
> to get into a state where a pg_init_privs entry is based on
> an owning role that's no longer the owner: you just have to use
> ALTER OWNER directly rather than via REASSIGN OWNED.  While
> I don't think the backend has much problem with that, it probably
> will confuse pg_dump to some extent.  However, such cases are
> going to confuse pg_dump anyway for reasons discussed upthread,
> namely that we don't really support dump/restore of extensions
> where not all the objects are owned by the extension owner.
> I'm content to leave that in the pile of unfinished work for now.

+1

> An alternative definition could be that ALTER OWNER also replaces
> old role with new in pg_init_privs entries.  That would reduce
> the surface for confusing pg_dump a little bit, but I don't think
> that I like it better, for two reasons:
>
> * ALTER OWNER would have to probe pg_init_acl for potential
> entries every time, which would be wasted work for most ALTERs.

Unless it would magically fix all the pg_dump problems I'd prefer to avoid
this.

> * As this stands, updateInitAclDependencies() no longer pays any
> attention to its ownerId argument, and in one place I depend on
> that to skip doing a rather expensive lookup of the current object
> owner.  Perhaps we should remove that argument altogether, and
> in consequence simplify some other callers too?  However, that
> would only help much if we were willing to revert 534287403's
> changes to pass the object's owner ID to recordExtensionInitPriv,
> which I'm hesitant to do because I suspect we'll end up wanting
> to record the owner ID explicitly in pg_init_privs entries.
> On the third hand, maybe we'll never do that, so perhaps we should
> revert those changes for now; some of them add nontrivial lookup
> costs.

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.

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?

> * In shdepReassignOwned, I refactored to move the switch on
> sdepForm->classid into a new subroutine.  We could have left
> it where it is, but it would need a couple more tab stops of
> indentation which felt like too much.  It's in the eye of
> the beholder though.

I prefer the new way.

+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.

--
Daniel Gustafsson