Re: BUG #17062: Assert failed in RemoveRoleFromObjectPolicy() on DROP OWNED policy applied to duplicate role

On Sun, Jun 20, 2021 at 04:15:08PM -0400, Tom Lane wrote:
> Here's a draft patch that gets rid of all the seems-to-me-unnecessary
> stuff in RemoveRoleFromObjectPolicy().  As I said before, the permission
> check seems misguided, the rebuild of non-shared dependencies is a waste
> of cycles, and the table locking is neither necessary nor well designed.
> In this version, if somebody manages to commit a concurrent table drop,
> policy change, etc, you just get a "tuple concurrently deleted" or
> equivalent failure.  That's much like most other DDL-conflict cases.

Hm.  This version still removes all the shared dependencies and
rebuilds them from scratch for each role.  Is that something you
intended to change?

> (I did not look to see if there's any documentation mentioning the
> existing permission check.  If there's not, I don't think we need any
> doc changes, since this seems to me to be strictly less surprising
> than the old behavior.)

Don't see any.

> There remains the question of whether we could preserve the policy
> in the edge case of deleting the last role by letting its polroles go
> to empty.  But that's clearly not going to be a back-patchable change,
> and I'm not terribly interested in working on it personally.

What would be the advantage of keeping the policy around anyway?  The
code behaves like that since its introduction.  If you decide to
change that, note that the current code would fail to empty
pg_policy.polroles as the array gets manipulated only when num_roles >
0.  So this could leave around policies with roles part of it even
after some DROP OWNED query.

-   if (!allowSystemTableMods && IsSystemRelation(rel))
-       ereport(ERROR,
-               (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-                errmsg("permission denied: \"%s\" is a system catalog",
-                       RelationGetRelationName(rel))));
-
-   if (!allowSystemTableMods && IsSystemRelation(rel))
-       ereport(ERROR,
-               (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-                errmsg("permission denied: \"%s\" is a system
-       catalog",
-                       RelationGetRelationName(rel))));
Right.  This looks like a copy-paste coming from
RangeVarCallbackForPolicy() and this removes the need to lock the
relation of a policy.
--
Michael