Daniel Gustafsson <dan...@yesql.se> writes: > On 15 Jun 2024, at 01:46, Tom Lane <t...@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