Обсуждение: BUG #17062: Assert failed in RemoveRoleFromObjectPolicy() on DROP OWNED policy applied to duplicate role
BUG #17062: Assert failed in RemoveRoleFromObjectPolicy() on DROP OWNED policy applied to duplicate role
От
PG Bug reporting form
Дата:
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
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
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
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
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
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
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"
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
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index 5d469309ce..abb6c66316 100644
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -18,6 +18,7 @@
#include "access/relation.h"
#include "access/sysattr.h"
#include "access/table.h"
+#include "access/xact.h"
#include "catalog/catalog.h"
#include "catalog/dependency.h"
#include "catalog/indexing.h"
@@ -425,7 +426,6 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
Oid relid;
Relation rel;
ArrayType *policy_roles;
- int num_roles;
Datum roles_datum;
bool attr_isnull;
bool noperm = true;
@@ -481,11 +481,6 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
policy_roles = DatumGetArrayTypePCopy(roles_datum);
- /* We should be removing exactly one entry from the roles array */
- num_roles = ARR_DIMS(policy_roles)[0] - 1;
-
- Assert(num_roles >= 0);
-
/* Must own relation. */
if (pg_class_ownercheck(relid, GetUserId()))
noperm = false; /* user is allowed to modify this policy */
@@ -497,15 +492,13 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
NameStr(((Form_pg_policy) GETSTRUCT(tuple))->polname),
RelationGetRelationName(rel))));
- /*
- * If multiple roles exist on this policy, then remove the one we were
- * asked to and leave the rest.
- */
- if (!noperm && num_roles > 0)
+ /* Skip the rest if we lack permissions. */
+ if (!noperm)
{
int i,
j;
Oid *roles = (Oid *) ARR_DATA_PTR(policy_roles);
+ int num_roles = ARR_DIMS(policy_roles)[0];
Datum *role_oids;
char *qual_value;
Node *qual_expr;
@@ -582,16 +575,22 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
else
with_check_qual = NULL;
- /* Rebuild the roles array to then update the pg_policy tuple with */
+ /*
+ * Rebuild the roles array, without any mentions of the target role.
+ * Ordinarily there'd be exactly one, but we must cope with duplicate
+ * mentions, since CREATE/ALTER POLICY historically have allowed that.
+ */
role_oids = (Datum *) palloc(num_roles * sizeof(Datum));
- for (i = 0, j = 0; i < ARR_DIMS(policy_roles)[0]; i++)
- /* Copy over all of the roles which are not the one being removed */
+ for (i = 0, j = 0; i < num_roles; i++)
+ {
if (roles[i] != roleid)
role_oids[j++] = ObjectIdGetDatum(roles[i]);
+ }
+ num_roles = j;
- /* We should have only removed the one role */
- Assert(j == num_roles);
-
+ /* If no roles remain, we have to drop the policy instead. */
+ if (num_roles > 0)
+ {
/* This is the array for the new tuple */
role_ids = construct_array(role_oids, num_roles, OIDOID,
sizeof(Oid), true, TYPALIGN_INT);
@@ -646,8 +645,15 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
heap_freetuple(new_tuple);
+ /* Make updates visible */
+ CommandCounterIncrement();
+
/* Invalidate Relation Cache */
CacheInvalidateRelcache(rel);
+
+ /* Report that policy should not be dropped */
+ noperm = true;
+ }
}
/* Clean up. */
@@ -657,7 +663,7 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
table_close(pg_policy_rel, RowExclusiveLock);
- return (noperm || num_roles > 0);
+ return noperm;
}
/*
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
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
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index fc27fd013e..71050bf93d 100644
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -404,13 +404,12 @@ RemovePolicyById(Oid policy_id)
/*
* RemoveRoleFromObjectPolicy -
- * remove a role from a policy by its OID. If the role is not a member of
- * the policy then an error is raised. False is returned to indicate that
- * the role could not be removed due to being the only role on the policy
- * and therefore the entire policy should be removed.
+ * remove a role from a policy's applicable-roles list.
*
- * Note that a warning will be thrown and true will be returned on a
- * permission error, as the policy should not be removed in that case.
+ * Returns true if the role was successfully removed from the policy.
+ * Returns false if the role was not removed because it would have left
+ * polroles empty (which is disallowed, though perhaps it should not be).
+ * On false return, the caller should instead drop the policy altogether.
*
* roleid - the oid of the role to remove
* classid - should always be PolicyRelationId
@@ -424,11 +423,15 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
ScanKeyData skey[1];
HeapTuple tuple;
Oid relid;
- Relation rel;
ArrayType *policy_roles;
Datum roles_datum;
+ Oid *roles;
+ int num_roles;
+ Datum *role_oids;
bool attr_isnull;
bool keep_policy = true;
+ int i,
+ j;
Assert(classid == PolicyRelationId);
@@ -451,26 +454,9 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
if (!HeapTupleIsValid(tuple))
elog(ERROR, "could not find tuple for policy %u", policy_id);
- /*
- * Open and exclusive-lock the relation the policy belongs to.
- */
+ /* Identify rel the policy belongs to */
relid = ((Form_pg_policy) GETSTRUCT(tuple))->polrelid;
- rel = relation_open(relid, AccessExclusiveLock);
-
- if (rel->rd_rel->relkind != RELKIND_RELATION &&
- rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table",
- RelationGetRelationName(rel))));
-
- if (!allowSystemTableMods && IsSystemRelation(rel))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied: \"%s\" is a system catalog",
- RelationGetRelationName(rel))));
-
/* Get the current set of roles */
roles_datum = heap_getattr(tuple,
Anum_pg_policy_polroles,
@@ -480,34 +466,31 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
Assert(!attr_isnull);
policy_roles = DatumGetArrayTypePCopy(roles_datum);
+ roles = (Oid *) ARR_DATA_PTR(policy_roles);
+ num_roles = ARR_DIMS(policy_roles)[0];
- /* Must own relation. */
- if (!pg_class_ownercheck(relid, GetUserId()))
- ereport(WARNING,
- (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
- errmsg("role \"%s\" could not be removed from policy \"%s\" on \"%s\"",
- GetUserNameFromId(roleid, false),
- NameStr(((Form_pg_policy) GETSTRUCT(tuple))->polname),
- RelationGetRelationName(rel))));
- else
+ /*
+ * Rebuild the polroles array, without any mentions of the target role.
+ * Ordinarily there'd be exactly one, but we must cope with duplicate
+ * mentions, since CREATE/ALTER POLICY historically have allowed that.
+ */
+ role_oids = (Datum *) palloc(num_roles * sizeof(Datum));
+ for (i = 0, j = 0; i < num_roles; i++)
{
- int i,
- j;
- Oid *roles = (Oid *) ARR_DATA_PTR(policy_roles);
- int num_roles = ARR_DIMS(policy_roles)[0];
- Datum *role_oids;
- char *qual_value;
- Node *qual_expr;
- List *qual_parse_rtable = NIL;
- char *with_check_value;
- Node *with_check_qual;
- List *with_check_parse_rtable = NIL;
+ if (roles[i] != roleid)
+ role_oids[j++] = ObjectIdGetDatum(roles[i]);
+ }
+ num_roles = j;
+
+ /* If any roles remain, update the policy entry. */
+ if (num_roles > 0)
+ {
+ ArrayType *role_ids;
Datum values[Natts_pg_policy];
bool isnull[Natts_pg_policy];
bool replaces[Natts_pg_policy];
- Datum value_datum;
- ArrayType *role_ids;
HeapTuple new_tuple;
+ HeapTuple reltup;
ObjectAddress target;
ObjectAddress myself;
@@ -516,77 +499,6 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
memset(replaces, 0, sizeof(replaces));
memset(isnull, 0, sizeof(isnull));
- /*
- * All of the dependencies will be removed from the policy and then
- * re-added. In order to get them correct, we need to extract out the
- * expressions in the policy and construct a parsestate just enough to
- * build the range table(s) to then pass to recordDependencyOnExpr().
- */
-
- /* Get policy qual, to update dependencies */
- value_datum = heap_getattr(tuple, Anum_pg_policy_polqual,
- RelationGetDescr(pg_policy_rel), &attr_isnull);
- if (!attr_isnull)
- {
- ParseState *qual_pstate;
-
- /* parsestate is built just to build the range table */
- qual_pstate = make_parsestate(NULL);
-
- qual_value = TextDatumGetCString(value_datum);
- qual_expr = stringToNode(qual_value);
-
- /* Add this rel to the parsestate's rangetable, for dependencies */
- (void) addRangeTableEntryForRelation(qual_pstate, rel,
- AccessShareLock,
- NULL, false, false);
-
- qual_parse_rtable = qual_pstate->p_rtable;
- free_parsestate(qual_pstate);
- }
- else
- qual_expr = NULL;
-
- /* Get WITH CHECK qual, to update dependencies */
- value_datum = heap_getattr(tuple, Anum_pg_policy_polwithcheck,
- RelationGetDescr(pg_policy_rel), &attr_isnull);
- if (!attr_isnull)
- {
- ParseState *with_check_pstate;
-
- /* parsestate is built just to build the range table */
- with_check_pstate = make_parsestate(NULL);
-
- with_check_value = TextDatumGetCString(value_datum);
- with_check_qual = stringToNode(with_check_value);
-
- /* Add this rel to the parsestate's rangetable, for dependencies */
- (void) addRangeTableEntryForRelation(with_check_pstate, rel,
- AccessShareLock,
- NULL, false, false);
-
- with_check_parse_rtable = with_check_pstate->p_rtable;
- free_parsestate(with_check_pstate);
- }
- else
- with_check_qual = NULL;
-
- /*
- * Rebuild the roles array, without any mentions of the target role.
- * Ordinarily there'd be exactly one, but we must cope with duplicate
- * mentions, since CREATE/ALTER POLICY historically have allowed that.
- */
- role_oids = (Datum *) palloc(num_roles * sizeof(Datum));
- for (i = 0, j = 0; i < num_roles; i++)
- {
- if (roles[i] != roleid)
- role_oids[j++] = ObjectIdGetDatum(roles[i]);
- }
- num_roles = j;
-
- /* If any roles remain, update the policy entry. */
- if (num_roles > 0)
- {
/* This is the array for the new tuple */
role_ids = construct_array(role_oids, num_roles, OIDOID,
sizeof(Oid), true, TYPALIGN_INT);
@@ -599,33 +511,14 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
values, isnull, replaces);
CatalogTupleUpdate(pg_policy_rel, &new_tuple->t_self, new_tuple);
- /* Remove all old dependencies. */
- deleteDependencyRecordsFor(PolicyRelationId, policy_id, false);
-
- /* Record the new set of dependencies */
- target.classId = RelationRelationId;
- target.objectId = relid;
- target.objectSubId = 0;
+ /* Remove all the old shared dependencies (roles) */
+ deleteSharedDependencyRecordsFor(PolicyRelationId, policy_id, 0);
+ /* Record the new shared dependencies (roles) */
myself.classId = PolicyRelationId;
myself.objectId = policy_id;
myself.objectSubId = 0;
- recordDependencyOn(&myself, &target, DEPENDENCY_AUTO);
-
- if (qual_expr)
- recordDependencyOnExpr(&myself, qual_expr, qual_parse_rtable,
- DEPENDENCY_NORMAL);
-
- if (with_check_qual)
- recordDependencyOnExpr(&myself, with_check_qual,
- with_check_parse_rtable,
- DEPENDENCY_NORMAL);
-
- /* Remove all the old shared dependencies (roles) */
- deleteSharedDependencyRecordsFor(PolicyRelationId, policy_id, 0);
-
- /* Record the new shared dependencies (roles) */
target.classId = AuthIdRelationId;
target.objectSubId = 0;
for (i = 0; i < num_roles; i++)
@@ -644,21 +537,25 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
/* Make updates visible */
CommandCounterIncrement();
- /* Invalidate Relation Cache */
- CacheInvalidateRelcache(rel);
- }
- else
- {
- /* No roles would remain, so drop the policy instead */
- keep_policy = false;
- }
+ /*
+ * Invalidate relcache entry for rel the policy belongs to, to force
+ * redoing any dependent plans. In case of a race condition where the
+ * rel was just dropped, we need do nothing.
+ */
+ reltup = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
+ if (HeapTupleIsValid(reltup))
+ CacheInvalidateRelcacheByTuple(reltup);
+ ReleaseSysCache(reltup);
+ }
+ else
+ {
+ /* No roles would remain, so drop the policy instead. */
+ keep_policy = false;
}
/* Clean up. */
systable_endscan(sscan);
- relation_close(rel, NoLock);
-
table_close(pg_policy_rel, RowExclusiveLock);
return keep_policy;
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