Re: DROP OWNED BY fails to clean out pg_init_privs grants

Robert Haas <robertmhaas(at)gmail(dot)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.

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.

The semantics I've implemented on top of that are:

* ALTER OWNER does not touch pg_init_privs entries.

* REASSIGN OWNED replaces pg_init_privs references with the new
role, whether the references are as grantor or grantee.

* DROP OWNED removes pg_init_privs mentions of the doomed role
(as grantor or grantee), removing the pg_init_privs entry
altogether if it goes to zero clauses. (This is what happened
already, but only if if a SHARED_DEPENDENCY_INITACL entry existed.)

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.

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.

* This gets away from the notion that pg_init_privs should be
a historical record of the state that existed at the end of CREATE
EXTENSION. Now, maybe that notion is unworkable anyway, but
I don't want to let go of it before we're sure about that.

A couple of more-minor points for review:

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

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

Comments?

regards, tom lane