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

Michael Paquier <michael(at)paquier(dot)xyz> writes:
> On Thu, Jun 17, 2021 at 05:51:18PM -0400, Tom Lane wrote:
>> While I'm whining ... that function's permissions checks seem
>> completely out of line too. How is it that, if I have the right
>> to drop some role, I lose that right if the role is mentioned in
>> a policy of some relation I don't own? It feels like this function
>> was written by copy-and-pasting a whole bunch of irrelevant logic.

> Hm. Wouldn't it be better to do something similar to 21378e1f where
> we ensure that there are no duplicated role OIDs in the catalogs to
> begin with, letting the drop code as it is now?

Well, we'd have to rewrite RemoveRoleFromObjectPolicy in the back
branches in any case. Furthermore, I don't think it's a great
idea for this code to just die with an Assert failure if someone
has modified the polroles array by hand. I think we should just
make it clean out however many matches there are.

Stepping back a bit, I suspect that the weird permission rules
here stem from not wanting to allow a policy to just disappear
without the table owner's involvement. There might be a fair
amount of intellectual investment in the USING and WITH CHECK
expressions, so I can sympathize with that point; but I don't
sympathize with allowing it to block an otherwise-allowed role
drop. It seems like we could resolve this tension if we allowed
the polroles array to go to empty rather than requiring the policy
to be dropped when that would happen. The main thing blocking
that is the need to be able to represent that situation in
CREATE/ALTER POLICY. Maybe it'd work to allow "TO NONE" for
that case? (I hasten to add that I'm envisioning that as a future
feature, not something to back-patch. I think in the back branches
we should just silently drop the policy.)

As far as the locking concerns go, I think if we get rid of the
unnecessary reprocessing of the expression dependencies, we don't
have to open or lock the associated table at all. The operation
would reduce to one UPDATE on the pg_policy row (plus maybe some
deletions of pg_shdepend rows), and I think the existing handling
of tuple update conflicts would then be good enough to cope with
concurrent alter/drop of the policy.

regards, tom lane