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

Commit5a0f1c8

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 parent8a80562 commit5a0f1c8

File tree

1 file changed

+47
-148
lines changed

1 file changed

+47
-148
lines changed

‎src/backend/commands/policy.c‎

Lines changed: 47 additions & 148 deletions
Original file line numberDiff line numberDiff line change
@@ -404,13 +404,12 @@ RemovePolicyById(Oid policy_id)
404404

405405
/*
406406
* RemoveRoleFromObjectPolicy -
407-
* remove a role from a policy by its OID. If the role is not a member of
408-
* the policy then an error is raised. False is returned to indicate that
409-
* the role could not be removed due to being the only role on the policy
410-
* and therefore the entire policy should be removed.
407+
* remove a role from a policy's applicable-roles list.
411408
*
412-
* Note that a warning will be thrown and true will be returned on a
413-
* permission error, as the policy should not be removed in that case.
409+
* Returns true if the role was successfully removed from the policy.
410+
* Returns false if the role was not removed because it would have left
411+
* polroles empty (which is disallowed, though perhaps it should not be).
412+
* On false return, the caller should instead drop the policy altogether.
414413
*
415414
* roleid - the oid of the role to remove
416415
* classid - should always be PolicyRelationId
@@ -424,11 +423,15 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
424423
ScanKeyDataskey[1];
425424
HeapTupletuple;
426425
Oidrelid;
427-
Relationrel;
428426
ArrayType*policy_roles;
429427
Datumroles_datum;
428+
Oid*roles;
429+
intnum_roles;
430+
Datum*role_oids;
430431
boolattr_isnull;
431432
boolkeep_policy= true;
433+
inti,
434+
j;
432435

433436
Assert(classid==PolicyRelationId);
434437

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

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

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

482468
policy_roles=DatumGetArrayTypePCopy(roles_datum);
469+
roles= (Oid*)ARR_DATA_PTR(policy_roles);
470+
num_roles=ARR_DIMS(policy_roles)[0];
483471

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

@@ -516,77 +499,6 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
516499
memset(replaces,0,sizeof(replaces));
517500
memset(isnull,0,sizeof(isnull));
518501

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

602-
/* Remove all old dependencies. */
603-
deleteDependencyRecordsFor(PolicyRelationId,policy_id, false);
604-
605-
/* Record the new set of dependencies */
606-
target.classId=RelationRelationId;
607-
target.objectId=relid;
608-
target.objectSubId=0;
514+
/* Remove all the old shared dependencies (roles) */
515+
deleteSharedDependencyRecordsFor(PolicyRelationId,policy_id,0);
609516

517+
/* Record the new shared dependencies (roles) */
610518
myself.classId=PolicyRelationId;
611519
myself.objectId=policy_id;
612520
myself.objectSubId=0;
613521

614-
recordDependencyOn(&myself,&target,DEPENDENCY_AUTO);
615-
616-
if (qual_expr)
617-
recordDependencyOnExpr(&myself,qual_expr,qual_parse_rtable,
618-
DEPENDENCY_NORMAL);
619-
620-
if (with_check_qual)
621-
recordDependencyOnExpr(&myself,with_check_qual,
622-
with_check_parse_rtable,
623-
DEPENDENCY_NORMAL);
624-
625-
/* Remove all the old shared dependencies (roles) */
626-
deleteSharedDependencyRecordsFor(PolicyRelationId,policy_id,0);
627-
628-
/* Record the new shared dependencies (roles) */
629522
target.classId=AuthIdRelationId;
630523
target.objectSubId=0;
631524
for (i=0;i<num_roles;i++)
@@ -644,21 +537,27 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
644537
/* Make updates visible */
645538
CommandCounterIncrement();
646539

647-
/* Invalidate Relation Cache */
648-
CacheInvalidateRelcache(rel);
649-
}
650-
else
540+
/*
541+
* Invalidate relcache entry for rel the policy belongs to, to force
542+
* redoing any dependent plans. In case of a race condition where the
543+
* rel was just dropped, we need do nothing.
544+
*/
545+
reltup=SearchSysCache1(RELOID,ObjectIdGetDatum(relid));
546+
if (HeapTupleIsValid(reltup))
651547
{
652-
/* No roles would remain, so drop the policy instead */
653-
keep_policy= false;
548+
CacheInvalidateRelcacheByTuple(reltup);
549+
ReleaseSysCache(reltup);
654550
}
655551
}
552+
else
553+
{
554+
/* No roles would remain, so drop the policy instead. */
555+
keep_policy= false;
556+
}
656557

657558
/* Clean up. */
658559
systable_endscan(sscan);
659560

660-
relation_close(rel,NoLock);
661-
662561
table_close(pg_policy_rel,RowExclusiveLock);
663562

664563
returnkeep_policy;

0 commit comments

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