Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commitfea89d6

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

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
ScanKeyDataskey[1];
424423
HeapTupletuple;
425424
Oidrelid;
426-
Relationrel;
427425
ArrayType*policy_roles;
428426
Datumroles_datum;
427+
Oid*roles;
428+
intnum_roles;
429+
Datum*role_oids;
429430
boolattr_isnull;
430431
boolkeep_policy= true;
432+
inti,
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-
inti,
494-
j;
495-
Oid*roles= (Oid*)ARR_DATA_PTR(policy_roles);
496-
intnum_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
Datumvalues[Natts_pg_policy];
505489
boolisnull[Natts_pg_policy];
506490
boolreplaces[Natts_pg_policy];
507-
Datumvalue_datum;
508-
ArrayType*role_ids;
509491
HeapTuplenew_tuple;
492+
HeapTuplereltup;
510493
ObjectAddresstarget;
511494
ObjectAddressmyself;
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

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
returnkeep_policy;

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp