> On 15 Jun 2024, at 01:46, Tom Lane <t...@sss.pgh.pa.us> wrote: > > Robert Haas <robertmh...@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
