Horton

Thread

Commits

  1. Remove unnecessary failure cases in RemoveRoleFromObjectPolicy().

  2. Fix misbehavior of DROP OWNED BY with duplicate polroles entries.

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

    The Post Office <noreply@postgresql.org> — 2021-06-17T20:33:46Z

    The following bug has been logged on the website:

Bug reference:      17062
Logged by:          Alexander Lakhin
Email address:      exclusion@gmail.com
PostgreSQL version: 14beta1
Operating system:   Ubuntu 20.04
Description:        

When executing the following query:
CREATE USER role1;
CREATE TABLE t1(id int);
CREATE POLICY p1 ON t1 TO role1,role1 USING (true);
DROP OWNED BY role1;

The server halts with the failed assertion:
Core was generated by `postgres: law regression [local] DROP OWNED          
                        '.
Program terminated with signal SIGABRT, Aborted.
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
50      ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007f09b9f31859 in __GI_abort () at abort.c:79
#2  0x0000560bfa59234a in ExceptionalCondition
(conditionName=conditionName@entry=0x560bfa6caf96 "j == num_roles", 
    errorType=errorType@entry=0x560bfa5f100b "FailedAssertion",
fileName=0x7ffdc84f5020 "+#Y\372\vV", 
    fileName@entry=0x560bfa6caf21 "policy.c",
lineNumber=lineNumber@entry=593) at assert.c:69
#3  0x0000560bfa23c83f in RemoveRoleFromObjectPolicy
(roleid=roleid@entry=16385, classid=<optimized out>, 
    policy_id=16389) at policy.c:593
#4  0x0000560bfa1a43ec in shdepDropOwned
(roleids=roleids@entry=0x560bfb3d9878, behavior=DROP_RESTRICT)
    at pg_shdepend.c:1420
#5  0x0000560bfa2849cf in DropOwnedObjects (stmt=stmt@entry=0x560bfb3b8138)
at user.c:1390
#6  0x0000560bfa45e591 in ProcessUtilitySlow
(pstate=pstate@entry=0x560bfb3d9760, pstmt=pstmt@entry=0x560bfb3b8448, 
    queryString=queryString@entry=0x560bfb3b7690 "DROP OWNED BY role1;", 
    context=context@entry=PROCESS_UTILITY_TOPLEVEL, params=params@entry=0x0,
queryEnv=queryEnv@entry=0x0, 
    dest=0x560bfb3b8518, qc=0x7ffdc84f5bb0) at utility.c:1761
#7  0x0000560bfa45d2f0 in standard_ProcessUtility (pstmt=0x560bfb3b8448, 
    queryString=0x560bfb3b7690 "DROP OWNED BY role1;",
context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, 
    dest=0x560bfb3b8518, qc=0x7ffdc84f5bb0) at utility.c:1034
#8  0x0000560bfa45d3cf in ProcessUtility (pstmt=pstmt@entry=0x560bfb3b8448,
queryString=<optimized out>, 
    context=context@entry=PROCESS_UTILITY_TOPLEVEL, params=<optimized out>,
queryEnv=<optimized out>, 
    dest=dest@entry=0x560bfb3b8518, qc=0x7ffdc84f5bb0) at utility.c:525
#9  0x0000560bfa45a91c in PortalRunUtility
(portal=portal@entry=0x560bfb425f70, pstmt=pstmt@entry=0x560bfb3b8448, 
    isTopLevel=isTopLevel@entry=true,
setHoldSnapshot=setHoldSnapshot@entry=false, dest=dest@entry=0x560bfb3b8518,

    qc=qc@entry=0x7ffdc84f5bb0) at pquery.c:1147
#10 0x0000560bfa45ac1b in PortalRunMulti
(portal=portal@entry=0x560bfb425f70, isTopLevel=isTopLevel@entry=true, 
    setHoldSnapshot=setHoldSnapshot@entry=false,
dest=dest@entry=0x560bfb3b8518, altdest=altdest@entry=0x560bfb3b8518, 
    qc=qc@entry=0x7ffdc84f5bb0) at pquery.c:1303
#11 0x0000560bfa45b04f in PortalRun (portal=portal@entry=0x560bfb425f70,
count=count@entry=9223372036854775807, 
    isTopLevel=isTopLevel@entry=true, run_once=run_once@entry=true,
dest=dest@entry=0x560bfb3b8518, 
    altdest=altdest@entry=0x560bfb3b8518, qc=0x7ffdc84f5bb0) at
pquery.c:786
#12 0x0000560bfa4572a5 in exec_simple_query
(query_string=query_string@entry=0x560bfb3b7690 "DROP OWNED BY role1;")
    at postgres.c:1214
#13 0x0000560bfa459277 in PostgresMain (argc=argc@entry=1,
argv=argv@entry=0x7ffdc84f5da0, dbname=<optimized out>, 
    username=<optimized out>) at postgres.c:4486
#14 0x0000560bfa3b41b4 in BackendRun (port=port@entry=0x560bfb3daae0) at
postmaster.c:4507
#15 0x0000560bfa3b73c9 in BackendStartup (port=port@entry=0x560bfb3daae0) at
postmaster.c:4229
#16 0x0000560bfa3b7610 in ServerLoop () at postmaster.c:1745
#17 0x0000560bfa3b8b5d in PostmasterMain (argc=3, argv=<optimized out>) at
postmaster.c:1417
#18 0x0000560bfa2f97a4 in main (argc=3, argv=0x560bfb3b1950) at main.c:209

Without asserts it emits:  ERROR:  tuple already updated by self

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-17T21:15:53Z

    PG Bug reporting form <noreply@postgresql.org> writes:
> When executing the following query:
> CREATE USER role1;
> CREATE TABLE t1(id int);
> CREATE POLICY p1 ON t1 TO role1,role1 USING (true);
> DROP OWNED BY role1;

> The server halts with the failed assertion:

Nice.  Seems to be that way at least as far back as 9.6, too.

			regards, tom lane

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-17T21:32:42Z

    I wroteL
> PG Bug reporting form <noreply@postgresql.org> writes:
>> When executing the following query:
>> CREATE USER role1;
>> CREATE TABLE t1(id int);
>> CREATE POLICY p1 ON t1 TO role1,role1 USING (true);
>> DROP OWNED BY role1;

>> The server halts with the failed assertion:

> Nice.  Seems to be that way at least as far back as 9.6, too.

So the proximate problem is RemoveRoleFromObjectPolicy's unfounded
assumption that there are no duplicate OIDs in a pg_policy.polroles
entry.  But that function has got some other serious problems too:

* Locking.  It acquires lock on the policy's relation only after
it looks up the pg_policy entry.  By that point the entry could
be gone or modified.

* Why is it expensively reconstructing the dependencies of the
policy expressions?  Those aren't going to be changed by this
operation.  AFAICS it ought to be sufficient to remove and
rebuild the policy's shared dependencies.

I wonder whether other operations on policies share either
of these issues.

			regards, tom lane

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-17T21:51:18Z

    I wrote:
> So the proximate problem is RemoveRoleFromObjectPolicy's unfounded
> assumption that there are no duplicate OIDs in a pg_policy.polroles
> entry.  But that function has got some other serious problems too:

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.

			regards, tom lane

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

    Michael Paquier <michael@paquier.xyz> — 2021-06-18T03:34:50Z

    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?  The array build
happens in policy_role_list_to_array() for CREATE/ALTER POLICY, so
that's more than just a CCI.  And I think that making the DROP OWNED
BY code more reliable with drops of the same role makes the handling
of shdepend entries a bit shaky.  Back-branches would need to handle a
multi-role logic, now nobody has complained either about this bug
issue for years.

> * Locking.  It acquires lock on the policy's relation only after
> it looks up the pg_policy entry.  By that point the entry could
> be gone or modified.

Yeah.  The policy could be gone, or its relations, so it is possible
to hit two elog() here.  rename_policy() does not take a lock on the
policy's relation for one.

> * Why is it expensively reconstructing the dependencies of the
> policy expressions?  Those aren't going to be changed by this
> operation.  AFAICS it ought to be sufficient to remove and
> rebuild the policy's shared dependencies.

For the shdepend entries dropped and then recreated from scratch, I
agree that this is a waste.  That would be simple enough to fix if we
make shdepDropDependency() available, and simpler to call it in loop
with the roles dropped.
--
Michael

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-18T18:18:28Z

    Michael Paquier <michael@paquier.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

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

    Stephen Frost <sfrost@snowman.net> — 2021-06-18T18:26:25Z

    Greetings,

On Fri, Jun 18, 2021 at 14:18 Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Michael Paquier <michael@paquier.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.)


I haven’t had a chance to delve into this but as far as the question above
goes- short answer is yes, there was generally an idea that we don’t want
policies just disappearing.  Also- we don’t allow a role to be dropped when
there are GRANT’d privileges, users have to go REVOKE any privileges that
reference the role.

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.


This does sound appealing.

Thanks,

Stephen

>

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-18T18:37:00Z

    Stephen Frost <sfrost@snowman.net> writes:
> I haven’t had a chance to delve into this but as far as the question above
> goes- short answer is yes, there was generally an idea that we don’t want
> policies just disappearing.  Also- we don’t allow a role to be dropped when
> there are GRANT’d privileges, users have to go REVOKE any privileges that
> reference the role.

But shouldn't DROP OWNED BY clean those out for you?  If you've got
the right to get rid of the role, ISTM that that should certainly
include the right to get rid of grants to it.

			regards, tom lane

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

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-06-18T18:39:22Z

    On 2021-Jun-18, Tom Lane wrote:

> Stephen Frost <sfrost@snowman.net> writes:
> > I haven’t had a chance to delve into this but as far as the question above
> > goes- short answer is yes, there was generally an idea that we don’t want
> > policies just disappearing.  Also- we don’t allow a role to be dropped when
> > there are GRANT’d privileges, users have to go REVOKE any privileges that
> > reference the role.
> 
> But shouldn't DROP OWNED BY clean those out for you?  If you've got
> the right to get rid of the role, ISTM that that should certainly
> include the right to get rid of grants to it.

Yeah, that's the intention of DROP OWNED BY.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
"Crear es tan difícil como ser libre" (Elsa Triolet)

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

    Stephen Frost <sfrost@snowman.net> — 2021-06-18T18:45:43Z

    Greetings,

On Fri, Jun 18, 2021 at 14:37 Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Stephen Frost <sfrost@snowman.net> writes:
> > I haven’t had a chance to delve into this but as far as the question
> above
> > goes- short answer is yes, there was generally an idea that we don’t want
> > policies just disappearing.  Also- we don’t allow a role to be dropped
> when
> > there are GRANT’d privileges, users have to go REVOKE any privileges that
> > reference the role.
>
> But shouldn't DROP OWNED BY clean those out for you?  If you've got
> the right to get rid of the role, ISTM that that should certainly
> include the right to get rid of grants to it.


Ah, yes, I misunderstood what was being suggested … ideally it would just
remove the role from the set and not blow away the entire policy though,
but then that gets to the point about a NONE option as you suggested since
you certainly wouldn’t want that policy to suddenly be as if it was
declared for PUBLIC.

Hrmpf. Makes it a bit awkward as you wouldn’t know, afterwards, what role
that policy HAD been for though.  Perhaps just letting it be removed in
such a case is the better option, if it’s the only role remaining.  That
would be in line with the GRANT system- it’s not like you can review what
ACLs a role had been given after a DROP OWNED BY has been run.

Thanks,

Stephen

>

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

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-06-18T19:25:55Z

    On 2021-Jun-18, Stephen Frost wrote:

> > But shouldn't DROP OWNED BY clean those out for you?  If you've got
> > the right to get rid of the role, ISTM that that should certainly
> > include the right to get rid of grants to it.
> 
> Ah, yes, I misunderstood what was being suggested … ideally it would just
> remove the role from the set and not blow away the entire policy though,
> but then that gets to the point about a NONE option as you suggested since
> you certainly wouldn’t want that policy to suddenly be as if it was
> declared for PUBLIC.

Could you just set the policy to be granted to "only the bootstrap
superuser" in that case?  I mean as an implementation path for back
branches; use NONE going forward.  That would make the policy allow
nobody who can't already access the record, instead of falling back to
PUBLIC -- which I agree seems suboptimal security-wise.

> Hrmpf. Makes it a bit awkward as you wouldn’t know, afterwards, what role
> that policy HAD been for though.  Perhaps just letting it be removed in
> such a case is the better option, if it’s the only role remaining.  That
> would be in line with the GRANT system- it’s not like you can review what
> ACLs a role had been given after a DROP OWNED BY has been run.

Yeah, I think if you really wanted to keep track of changes, you would
have an auditing system that records them.  Pity you can't build one
with event triggers (because these don't work for global objects).

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
"No hay hombre que no aspire a la plenitud, es decir,
la suma de experiencias de que un hombre es capaz"

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-18T19:53:14Z

    Here's a stop-the-bleeding patch that just gets rid of the assertion
failure and the tuple-updated-by-self problem.  I think this is
reasonable to slip in for beta2, although the other issues we
mentioned seem to require more thought.

(For ease of review, I've not re-pgindented.)

			regards, tom lane

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-18T20:00:02Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Could you just set the policy to be granted to "only the bootstrap
> superuser" in that case?  I mean as an implementation path for back
> branches; use NONE going forward.  That would make the policy allow
> nobody who can't already access the record, instead of falling back to
> PUBLIC -- which I agree seems suboptimal security-wise.

That doesn't seem like a great solution --- it would produce very
confusing output from pg_dump for instance.  In fact, I think it
breaks pg_dump for cases where the target DB has a different
bootstrap superuser name.

			regards, tom lane

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-20T20:15:08Z

    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.

(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.)

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.

			regards, tom lane

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

    Michael Paquier <michael@paquier.xyz> — 2021-06-22T03:34:52Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-22T15:10:42Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2021-06-23T00:20:31Z

    On Tue, Jun 22, 2021 at 11:10:42AM -0400, Tom Lane wrote:
> 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.

Okay.  There is no urgency in that.  You mentioned it, so I was just
wondering about it.

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

At the same time, nobody has complained about the current behavior
either, and what HEAD does now is the same behavior as one would have
for any objects using SHARED_DEPENDENCY_OWNER which is what DROP OWNED
BY ensures.  So the existing forced deletion seems better kept as-is.
--
Michael