8000 Remove unnecessary failure cases in RemoveRoleFromObjectPolicy(). · postgres/postgres@f5b780c · GitHub
[go: up one dir, main page]
Skip to content
8000

Commit f5b780c

Browse files
tglsfdctglsfdc
committed
Remove unnecessary failure cases in RemoveRoleFromObjectPolicy().
It's not really necessary for this function to open or lock the relation associated with the pg_policy entry it's modifying. The error checks it's making on the rel are if anything counterproductive (e.g., if we don't want to allow installation of policies on system catalogs, here is not the place to prevent that). In particular, it seems just wrong to insist on an ownership check. That has the net effect of forcing people to use superuser for DROP OWNED BY, which surely is not an effect we want. Also there is no point in rebuilding the dependencies of the policy expressions, which aren't being changed. Lastly, locking the table also seems counterproductive; it's not helping to prevent race conditions, since we failed to re-read the pg_policy row after acquiring the lock. That means that concurrent DDL would likely result in "tuple concurrently updated/deleted" errors; which is the same behavior this code will produce, with less overhead. Per discussion of bug #17062. Back-patch to all supported versions, as the failure cases this eliminates seem just as undesirable in 9.6 as in HEAD. Discussion: https://postgr.es/m/1573181.1624220108@sss.pgh.pa.us
1 parent 4040139 commit f5b780c

File tree

1 file changed

+47
-145
lines changed

1 file changed

+47
-145
lines changed

‎src/backend/commands/policy.c

Lines changed: 47 additions & 145 deletions
Original file line numberDiff line numberDiff line change
@@ -401,13 +401,12 @@ RemovePolicyById(Oid policy_id)
401401

402402
/*
403403
* RemoveRoleFromObjectPolicy -
404-
* remove a role from a policy by its OID. If the role is not a member of
405-
* the policy then an error is raised. False is returned to indicate that
406-
* the role could not be removed due to being the only role on the policy
407-
* and therefore the entire policy should be removed.
404+
* remove a role from a policy's applicable-roles list.
408405
*
409-
* Note that a warning will be thrown and true will be returned on a
410-
* permission error, as the policy should not be removed in that case.
406+
* Returns true if the role was successfully removed from the policy.
407+
* Returns false if the role was not removed because it would have left
408+
* polroles empty (which is disallowed, though perhaps it should not be).
409+
* On false return, the caller should instead drop the policy altogether.
411410
*
412411
* roleid - the oid of the role to remove
413412
* classid - should always be PolicyRelationId
@@ -421,11 +420,15 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
421420
ScanKeyData skey[1];
422421
HeapTuple tuple;
423422
Oid relid;
424-
Relation rel;
425423
ArrayType *policy_roles;
426424
Datum roles_datum;
425+
Oid *roles;
426+
int num_roles;
427+
Datum *role_oids;
427428
bool attr_isnull;
428429
bool keep_policy = true;
430+
int i,
431+
j;
429432

430433
Assert(classid == PolicyRelationId);
431434

@@ -448,26 +451,9 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
448451
if (!HeapTupleIsValid(tuple))
449452
elog(ERROR, "could not find tuple for policy %u", policy_id);
450453

451-
/*
452-
* Open and exclusive-lock the relation the policy belongs to.
453-
*/
454+
/* Identify rel the policy belongs to */
454455
relid = ((Form_pg_policy) GETSTRUCT(tuple))->polrelid;
455456

456-
rel = relation_open(relid, AccessExclusiveLock);
457-
458-
if (rel->rd_rel->relkind != RELKIND_RELATION &&
459-
rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
460-
ereport(ERROR,
461-
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
462-
errmsg("\"%s\" is not a table",
463-
RelationGetRelationName(rel))));
464-
465-
if (!allowSystemTableMods && IsSystemRelation(rel))
466-
ereport(ERROR,
467-
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
468-
errmsg("permission denied: \"%s\" is a system catalog",
469-
RelationGetRelationName(rel))));
470-
471457
/* Get the current set of roles */
472458
roles_datum = heap_getattr(tuple,
473459
Anum_pg_policy_polroles,
@@ -477,34 +463,31 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
477463
Assert(!attr_isnull);
478464

479465
policy_roles = DatumGetArrayTypePCopy(roles_datum);
466+
roles = (Oid *) ARR_DATA_PTR(policy_roles);
467+
num_roles = ARR_DIMS(policy_roles)[0];
480468

481-
/* Must own relation. */
482-
if (!pg_class_ownercheck(relid, GetUserId()))
483-
ereport(WARNING,
484-
(errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
485-
errmsg("role \"%s\" could not be removed from policy \"%s\" on \"%s\"",
486-
GetUserNameFromId(roleid, false),
487-
NameStr(((Form_pg_policy) GETSTRUCT(tuple))->polname),
488-
RelationGetRelationName(rel))));
489-
else
469+
/*
470+
* Rebuild the polroles array, without any mentions of the target role.
471+
* Ordinarily there'd be exactly one, but we must cope with duplicate
472+
* mentions, since CREATE/ALTER POLICY historically have allowed that.
473+
*/
474+
role_oids = (Datum *) palloc(num_roles * sizeof(Datum));
475+
for (i = 0, j = 0; i < num_roles; i++)
476+
{
477+
if (roles[i] != roleid)
478+
role_oids[j++] = ObjectIdGetDatum(roles[i]);
479+
}
480+
num_roles = j;
481+
482+
/* If any roles remain, update the policy entry. */
483+
if (num_roles > 0)
490484
{
491-
int i,
492-
j;
493-
Oid *roles = (Oid *) ARR_DATA_PTR(policy_roles);
494-
int num_roles = ARR_DIMS(policy_roles)[0];
495-
Datum *role_oids;
496-
char *qual_value;
497-
Node *qual_expr;
498-
List *qual_parse_rtable = NIL;
499-
char *with_check_value;
500-
Node *with_check_qual;
501-
List *with_check_parse_rtable = NIL;
485+
ArrayType *role_ids;
502486
Datum values[Natts_pg_policy];
503487
bool isnull[Natts_pg_policy];
504488
bool replaces[Natts_pg_policy];
505-
Datum value_datum;
506-
ArrayType *role_ids;
507489
HeapTuple new_tuple;
490+
HeapTuple reltup;
508491
ObjectAddress target;
509492
ObjectAddress myself;
510493

@@ -513,74 +496,6 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
513496
memset(replaces, 0, sizeof(replaces));
514497
memset(isnull, 0, sizeof(isnull));
515498

516-
/*
517-
* All of the dependencies will be removed from the policy and then
518-
* re-added. In order to get them correct, we need to extract out the
519-
* expressions in the policy and construct a parsestate just enough to
520-
* build the range table(s) to then pass to recordDependencyOnExpr().
521-
*/
522-
523-
/* Get policy qual, to update dependencies */
524-
value_datum = heap_getattr(tuple, Anum_pg_policy_polqual,
525-
RelationGetDescr(pg_policy_rel), &attr_isnull);
526-
if (!attr_isnull)
527-
{
528-
ParseState *qual_pstate;
529-
530-
/* parsestate is built just to build the range table */
531-
qual_pstate = make_parsestate(NULL);
532-
533-
qual_value = TextDatumGetCString(value_datum);
534-
qual_expr = stringToNode(qual_value);
535-
536-
/* Add this rel to the parsestate's rangetable, for dependencies */
537-
addRangeTableEntryForRelation(qual_pstate, rel, NULL, false, false);
538-
539-
qual_parse_rtable = qual_pstate->p_rtable;
540-
free_parsestate(qual_pstate);
541-
}
542-
else
543-
qual_expr = NULL;
544-
545-
/* Get WITH CHECK qual, to update dependencies */
546-
value_datum = heap_getattr(tuple, Anum_pg_policy_polwithcheck,
547-
RelationGetDescr(pg_policy_rel), &attr_isnull);
548-
if (!attr_isnull)
549-
{
550-
ParseState *with_check_pstate;
551-
552-
/* parsestate is built just to build the range table */
553-
with_check_pstate = make_parsestate(NULL);
554-
555-
with_check_value = TextDatumGetCString(value_datum);
556-
with_check_qual = stringToNode(with_check_value);
557-
558-
/* Add this rel to the parsestate's rangetable, for dependencies */
559-
addRangeTableEntryForRelation(with_check_pstate, rel, NULL, false,
560-
false);
561-
562-
with_check_parse_rtable = with_check_pstate->p_rtable;
563-
free_parsestate(with_check_pstate);
564-
}
565-
else
566-
with_check_qual = NULL;
567-
568-
/*
569-
* Rebuild the roles array, without any mentions of the target role.
570-
* Ordinarily there'd be exactly one, but we must cope with duplicate
571-
* mentions, since CREATE/ALTER POLICY historically have allowed that.
572-
*/
573-
role_oids = (Datum *) palloc(num_roles * sizeof(Datum));
574-
for (i = 0, j = 0; i < num_roles; i++)
575-
{
576-
if (roles[i] != roleid)
577-
role_oids[j++] = ObjectIdGetDatum(roles[i]);
578-
}
579-
num_roles = j;
580-
581-
/* If any roles remain, update the policy entry. */
582-
if (num_roles > 0)
583-
{
584499
/* This is the array for the new tuple */
585500
role_ids = construct_array(role_oids, num_roles, OIDOID,
586501
sizeof(Oid), true, 'i');
@@ -593,33 +508,14 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
593508
values, isnull, replaces);
594509
CatalogTupleUpdate(pg_policy_rel, &new_tuple->t_self, new_tuple);
595510

596-
/* Remove all old dependencies. */
597-
deleteDependencyRecordsFor(PolicyRelationId, policy_id, false);
598-
599-
/* Record the new set of dependencies */
600-
target.classId = RelationRelationId;
601-
target.objectId = relid;
602-
target.objectSubId = 0;
511+
/* Remove all the old shared dependencies (roles) */
512+
deleteSharedDependencyRecordsFor(PolicyRelationId, policy_id, 0);
603513

514+
/* Record the new shared dependencies (roles) */
604515
myself.classId = PolicyRelationId;
605516
myself.objectId = policy_id;
606517
myself.objectSubId = 0;
607518

608-
recordDependencyOn(&myself, &target, DEPENDENCY_AUTO);
609-
610-
if (qual_expr)
611-
recordDependencyOnExpr(&myself, qual_expr, qual_parse_rtable,
612-
DEPENDENCY_NORMAL);
613-
614-
if (with_check_qual)
615-
recordDependencyOnExpr(&myself, with_check_qual,
616-
with_check_parse_rtable,
617-
DEPENDENCY_NORMAL);
618-
619-
/* Remove all the old shared dependencies (roles) */
620-
deleteSharedDependencyRecordsFor(PolicyRelationId, policy_id, 0);
621-
622-
/* Record the new shared dependencies (roles) */
623519
target.classId = AuthIdRelationId;
624520
target.objectSubId = 0;
625521
for (i = 0; i < num_roles; i++)
@@ -638,21 +534,27 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
638534
/* Make updates visible */
639535
CommandCounterIncrement();
640536

641-
/* Invalidate Relation Cache */
642-
CacheInvalidateRelcache(rel);
643-
}
644-
else
537+
/*
538+
* Invalidate relcache entry for rel the policy belongs to, to force
539+
* redoing any dependent plans. In case of a race condition where the
540+
* rel was just dropped, we need do nothing.
541+
*/
542+
reltup = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
543+
if (HeapTupleIsValid(reltup))
645544
{
646-
/* No roles would remain, so drop the policy instead */
647-
keep_policy = false;
545+
CacheInvalidateRelcacheByTuple(reltup);
546+
ReleaseSysCache(reltup);
648547
}
649548
}
549+
else
550+
{
551+
/* No roles would remain, so drop the policy instead. */
552+
keep_policy = false;
553+
}
650554

651555
/* Clean up. */
652556
systable_endscan(sscan);
653557

654-
relation_close(rel, NoLock);
655-
656558
heap_close(pg_policy_rel, RowExclusiveLock);
657559

658560
return keep_policy;

0 commit comments

Comments
 (0)
0