From ef50bc4c71b08d08aa4a3c3e9fe0ea218f68d326 Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Tue, 26 Aug 2014 21:28:40 -0700 Subject: [PATCH 1/4] Make UPDATE privileges distinct from INSERT privileges in RTEs Previously, relation range table entries used a single Bitmapset field representing which columns required either UPDATE or INSERT privileges, despite the fact that INSERT and UPDATE privileges are separately cataloged, and may be independently held. This worked because ExecCheckRTEPerms() was called with a ACL_INSERT or ACL_UPDATE requiredPerms, and based on that it was evident which type of optimizable statement was under consideration. Since historically no type of optimizable statement could directly INSERT and UPDATE at the same time, there was no ambiguity as to which privileges were required. This largely mechanical commit is required infrastructure for the INSERT...ON CONFLICT UPDATE feature, which introduces an optimizable statement that may be subject to both INSERT and UPDATE permissions enforcement. Tests follow in a later commit. Note that this commit necessitates an initdb, since stored ACLs are broken. --- contrib/postgres_fdw/postgres_fdw.c | 2 +- src/backend/commands/copy.c | 2 +- src/backend/commands/createas.c | 2 +- src/backend/commands/trigger.c | 12 ++++---- src/backend/executor/execMain.c | 51 ++++++++++++++++++++++++++----- src/backend/nodes/copyfuncs.c | 3 +- src/backend/nodes/equalfuncs.c | 3 +- src/backend/nodes/outfuncs.c | 3 +- src/backend/nodes/readfuncs.c | 3 +- src/backend/optimizer/prep/prepsecurity.c | 6 ++-- src/backend/optimizer/prep/prepunion.c | 4 ++- src/backend/parser/analyze.c | 4 +-- src/backend/parser/parse_relation.c | 21 ++++++++----- src/backend/rewrite/rewriteHandler.c | 18 ++++++++--- src/include/nodes/parsenodes.h | 3 +- 15 files changed, 98 insertions(+), 39 deletions(-) diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index 4c49776..c99a3ee 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -1197,7 +1197,7 @@ postgresPlanForeignModify(PlannerInfo *root, } else if (operation == CMD_UPDATE) { - Bitmapset *tmpset = bms_copy(rte->modifiedCols); + Bitmapset *tmpset = bms_copy(rte->updatedCols); AttrNumber col; while ((col = bms_first_member(tmpset)) >= 0) diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index fbd7492..eb2844a 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -832,7 +832,7 @@ DoCopy(const CopyStmt *stmt, const char *queryString, uint64 *processed) FirstLowInvalidHeapAttributeNumber; if (is_from) - rte->modifiedCols = bms_add_member(rte->modifiedCols, attno); + rte->insertedCols = bms_add_member(rte->insertedCols, attno); else rte->selectedCols = bms_add_member(rte->selectedCols, attno); } diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c index 5245171..9688916 100644 --- a/src/backend/commands/createas.c +++ b/src/backend/commands/createas.c @@ -414,7 +414,7 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo) rte->requiredPerms = ACL_INSERT; for (attnum = 1; attnum <= intoRelationDesc->rd_att->natts; attnum++) - rte->modifiedCols = bms_add_member(rte->modifiedCols, + rte->insertedCols = bms_add_member(rte->insertedCols, attnum - FirstLowInvalidHeapAttributeNumber); ExecCheckRTPerms(list_make1(rte), true); diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 9bf0098..b9cd78b 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -65,8 +65,8 @@ int SessionReplicationRole = SESSION_REPLICATION_ROLE_ORIGIN; /* How many levels deep into trigger execution are we? */ static int MyTriggerDepth = 0; -#define GetModifiedColumns(relinfo, estate) \ - (rt_fetch((relinfo)->ri_RangeTableIndex, (estate)->es_range_table)->modifiedCols) +#define GetUpdatedColumns(relinfo, estate) \ + (rt_fetch((relinfo)->ri_RangeTableIndex, (estate)->es_range_table)->updatedCols) /* Local function prototypes */ static void ConvertTriggerToFK(CreateTrigStmt *stmt, Oid funcoid); @@ -2345,7 +2345,7 @@ ExecBSUpdateTriggers(EState *estate, ResultRelInfo *relinfo) if (!trigdesc->trig_update_before_statement) return; - modifiedCols = GetModifiedColumns(relinfo, estate); + modifiedCols = GetUpdatedColumns(relinfo, estate); LocTriggerData.type = T_TriggerData; LocTriggerData.tg_event = TRIGGER_EVENT_UPDATE | @@ -2391,7 +2391,7 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo *relinfo) if (trigdesc && trigdesc->trig_update_after_statement) AfterTriggerSaveEvent(estate, relinfo, TRIGGER_EVENT_UPDATE, false, NULL, NULL, NIL, - GetModifiedColumns(relinfo, estate)); + GetUpdatedColumns(relinfo, estate)); } TupleTableSlot * @@ -2418,7 +2418,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate, * been modified, then we can use a weaker lock, allowing for better * concurrency. */ - modifiedCols = GetModifiedColumns(relinfo, estate); + modifiedCols = GetUpdatedColumns(relinfo, estate); keyCols = RelationGetIndexAttrBitmap(relinfo->ri_RelationDesc, INDEX_ATTR_BITMAP_KEY); if (bms_overlap(keyCols, modifiedCols)) @@ -2545,7 +2545,7 @@ ExecARUpdateTriggers(EState *estate, ResultRelInfo *relinfo, AfterTriggerSaveEvent(estate, relinfo, TRIGGER_EVENT_UPDATE, true, trigtuple, newtuple, recheckIndexes, - GetModifiedColumns(relinfo, estate)); + GetUpdatedColumns(relinfo, estate)); if (trigtuple != fdw_trigtuple) heap_freetuple(trigtuple); } diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 072c7df..d71e9b8 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -631,11 +631,11 @@ ExecCheckRTEPerms(RangeTblEntry *rte) } /* - * Basically the same for the mod columns, with either INSERT or - * UPDATE privilege as specified by remainingPerms. + * Basically the same for the mod columns, for both INSERT and UPDATE + * privilege as specified by remainingPerms (INSERT...ON CONFLICT + * UPDATE may set both). */ - remainingPerms &= ~ACL_SELECT; - if (remainingPerms != 0) + if (remainingPerms & ACL_INSERT) { /* * When the query doesn't explicitly change any columns, allow the @@ -643,14 +643,14 @@ ExecCheckRTEPerms(RangeTblEntry *rte) * to handle SELECT FOR UPDATE as well as possible corner cases in * INSERT and UPDATE. */ - if (bms_is_empty(rte->modifiedCols)) + if (bms_is_empty(rte->insertedCols)) { - if (pg_attribute_aclcheck_all(relOid, userid, remainingPerms, + if (pg_attribute_aclcheck_all(relOid, userid, ACL_INSERT, ACLMASK_ANY) != ACLCHECK_OK) return false; } - tmpset = bms_copy(rte->modifiedCols); + tmpset = bms_copy(rte->insertedCols); while ((col = bms_first_member(tmpset)) >= 0) { /* remove the column number offset */ @@ -663,7 +663,42 @@ ExecCheckRTEPerms(RangeTblEntry *rte) else { if (pg_attribute_aclcheck(relOid, col, userid, - remainingPerms) != ACLCHECK_OK) + ACL_INSERT) != ACLCHECK_OK) + return false; + } + } + bms_free(tmpset); + } + + if (remainingPerms & ACL_UPDATE) + { + /* + * When the query doesn't explicitly change any columns, allow the + * query if we have permission on any column of the rel. This is + * to handle SELECT FOR UPDATE as well as possible corner cases in + * INSERT and UPDATE. + */ + if (bms_is_empty(rte->updatedCols)) + { + if (pg_attribute_aclcheck_all(relOid, userid, ACL_UPDATE, + ACLMASK_ANY) != ACLCHECK_OK) + return false; + } + + tmpset = bms_copy(rte->updatedCols); + while ((col = bms_first_member(tmpset)) >= 0) + { + /* remove the column number offset */ + col += FirstLowInvalidHeapAttributeNumber; + if (col == InvalidAttrNumber) + { + /* whole-row reference can't happen here */ + elog(ERROR, "whole-row update is not implemented"); + } + else + { + if (pg_attribute_aclcheck(relOid, col, userid, + ACL_UPDATE) != ACLCHECK_OK) return false; } } diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index aa053a0..9f5bcd7 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -1998,7 +1998,8 @@ _copyRangeTblEntry(const RangeTblEntry *from) COPY_SCALAR_FIELD(requiredPerms); COPY_SCALAR_FIELD(checkAsUser); COPY_BITMAPSET_FIELD(selectedCols); - COPY_BITMAPSET_FIELD(modifiedCols); + COPY_BITMAPSET_FIELD(insertedCols); + COPY_BITMAPSET_FIELD(updatedCols); COPY_NODE_FIELD(securityQuals); return newnode; diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c index 719923e..8d13c69 100644 --- a/src/backend/nodes/equalfuncs.c +++ b/src/backend/nodes/equalfuncs.c @@ -2319,7 +2319,8 @@ _equalRangeTblEntry(const RangeTblEntry *a, const RangeTblEntry *b) COMPARE_SCALAR_FIELD(requiredPerms); COMPARE_SCALAR_FIELD(checkAsUser); COMPARE_BITMAPSET_FIELD(selectedCols); - COMPARE_BITMAPSET_FIELD(modifiedCols); + COMPARE_BITMAPSET_FIELD(insertedCols); + COMPARE_BITMAPSET_FIELD(updatedCols); COMPARE_NODE_FIELD(securityQuals); return true; diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c index e686a6c..da75e29 100644 --- a/src/backend/nodes/outfuncs.c +++ b/src/backend/nodes/outfuncs.c @@ -2423,7 +2423,8 @@ _outRangeTblEntry(StringInfo str, const RangeTblEntry *node) WRITE_UINT_FIELD(requiredPerms); WRITE_OID_FIELD(checkAsUser); WRITE_BITMAPSET_FIELD(selectedCols); - WRITE_BITMAPSET_FIELD(modifiedCols); + WRITE_BITMAPSET_FIELD(insertedCols); + WRITE_BITMAPSET_FIELD(updatedCols); WRITE_NODE_FIELD(securityQuals); } diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c index 69d9989..65584d1 100644 --- a/src/backend/nodes/readfuncs.c +++ b/src/backend/nodes/readfuncs.c @@ -1252,7 +1252,8 @@ _readRangeTblEntry(void) READ_UINT_FIELD(requiredPerms); READ_OID_FIELD(checkAsUser); READ_BITMAPSET_FIELD(selectedCols); - READ_BITMAPSET_FIELD(modifiedCols); + READ_BITMAPSET_FIELD(insertedCols); + READ_BITMAPSET_FIELD(updatedCols); READ_NODE_FIELD(securityQuals); READ_DONE(); diff --git a/src/backend/optimizer/prep/prepsecurity.c b/src/backend/optimizer/prep/prepsecurity.c index 2420f97..1de9d99 100644 --- a/src/backend/optimizer/prep/prepsecurity.c +++ b/src/backend/optimizer/prep/prepsecurity.c @@ -115,7 +115,8 @@ expand_security_quals(PlannerInfo *root, List *tlist) rte->requiredPerms = 0; rte->checkAsUser = InvalidOid; rte->selectedCols = NULL; - rte->modifiedCols = NULL; + rte->insertedCols = NULL; + rte->updatedCols = NULL; /* * For the most part, Vars referencing the original relation @@ -213,7 +214,8 @@ expand_security_qual(PlannerInfo *root, List *tlist, int rt_index, rte->requiredPerms = 0; rte->checkAsUser = InvalidOid; rte->selectedCols = NULL; - rte->modifiedCols = NULL; + rte->insertedCols = NULL; + rte->updatedCols = NULL; /* * Now deal with any PlanRowMark on this RTE by requesting a lock diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c index 0410fdd..d607523 100644 --- a/src/backend/optimizer/prep/prepunion.c +++ b/src/backend/optimizer/prep/prepunion.c @@ -1374,7 +1374,9 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti) { childrte->selectedCols = translate_col_privs(rte->selectedCols, appinfo->translated_vars); - childrte->modifiedCols = translate_col_privs(rte->modifiedCols, + childrte->insertedCols = translate_col_privs(rte->insertedCols, + appinfo->translated_vars); + childrte->updatedCols = translate_col_privs(rte->updatedCols, appinfo->translated_vars); } diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c index fb6c44c..c0b1fe3 100644 --- a/src/backend/parser/analyze.c +++ b/src/backend/parser/analyze.c @@ -737,7 +737,7 @@ transformInsertStmt(ParseState *pstate, InsertStmt *stmt) false); qry->targetList = lappend(qry->targetList, tle); - rte->modifiedCols = bms_add_member(rte->modifiedCols, + rte->insertedCols = bms_add_member(rte->insertedCols, attr_num - FirstLowInvalidHeapAttributeNumber); icols = lnext(icols); @@ -2006,7 +2006,7 @@ transformUpdateStmt(ParseState *pstate, UpdateStmt *stmt) origTarget->location); /* Mark the target column as requiring update permissions */ - target_rte->modifiedCols = bms_add_member(target_rte->modifiedCols, + target_rte->updatedCols = bms_add_member(target_rte->updatedCols, attrno - FirstLowInvalidHeapAttributeNumber); origTargetList = lnext(origTargetList); diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c index 478584d..364ced0 100644 --- a/src/backend/parser/parse_relation.c +++ b/src/backend/parser/parse_relation.c @@ -1052,7 +1052,8 @@ addRangeTableEntry(ParseState *pstate, rte->requiredPerms = ACL_SELECT; rte->checkAsUser = InvalidOid; /* not set-uid by default, either */ rte->selectedCols = NULL; - rte->modifiedCols = NULL; + rte->insertedCols = NULL; + rte->updatedCols = NULL; /* * Add completed RTE to pstate's range table list, but not to join list @@ -1105,7 +1106,8 @@ addRangeTableEntryForRelation(ParseState *pstate, rte->requiredPerms = ACL_SELECT; rte->checkAsUser = InvalidOid; /* not set-uid by default, either */ rte->selectedCols = NULL; - rte->modifiedCols = NULL; + rte->insertedCols = NULL; + rte->updatedCols = NULL; /* * Add completed RTE to pstate's range table list, but not to join list @@ -1183,7 +1185,8 @@ addRangeTableEntryForSubquery(ParseState *pstate, rte->requiredPerms = 0; rte->checkAsUser = InvalidOid; rte->selectedCols = NULL; - rte->modifiedCols = NULL; + rte->insertedCols = NULL; + rte->updatedCols = NULL; /* * Add completed RTE to pstate's range table list, but not to join list @@ -1437,7 +1440,8 @@ addRangeTableEntryForFunction(ParseState *pstate, rte->requiredPerms = 0; rte->checkAsUser = InvalidOid; rte->selectedCols = NULL; - rte->modifiedCols = NULL; + rte->insertedCols = NULL; + rte->updatedCols = NULL; /* * Add completed RTE to pstate's range table list, but not to join list @@ -1509,7 +1513,8 @@ addRangeTableEntryForValues(ParseState *pstate, rte->requiredPerms = 0; rte->checkAsUser = InvalidOid; rte->selectedCols = NULL; - rte->modifiedCols = NULL; + rte->insertedCols = NULL; + rte->updatedCols = NULL; /* * Add completed RTE to pstate's range table list, but not to join list @@ -1577,7 +1582,8 @@ addRangeTableEntryForJoin(ParseState *pstate, rte->requiredPerms = 0; rte->checkAsUser = InvalidOid; rte->selectedCols = NULL; - rte->modifiedCols = NULL; + rte->insertedCols = NULL; + rte->updatedCols = NULL; /* * Add completed RTE to pstate's range table list, but not to join list @@ -1677,7 +1683,8 @@ addRangeTableEntryForCTE(ParseState *pstate, rte->requiredPerms = 0; rte->checkAsUser = InvalidOid; rte->selectedCols = NULL; - rte->modifiedCols = NULL; + rte->insertedCols = NULL; + rte->updatedCols = NULL; /* * Add completed RTE to pstate's range table list, but not to join list diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index e6c5530..cc967f0 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -1401,7 +1401,8 @@ ApplyRetrieveRule(Query *parsetree, rte->requiredPerms = 0; rte->checkAsUser = InvalidOid; rte->selectedCols = NULL; - rte->modifiedCols = NULL; + rte->insertedCols = NULL; + rte->updatedCols = NULL; /* * For the most part, Vars referencing the view should remain as @@ -1464,12 +1465,14 @@ ApplyRetrieveRule(Query *parsetree, subrte->requiredPerms = rte->requiredPerms; subrte->checkAsUser = rte->checkAsUser; subrte->selectedCols = rte->selectedCols; - subrte->modifiedCols = rte->modifiedCols; + subrte->insertedCols = rte->insertedCols; + subrte->updatedCols = rte->updatedCols; rte->requiredPerms = 0; /* no permission check on subquery itself */ rte->checkAsUser = InvalidOid; rte->selectedCols = NULL; - rte->modifiedCols = NULL; + rte->insertedCols = NULL; + rte->updatedCols = NULL; /* * If FOR [KEY] UPDATE/SHARE of view, mark all the contained tables as @@ -2697,8 +2700,13 @@ rewriteTargetView(Query *parsetree, Relation view) * This step needs the modified view targetlist, so we have to do things * in this order. */ - Assert(bms_is_empty(new_rte->modifiedCols)); - new_rte->modifiedCols = adjust_view_column_set(view_rte->modifiedCols, + Assert(bms_is_empty(new_rte->insertedCols) && + bms_is_empty(new_rte->updatedCols)); + + new_rte->insertedCols = adjust_view_column_set(view_rte->insertedCols, + view_targetlist); + + new_rte->updatedCols = adjust_view_column_set(view_rte->updatedCols, view_targetlist); /* diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index d2c0b29..2c5d842 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -814,7 +814,8 @@ typedef struct RangeTblEntry AclMode requiredPerms; /* bitmask of required access permissions */ Oid checkAsUser; /* if valid, check access as this role */ Bitmapset *selectedCols; /* columns needing SELECT permission */ - Bitmapset *modifiedCols; /* columns needing INSERT/UPDATE permission */ + Bitmapset *insertedCols; /* columns needing INSERT permission */ + Bitmapset *updatedCols; /* columns needing UPDATE permission */ List *securityQuals; /* any security barrier quals to apply */ } RangeTblEntry; -- 1.9.1