From fdba460a26af919c0b366755d119f384706e670a Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sun, 30 Sep 2018 13:55:51 -0400 Subject: [PATCH] Create an RTE field to record the query's lock mode for each relation. Add RangeTblEntry.rellockmode, which records the appropriate lock mode for each RTE_RELATION rangetable entry (either AccessShareLock, RowShareLock, or RowExclusiveLock depending on the RTE's role in the query). This patch creates the field and makes all creators of RTE nodes fill it in reasonably, but for the moment nothing much is done with it. The plan is to replace assorted post-parser logic that re-determines the right lockmode to use with simple uses of rte->rellockmode. For now, just add Asserts in each of those places that the rellockmode matches what they are computing today. (In some cases the match isn't perfect, so the Asserts are weaker than you might expect; but this seems OK, as per discussion.) This passes check-world for me, but it seems worth pushing in this state to see if the buildfarm finds any problems in cases I failed to test. catversion bump due to change of stored rules. Amit Langote, reviewed by David Rowley and Jesper Pedersen, and whacked around a bit more by me Discussion: https://postgr.es/m/468c85d9-540e-66a2-1dde-fec2b741e688@lab.ntt.co.jp --- src/backend/catalog/dependency.c | 1 + src/backend/catalog/heap.c | 1 + src/backend/commands/copy.c | 9 +++-- src/backend/commands/createas.c | 1 + src/backend/commands/policy.c | 23 +++++++---- src/backend/commands/tablecmds.c | 3 +- src/backend/commands/trigger.c | 2 + src/backend/commands/view.c | 4 +- src/backend/executor/execMain.c | 11 ++++++ src/backend/executor/execUtils.c | 20 ++++++++-- src/backend/nodes/copyfuncs.c | 1 + src/backend/nodes/equalfuncs.c | 1 + src/backend/nodes/outfuncs.c | 1 + src/backend/nodes/readfuncs.c | 1 + src/backend/optimizer/plan/planner.c | 2 + src/backend/parser/analyze.c | 1 + src/backend/parser/parse_clause.c | 1 + src/backend/parser/parse_relation.c | 30 ++++++++++++-- src/backend/parser/parse_utilcmd.c | 9 ++++- src/backend/replication/logical/tablesync.c | 3 +- src/backend/replication/logical/worker.c | 1 + src/backend/rewrite/rewriteHandler.c | 39 ++++++++++++++----- src/backend/utils/adt/ri_triggers.c | 2 + src/backend/utils/adt/ruleutils.c | 3 ++ src/backend/utils/cache/plancache.c | 6 ++- src/include/catalog/catversion.h | 2 +- src/include/nodes/parsenodes.h | 12 ++++++ src/include/parser/parse_relation.h | 1 + .../modules/test_rls_hooks/test_rls_hooks.c | 8 ++-- 29 files changed, 160 insertions(+), 39 deletions(-) diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c index 4f1d365357..7dfa3278a5 100644 --- a/src/backend/catalog/dependency.c +++ b/src/backend/catalog/dependency.c @@ -1415,6 +1415,7 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender, rte.rtekind = RTE_RELATION; rte.relid = relId; rte.relkind = RELKIND_RELATION; /* no need for exactness here */ + rte.rellockmode = AccessShareLock; context.rtables = list_make1(list_make1(&rte)); diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index 9176f6280b..4ad58689fc 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -2549,6 +2549,7 @@ AddRelationNewConstraints(Relation rel, pstate->p_sourcetext = queryString; rte = addRangeTableEntryForRelation(pstate, rel, + AccessShareLock, NULL, false, true); diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index d06662bd32..2d5bc8add6 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -833,20 +833,21 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt, if (stmt->relation) { + LOCKMODE lockmode = is_from ? RowExclusiveLock : AccessShareLock; + RangeTblEntry *rte; TupleDesc tupDesc; List *attnums; ListCell *cur; - RangeTblEntry *rte; Assert(!stmt->query); /* Open and lock the relation, using the appropriate lock type. */ - rel = heap_openrv(stmt->relation, - (is_from ? RowExclusiveLock : AccessShareLock)); + rel = heap_openrv(stmt->relation, lockmode); relid = RelationGetRelid(rel); - rte = addRangeTableEntryForRelation(pstate, rel, NULL, false, false); + rte = addRangeTableEntryForRelation(pstate, rel, lockmode, + NULL, false, false); rte->requiredPerms = (is_from ? ACL_INSERT : ACL_SELECT); tupDesc = RelationGetDescr(rel); diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c index 3d82edbf58..d5cb62da15 100644 --- a/src/backend/commands/createas.c +++ b/src/backend/commands/createas.c @@ -528,6 +528,7 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo) rte->rtekind = RTE_RELATION; rte->relid = intoRelationAddr.objectId; rte->relkind = relkind; + rte->rellockmode = RowExclusiveLock; rte->requiredPerms = ACL_INSERT; for (attnum = 1; attnum <= intoRelationDesc->rd_att->natts; attnum++) diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c index cee0ef915b..2fd17b24b9 100644 --- a/src/backend/commands/policy.c +++ b/src/backend/commands/policy.c @@ -567,7 +567,9 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id) qual_expr = stringToNode(qual_value); /* Add this rel to the parsestate's rangetable, for dependencies */ - addRangeTableEntryForRelation(qual_pstate, rel, NULL, false, false); + addRangeTableEntryForRelation(qual_pstate, rel, + AccessShareLock, + NULL, false, false); qual_parse_rtable = qual_pstate->p_rtable; free_parsestate(qual_pstate); @@ -589,8 +591,9 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id) with_check_qual = stringToNode(with_check_value); /* Add this rel to the parsestate's rangetable, for dependencies */ - addRangeTableEntryForRelation(with_check_pstate, rel, NULL, false, - false); + addRangeTableEntryForRelation(with_check_pstate, rel, + AccessShareLock, + NULL, false, false); with_check_parse_rtable = with_check_pstate->p_rtable; free_parsestate(with_check_pstate); @@ -752,11 +755,13 @@ CreatePolicy(CreatePolicyStmt *stmt) /* Add for the regular security quals */ rte = addRangeTableEntryForRelation(qual_pstate, target_table, + AccessShareLock, NULL, false, false); addRTEtoQuery(qual_pstate, rte, false, true, true); /* Add for the with-check quals */ rte = addRangeTableEntryForRelation(with_check_pstate, target_table, + AccessShareLock, NULL, false, false); addRTEtoQuery(with_check_pstate, rte, false, true, true); @@ -928,6 +933,7 @@ AlterPolicy(AlterPolicyStmt *stmt) ParseState *qual_pstate = make_parsestate(NULL); rte = addRangeTableEntryForRelation(qual_pstate, target_table, + AccessShareLock, NULL, false, false); addRTEtoQuery(qual_pstate, rte, false, true, true); @@ -950,6 +956,7 @@ AlterPolicy(AlterPolicyStmt *stmt) ParseState *with_check_pstate = make_parsestate(NULL); rte = addRangeTableEntryForRelation(with_check_pstate, target_table, + AccessShareLock, NULL, false, false); addRTEtoQuery(with_check_pstate, rte, false, true, true); @@ -1096,8 +1103,9 @@ AlterPolicy(AlterPolicyStmt *stmt) qual = stringToNode(qual_value); /* Add this rel to the parsestate's rangetable, for dependencies */ - addRangeTableEntryForRelation(qual_pstate, target_table, NULL, - false, false); + addRangeTableEntryForRelation(qual_pstate, target_table, + AccessShareLock, + NULL, false, false); qual_parse_rtable = qual_pstate->p_rtable; free_parsestate(qual_pstate); @@ -1137,8 +1145,9 @@ AlterPolicy(AlterPolicyStmt *stmt) with_check_qual = stringToNode(with_check_value); /* Add this rel to the parsestate's rangetable, for dependencies */ - addRangeTableEntryForRelation(with_check_pstate, target_table, NULL, - false, false); + addRangeTableEntryForRelation(with_check_pstate, target_table, + AccessShareLock, + NULL, false, false); with_check_parse_rtable = with_check_pstate->p_rtable; free_parsestate(with_check_pstate); diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 29d8353779..9e60bb37a7 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -13632,7 +13632,8 @@ transformPartitionSpec(Relation rel, PartitionSpec *partspec, char *strategy) * rangetable entry. We need a ParseState for transformExpr. */ pstate = make_parsestate(NULL); - rte = addRangeTableEntryForRelation(pstate, rel, NULL, false, true); + rte = addRangeTableEntryForRelation(pstate, rel, AccessShareLock, + NULL, false, true); addRTEtoQuery(pstate, rte, true, true, true); /* take care of any partition expressions */ diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 36778b6f4d..b2de1cc391 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -577,10 +577,12 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString, * 'OLD' must always have varno equal to 1 and 'NEW' equal to 2. */ rte = addRangeTableEntryForRelation(pstate, rel, + AccessShareLock, makeAlias("old", NIL), false, false); addRTEtoQuery(pstate, rte, false, true, true); rte = addRangeTableEntryForRelation(pstate, rel, + AccessShareLock, makeAlias("new", NIL), false, false); addRTEtoQuery(pstate, rte, false, true, true); diff --git a/src/backend/commands/view.c b/src/backend/commands/view.c index ffb71c0ea7..e73f1d8894 100644 --- a/src/backend/commands/view.c +++ b/src/backend/commands/view.c @@ -353,7 +353,7 @@ DefineViewRules(Oid viewOid, Query *viewParse, bool replace) * by 2... * * These extra RT entries are not actually used in the query, - * except for run-time permission checking. + * except for run-time locking and permission checking. *--------------------------------------------------------------- */ static Query * @@ -386,9 +386,11 @@ UpdateRangeTableOfViewParse(Oid viewOid, Query *viewParse) * OLD first, then NEW.... */ rt_entry1 = addRangeTableEntryForRelation(pstate, viewRel, + AccessShareLock, makeAlias("old", NIL), false, false); rt_entry2 = addRangeTableEntryForRelation(pstate, viewRel, + AccessShareLock, makeAlias("new", NIL), false, false); /* Must override addRangeTableEntry's default access-check flags */ diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 5443cbf67d..2b7cf26380 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -864,6 +864,7 @@ InitPlan(QueryDesc *queryDesc, int eflags) Relation resultRelation; resultRelationOid = getrelid(resultRelationIndex, rangeTable); + Assert(rt_fetch(resultRelationIndex, rangeTable)->rellockmode == RowExclusiveLock); resultRelation = heap_open(resultRelationOid, RowExclusiveLock); InitResultRelInfo(resultRelInfo, @@ -904,6 +905,7 @@ InitPlan(QueryDesc *queryDesc, int eflags) Relation resultRelDesc; resultRelOid = getrelid(resultRelIndex, rangeTable); + Assert(rt_fetch(resultRelIndex, rangeTable)->rellockmode == RowExclusiveLock); resultRelDesc = heap_open(resultRelOid, RowExclusiveLock); InitResultRelInfo(resultRelInfo, resultRelDesc, @@ -924,8 +926,11 @@ InitPlan(QueryDesc *queryDesc, int eflags) /* We locked the roots above. */ if (!list_member_int(plannedstmt->rootResultRelations, resultRelIndex)) + { + Assert(rt_fetch(resultRelIndex, rangeTable)->rellockmode == RowExclusiveLock); LockRelationOid(getrelid(resultRelIndex, rangeTable), RowExclusiveLock); + } } } } @@ -955,6 +960,7 @@ InitPlan(QueryDesc *queryDesc, int eflags) { PlanRowMark *rc = (PlanRowMark *) lfirst(l); Oid relid; + LOCKMODE rellockmode; Relation relation; ExecRowMark *erm; @@ -964,6 +970,7 @@ InitPlan(QueryDesc *queryDesc, int eflags) /* get relation's OID (will produce InvalidOid if subquery) */ relid = getrelid(rc->rti, rangeTable); + rellockmode = rt_fetch(rc->rti, rangeTable)->rellockmode; /* * If you change the conditions under which rel locks are acquired @@ -975,9 +982,13 @@ InitPlan(QueryDesc *queryDesc, int eflags) case ROW_MARK_NOKEYEXCLUSIVE: case ROW_MARK_SHARE: case ROW_MARK_KEYSHARE: + Assert(rellockmode == RowShareLock); relation = heap_open(relid, RowShareLock); break; case ROW_MARK_REFERENCE: + /* RTE might be a query target table */ + Assert(rellockmode == AccessShareLock || + rellockmode == RowExclusiveLock); relation = heap_open(relid, AccessShareLock); break; case ROW_MARK_COPY: diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c index 5b3eaec80b..da84d5df44 100644 --- a/src/backend/executor/execUtils.c +++ b/src/backend/executor/execUtils.c @@ -657,20 +657,32 @@ ExecOpenScanRelation(EState *estate, Index scanrelid, int eflags) /* * Determine the lock type we need. First, scan to see if target relation * is a result relation. If not, check if it's a FOR UPDATE/FOR SHARE - * relation. In either of those cases, we got the lock already. + * relation. + * + * Note: we may have already gotten the desired lock type, but for now + * don't try to optimize; this logic is going away soon anyhow. */ lockmode = AccessShareLock; if (ExecRelationIsTargetRelation(estate, scanrelid)) - lockmode = NoLock; + lockmode = RowExclusiveLock; else { /* Keep this check in sync with InitPlan! */ ExecRowMark *erm = ExecFindRowMark(estate, scanrelid, true); - if (erm != NULL && erm->relation != NULL) - lockmode = NoLock; + if (erm != NULL) + { + if (erm->markType == ROW_MARK_REFERENCE || + erm->markType == ROW_MARK_COPY) + lockmode = AccessShareLock; + else + lockmode = RowShareLock; + } } + /* lockmode per above logic must not be more than we previously acquired */ + Assert(lockmode <= rt_fetch(scanrelid, estate->es_range_table)->rellockmode); + /* Open the relation and acquire lock as needed */ reloid = getrelid(scanrelid, estate->es_range_table); rel = heap_open(reloid, lockmode); diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index 7c8220cf65..925cb8f380 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -2356,6 +2356,7 @@ _copyRangeTblEntry(const RangeTblEntry *from) COPY_SCALAR_FIELD(rtekind); COPY_SCALAR_FIELD(relid); COPY_SCALAR_FIELD(relkind); + COPY_SCALAR_FIELD(rellockmode); COPY_NODE_FIELD(tablesample); COPY_NODE_FIELD(subquery); COPY_SCALAR_FIELD(security_barrier); diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c index 378f2facb8..3bb91c9595 100644 --- a/src/backend/nodes/equalfuncs.c +++ b/src/backend/nodes/equalfuncs.c @@ -2630,6 +2630,7 @@ _equalRangeTblEntry(const RangeTblEntry *a, const RangeTblEntry *b) COMPARE_SCALAR_FIELD(rtekind); COMPARE_SCALAR_FIELD(relid); COMPARE_SCALAR_FIELD(relkind); + COMPARE_SCALAR_FIELD(rellockmode); COMPARE_NODE_FIELD(tablesample); COMPARE_NODE_FIELD(subquery); COMPARE_SCALAR_FIELD(security_barrier); diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c index 93f1e2c4eb..22dbae15d3 100644 --- a/src/backend/nodes/outfuncs.c +++ b/src/backend/nodes/outfuncs.c @@ -3131,6 +3131,7 @@ _outRangeTblEntry(StringInfo str, const RangeTblEntry *node) case RTE_RELATION: WRITE_OID_FIELD(relid); WRITE_CHAR_FIELD(relkind); + WRITE_INT_FIELD(rellockmode); WRITE_NODE_FIELD(tablesample); break; case RTE_SUBQUERY: diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c index 519deab63a..ce556580a5 100644 --- a/src/backend/nodes/readfuncs.c +++ b/src/backend/nodes/readfuncs.c @@ -1361,6 +1361,7 @@ _readRangeTblEntry(void) case RTE_RELATION: READ_OID_FIELD(relid); READ_CHAR_FIELD(relkind); + READ_INT_FIELD(rellockmode); READ_NODE_FIELD(tablesample); break; case RTE_SUBQUERY: diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 22c010c19e..89625f4f5b 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -6021,6 +6021,7 @@ plan_cluster_use_sort(Oid tableOid, Oid indexOid) rte->rtekind = RTE_RELATION; rte->relid = tableOid; rte->relkind = RELKIND_RELATION; /* Don't be too picky. */ + rte->rellockmode = AccessShareLock; rte->lateral = false; rte->inh = false; rte->inFromCl = true; @@ -6143,6 +6144,7 @@ plan_create_index_workers(Oid tableOid, Oid indexOid) rte->rtekind = RTE_RELATION; rte->relid = tableOid; rte->relkind = RELKIND_RELATION; /* Don't be too picky. */ + rte->rellockmode = AccessShareLock; rte->lateral = false; rte->inh = true; rte->inFromCl = true; diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c index c020600955..226927b7ab 100644 --- a/src/backend/parser/analyze.c +++ b/src/backend/parser/analyze.c @@ -1037,6 +1037,7 @@ transformOnConflictClause(ParseState *pstate, */ exclRte = addRangeTableEntryForRelation(pstate, targetrel, + RowExclusiveLock, makeAlias("excluded", NIL), false, false); exclRte->relkind = RELKIND_COMPOSITE_TYPE; diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c index d6b93f55df..660011a3ec 100644 --- a/src/backend/parser/parse_clause.c +++ b/src/backend/parser/parse_clause.c @@ -217,6 +217,7 @@ setTargetTable(ParseState *pstate, RangeVar *relation, * Now build an RTE. */ rte = addRangeTableEntryForRelation(pstate, pstate->p_target_relation, + RowExclusiveLock, relation->alias, inh, false); pstate->p_target_rangetblentry = rte; diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c index 60b8de0f95..da600dc0ff 100644 --- a/src/backend/parser/parse_relation.c +++ b/src/backend/parser/parse_relation.c @@ -1208,15 +1208,22 @@ addRangeTableEntry(ParseState *pstate, rte->alias = alias; /* - * Get the rel's OID. This access also ensures that we have an up-to-date - * relcache entry for the rel. Since this is typically the first access - * to a rel in a statement, be careful to get the right access level - * depending on whether we're doing SELECT FOR UPDATE/SHARE. + * Identify the type of lock we'll need on this relation. It's not the + * query's target table (that case is handled elsewhere), so we need + * either RowShareLock if it's locked by FOR UPDATE/SHARE, or plain + * AccessShareLock otherwise. */ lockmode = isLockedRefname(pstate, refname) ? RowShareLock : AccessShareLock; + + /* + * Get the rel's OID. This access also ensures that we have an up-to-date + * relcache entry for the rel. Since this is typically the first access + * to a rel in a statement, we must open the rel with the proper lockmode. + */ rel = parserOpenTable(pstate, relation, lockmode); rte->relid = RelationGetRelid(rel); rte->relkind = rel->rd_rel->relkind; + rte->rellockmode = lockmode; /* * Build the list of effective column names using user-supplied aliases @@ -1262,10 +1269,20 @@ addRangeTableEntry(ParseState *pstate, * * This is just like addRangeTableEntry() except that it makes an RTE * given an already-open relation instead of a RangeVar reference. + * + * lockmode is the lock type required for query execution; it must be one + * of AccessShareLock, RowShareLock, or RowExclusiveLock depending on the + * RTE's role within the query. The caller should always hold that lock mode + * or a stronger one. + * + * Note: properly, lockmode should be declared LOCKMODE not int, but that + * would require importing storage/lock.h into parse_relation.h. Since + * LOCKMODE is typedef'd as int anyway, that seems like overkill. */ RangeTblEntry * addRangeTableEntryForRelation(ParseState *pstate, Relation rel, + int lockmode, Alias *alias, bool inh, bool inFromCl) @@ -1275,10 +1292,15 @@ addRangeTableEntryForRelation(ParseState *pstate, Assert(pstate != NULL); + Assert(lockmode == AccessShareLock || + lockmode == RowShareLock || + lockmode == RowExclusiveLock); + rte->rtekind = RTE_RELATION; rte->alias = alias; rte->relid = RelationGetRelid(rel); rte->relkind = rel->rd_rel->relkind; + rte->rellockmode = lockmode; /* * Build the list of effective column names using user-supplied aliases diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c index f5c1e2a0d7..42bf9bfec6 100644 --- a/src/backend/parser/parse_utilcmd.c +++ b/src/backend/parser/parse_utilcmd.c @@ -2526,7 +2526,9 @@ transformIndexStmt(Oid relid, IndexStmt *stmt, const char *queryString) * relation, but we still need to open it. */ rel = relation_open(relid, NoLock); - rte = addRangeTableEntryForRelation(pstate, rel, NULL, false, true); + rte = addRangeTableEntryForRelation(pstate, rel, + AccessShareLock, + NULL, false, true); /* no to join list, yes to namespaces */ addRTEtoQuery(pstate, rte, false, true, true); @@ -2635,9 +2637,11 @@ transformRuleStmt(RuleStmt *stmt, const char *queryString, * qualification. */ oldrte = addRangeTableEntryForRelation(pstate, rel, + AccessShareLock, makeAlias("old", NIL), false, false); newrte = addRangeTableEntryForRelation(pstate, rel, + AccessShareLock, makeAlias("new", NIL), false, false); /* Must override addRangeTableEntry's default access-check flags */ @@ -2733,9 +2737,11 @@ transformRuleStmt(RuleStmt *stmt, const char *queryString, * them in the joinlist. */ oldrte = addRangeTableEntryForRelation(sub_pstate, rel, + AccessShareLock, makeAlias("old", NIL), false, false); newrte = addRangeTableEntryForRelation(sub_pstate, rel, + AccessShareLock, makeAlias("new", NIL), false, false); oldrte->requiredPerms = 0; @@ -2938,6 +2944,7 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt, pstate->p_sourcetext = queryString; rte = addRangeTableEntryForRelation(pstate, rel, + AccessShareLock, NULL, false, true); diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c index acc6498567..6e420d893c 100644 --- a/src/backend/replication/logical/tablesync.c +++ b/src/backend/replication/logical/tablesync.c @@ -795,7 +795,8 @@ copy_table(Relation rel) copybuf = makeStringInfo(); pstate = make_parsestate(NULL); - addRangeTableEntryForRelation(pstate, rel, NULL, false, false); + addRangeTableEntryForRelation(pstate, rel, AccessShareLock, + NULL, false, false); attnamelist = make_copy_attnamelist(relmapentry); cstate = BeginCopyFrom(pstate, rel, NULL, false, copy_read_data, attnamelist, NIL); diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c index 42345f94d3..e247539d7b 100644 --- a/src/backend/replication/logical/worker.c +++ b/src/backend/replication/logical/worker.c @@ -199,6 +199,7 @@ create_estate_for_relation(LogicalRepRelMapEntry *rel) rte->rtekind = RTE_RELATION; rte->relid = RelationGetRelid(rel->localrel); rte->relkind = rel->localrel->rd_rel->relkind; + rte->rellockmode = AccessShareLock; estate->es_range_table = list_make1(rte); resultRelInfo = makeNode(ResultRelInfo); diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index 327e5c33d7..42cec2aeae 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -89,6 +89,10 @@ static Bitmapset *adjust_view_column_set(Bitmapset *cols, List *targetlist); * These locks will ensure that the relation schemas don't change under us * while we are rewriting and planning the query. * + * Caution: this may modify the querytree, therefore caller should usually + * have done a copyObject() to make a writable copy of the querytree in the + * current memory context. + * * forExecute indicates that the query is about to be executed. * If so, we'll acquire RowExclusiveLock on the query's resultRelation, * RowShareLock on any relation accessed FOR [KEY] UPDATE/SHARE, and @@ -101,13 +105,11 @@ static Bitmapset *adjust_view_column_set(Bitmapset *cols, List *targetlist); * forUpdatePushedDown indicates that a pushed-down FOR [KEY] UPDATE/SHARE * applies to the current subquery, requiring all rels to be opened with at * least RowShareLock. This should always be false at the top of the - * recursion. This flag is ignored if forExecute is false. + * recursion. When it is true, we adjust RTE rellockmode fields to reflect + * the higher lock level. This flag is ignored if forExecute is false. * * A secondary purpose of this routine is to fix up JOIN RTE references to - * dropped columns (see details below). Because the RTEs are modified in - * place, it is generally appropriate for the caller of this routine to have - * first done a copyObject() to make a writable copy of the querytree in the - * current memory context. + * dropped columns (see details below). Such RTEs are modified in-place. * * This processing can, and for efficiency's sake should, be skipped when the * querytree has just been built by the parser: parse analysis already got @@ -170,12 +172,22 @@ AcquireRewriteLocks(Query *parsetree, lockmode = AccessShareLock; else if (rt_index == parsetree->resultRelation) lockmode = RowExclusiveLock; - else if (forUpdatePushedDown || - get_parse_rowmark(parsetree, rt_index) != NULL) + else if (forUpdatePushedDown) + { + lockmode = RowShareLock; + /* Upgrade RTE's lock mode to reflect pushed-down lock */ + if (rte->rellockmode == AccessShareLock) + rte->rellockmode = RowShareLock; + } + else if (get_parse_rowmark(parsetree, rt_index) != NULL) lockmode = RowShareLock; else lockmode = AccessShareLock; + Assert(!forExecute || lockmode == rte->rellockmode || + (lockmode == AccessShareLock && + rte->rellockmode == RowExclusiveLock)); + rel = heap_open(rte->relid, lockmode); /* @@ -1593,6 +1605,7 @@ ApplyRetrieveRule(Query *parsetree, /* Clear fields that should not be set in a subquery RTE */ rte->relid = InvalidOid; rte->relkind = 0; + rte->rellockmode = 0; rte->tablesample = NULL; rte->inh = false; /* must not be set for a subquery */ @@ -2920,8 +2933,14 @@ rewriteTargetView(Query *parsetree, Relation view) * very much like what the planner would do to "pull up" the view into the * outer query. Perhaps someday we should refactor things enough so that * we can share code with the planner.) + * + * Be sure to set rellockmode to the correct thing for the target table. + * Since we copied the whole viewquery above, we can just scribble on + * base_rte instead of copying it. */ - new_rte = (RangeTblEntry *) base_rte; + new_rte = base_rte; + new_rte->rellockmode = RowExclusiveLock; + parsetree->rtable = lappend(parsetree->rtable, new_rte); new_rt_index = list_length(parsetree->rtable); @@ -3101,8 +3120,8 @@ rewriteTargetView(Query *parsetree, Relation view) new_exclRte = addRangeTableEntryForRelation(make_parsestate(NULL), base_rel, - makeAlias("excluded", - NIL), + RowExclusiveLock, + makeAlias("excluded", NIL), false, false); new_exclRte->relkind = RELKIND_COMPOSITE_TYPE; new_exclRte->requiredPerms = 0; diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index fc034ce601..049b20449a 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -1878,12 +1878,14 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel) pkrte->rtekind = RTE_RELATION; pkrte->relid = RelationGetRelid(pk_rel); pkrte->relkind = pk_rel->rd_rel->relkind; + pkrte->rellockmode = AccessShareLock; pkrte->requiredPerms = ACL_SELECT; fkrte = makeNode(RangeTblEntry); fkrte->rtekind = RTE_RELATION; fkrte->relid = RelationGetRelid(fk_rel); fkrte->relkind = fk_rel->rd_rel->relkind; + fkrte->rellockmode = AccessShareLock; fkrte->requiredPerms = ACL_SELECT; for (i = 0; i < riinfo->nkeys; i++) diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index eecd64e4b5..f6693eaa79 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -1000,6 +1000,7 @@ pg_get_triggerdef_worker(Oid trigid, bool pretty) oldrte->rtekind = RTE_RELATION; oldrte->relid = trigrec->tgrelid; oldrte->relkind = relkind; + oldrte->rellockmode = AccessShareLock; oldrte->alias = makeAlias("old", NIL); oldrte->eref = oldrte->alias; oldrte->lateral = false; @@ -1010,6 +1011,7 @@ pg_get_triggerdef_worker(Oid trigid, bool pretty) newrte->rtekind = RTE_RELATION; newrte->relid = trigrec->tgrelid; newrte->relkind = relkind; + newrte->rellockmode = AccessShareLock; newrte->alias = makeAlias("new", NIL); newrte->eref = newrte->alias; newrte->lateral = false; @@ -3206,6 +3208,7 @@ deparse_context_for(const char *aliasname, Oid relid) rte->rtekind = RTE_RELATION; rte->relid = relid; rte->relkind = RELKIND_RELATION; /* no need for exactness here */ + rte->rellockmode = AccessShareLock; rte->alias = makeAlias(aliasname, NIL); rte->eref = rte->alias; rte->lateral = false; diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c index 7271b5880b..f3d06cd074 100644 --- a/src/backend/utils/cache/plancache.c +++ b/src/backend/utils/cache/plancache.c @@ -107,7 +107,7 @@ static void PlanCacheFuncCallback(Datum arg, int cacheid, uint32 hashvalue); static void PlanCacheSysCallback(Datum arg, int cacheid, uint32 hashvalue); /* GUC parameter */ -int plan_cache_mode; +int plan_cache_mode; /* * InitPlanCache: initialize module during InitPostgres. @@ -1539,6 +1539,8 @@ AcquireExecutorLocks(List *stmt_list, bool acquire) else lockmode = AccessShareLock; + Assert(lockmode == rte->rellockmode); + if (acquire) LockRelationOid(rte->relid, lockmode); else @@ -1609,6 +1611,8 @@ ScanQueryForLocks(Query *parsetree, bool acquire) lockmode = RowShareLock; else lockmode = AccessShareLock; + Assert(lockmode == rte->rellockmode || + (lockmode == AccessShareLock && rte->rellockmode == RowExclusiveLock)); if (acquire) LockRelationOid(rte->relid, lockmode); else diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h index 07e0576db7..02a9866f32 100644 --- a/src/include/catalog/catversion.h +++ b/src/include/catalog/catversion.h @@ -53,6 +53,6 @@ */ /* yyyymmddN */ -#define CATALOG_VERSION_NO 201809241 +#define CATALOG_VERSION_NO 201809301 #endif diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 62209a8f10..200df8e659 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -973,9 +973,21 @@ typedef struct RangeTblEntry * that the tuple format of the tuplestore is the same as the referenced * relation. This allows plans referencing AFTER trigger transition * tables to be invalidated if the underlying table is altered. + * + * rellockmode is really LOCKMODE, but it's declared int to avoid having + * to include lock-related headers here. It must be RowExclusiveLock if + * the RTE is an INSERT/UPDATE/DELETE target, else RowShareLock if the RTE + * is a SELECT FOR UPDATE/FOR SHARE target, else AccessShareLock. + * + * Note: in some cases, rule expansion may result in RTEs that are marked + * with RowExclusiveLock even though they are not the target of the + * current query; this happens if a DO ALSO rule simply scans the original + * target table. We leave such RTEs with their original lockmode so as to + * avoid getting an additional, lesser lock. */ Oid relid; /* OID of the relation */ char relkind; /* relation kind (see pg_class.relkind) */ + int rellockmode; /* lock level that query requires on the rel */ struct TableSampleClause *tablesample; /* sampling info, or NULL */ /* diff --git a/src/include/parser/parse_relation.h b/src/include/parser/parse_relation.h index b9792acdae..687a7d7b1b 100644 --- a/src/include/parser/parse_relation.h +++ b/src/include/parser/parse_relation.h @@ -69,6 +69,7 @@ extern RangeTblEntry *addRangeTableEntry(ParseState *pstate, bool inFromCl); extern RangeTblEntry *addRangeTableEntryForRelation(ParseState *pstate, Relation rel, + int lockmode, Alias *alias, bool inh, bool inFromCl); diff --git a/src/test/modules/test_rls_hooks/test_rls_hooks.c b/src/test/modules/test_rls_hooks/test_rls_hooks.c index cab67a60aa..d492697e88 100644 --- a/src/test/modules/test_rls_hooks/test_rls_hooks.c +++ b/src/test/modules/test_rls_hooks/test_rls_hooks.c @@ -80,8 +80,8 @@ test_rls_hooks_permissive(CmdType cmdtype, Relation relation) qual_pstate = make_parsestate(NULL); - rte = addRangeTableEntryForRelation(qual_pstate, relation, NULL, false, - false); + rte = addRangeTableEntryForRelation(qual_pstate, relation, AccessShareLock, + NULL, false, false); addRTEtoQuery(qual_pstate, rte, false, true, true); role = ObjectIdGetDatum(ACL_ID_PUBLIC); @@ -143,8 +143,8 @@ test_rls_hooks_restrictive(CmdType cmdtype, Relation relation) qual_pstate = make_parsestate(NULL); - rte = addRangeTableEntryForRelation(qual_pstate, relation, NULL, false, - false); + rte = addRangeTableEntryForRelation(qual_pstate, relation, AccessShareLock, + NULL, false, false); addRTEtoQuery(qual_pstate, rte, false, true, true); role = ObjectIdGetDatum(ACL_ID_PUBLIC);