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

Commit fea89d6

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 c399836 commit fea89d6

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
@@ -403,13 +403,12 @@ RemovePolicyById(Oid policy_id)
403403

404404
/*
405405
* RemoveRoleFromObjectPolicy -
406-
* remove a role from a policy by its OID. If the role is not a member of
407-
* the policy then an error is raised. False is returned to indicate that
408-
* the role could not be removed due to being the only role on the policy
409-
* and therefore the entire policy should be removed.
406+
* remove a role from a policy's applicable-roles list.
410407
*
411-
* Note that a warning will be thrown and true will be returned on a
412-
* permission error, as the policy should not be removed in that case.
408+
* Returns true if the role was successfully removed from the policy.
409+
* Returns false if the role was not removed because it would have left
410+
* polroles empty (which is disallowed, though perhaps it should not be).
411+
* On false return, the caller should instead drop the policy altogether.
413412
*
414413
* roleid - the oid of the role to remove
415414
* classid - should always be PolicyRelationId
@@ -423,11 +422,15 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
423422
ScanKeyData skey[1];
424423
HeapTuple tuple;
425424
Oid relid;
426-
Relation rel;
427425
ArrayType *policy_roles;
428426
Datum roles_datum;
427+
Oid *roles;
428+
int num_roles;
429+
Datum *role_oids;
429430
bool attr_isnull;
430431
bool keep_policy = true;
432+
int i,
433+
j;
431434

432435
Assert(classid == PolicyRelationId);
433436

@@ -450,26 +453,9 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
450453
if (!HeapTupleIsValid(tuple))
451454
elog(ERROR, "could not find tuple for policy %u", policy_id);
452455

453-
/*
454-
* Open and exclusive-lock the relation the policy belongs to.
455-
*/
456+
/* Identify rel the policy belongs to */
456457
relid = ((Form_pg_policy) GETSTRUCT(tuple))->polrelid;
457458

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

481467
policy_roles = DatumGetArrayTypePCopy(roles_datum);
468+
roles = (Oid *) ARR_DATA_PTR(policy_roles);
469+
num_roles = ARR_DIMS(policy_roles)[0];
482470

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

@@ -515,74 +498,6 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
515498
memset(replaces, 0, sizeof(replaces));
516499
memset(isnull, 0, sizeof(isnull));
517500

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

598-
/* Remove all old dependencies. */
599-
deleteDependencyRecordsFor(PolicyRelationId, policy_id, false);
600-
601-
/* Record the new set of dependencies */
602-
target.classId = RelationRelationId;
603-
target.objectId = relid;
604-
target.objectSubId = 0;
513+
/* Remove all the old shared dependencies (roles) */
514+
deleteSharedDependencyRecordsFor(PolicyRelationId, policy_id, 0);
605515

516+
/* Record the new shared dependencies (roles) */
606517
myself.classId = PolicyRelationId;
607518
myself.objectId = policy_id;
608519
myself.objectSubId = 0;
609520

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

643-
/* Invalidate Relation Cache */
644-
CacheInvalidateRelcache(rel);
645-
}
646-
else
539+
/*
540+
* Invalidate relcache entry for rel the policy belongs to, to force
541+
* redoing any dependent plans. In case of a race condition where the
542+
* rel was just dropped, we need do nothing.
543+
*/
544+
reltup = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
545+
if (HeapTupleIsValid(reltup))
647546
{
648-
/* No roles would remain, so drop the policy instead */
649-
keep_policy = false;
547+
CacheInvalidateRelcacheByTuple(reltup);
548+
ReleaseSysCache(reltup);
650549
}
651550
}
551+
else
552+
{
553+
/* No roles would remain, so drop the policy instead. */
554+
keep_policy = false;
555+
}
652556

653557
/* Clean up. */
654558
systable_endscan(sscan);
655559

656-
relation_close(rel, NoLock);
657-
658560
heap_close(pg_policy_rel, RowExclusiveLock);
659561

660562
return keep_policy;

0 commit comments

Comments
 (0)
0