Michael Paquier <michael@paquier.xyz> writes:
> 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 hadn't intended to mess with that, since the main point of this
patch was just to get rid of unnecessary code. It could be done as a
follow-on micro-optimization, if you like. You'd need to be sure
that the new coding gets rid of duplicate pg_shdepend entries if there
are any.
>> (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.
Thanks for looking!
>> 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.
Just that if you wanted to add some new roles to the same policy,
you wouldn't have to remember the rest of the policy's details.
> 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.
Sure, you'd get rid of that if-test, and for that matter the
function's return convention.
regards, tom lane