From 52c004bd2be0b4541080b013828c2b55bd7c860b Mon Sep 17 00:00:00 2001 From: amit Date: Thu, 19 Jul 2018 16:14:51 +0900 Subject: [PATCH v5 1/4] Don't lock range table relations in the executor As noted in https://postgre.es/m/4114.1531674142%40sss.pgh.pa.us, taking relation locks inside the executor proper is redundant, because appropriate locks should've been taken by the upstream code, that is, during the parse/rewrite/plan pipeline when adding the relations to range table or by the AcquireExecutorLocks if using a cached plan. Also, InitPlan would do certain initiaization steps, such as creating result rels, ExecRowMarks, etc. only because of the concerns about locking order, which it needs not to do anymore, because we've eliminated executor locking at all. Instead create result rels and ExecRowMarks, etc. in the ExecInit* routines of the respective nodes. To indicate which lock was taken on a relation before creating a RTE_RELATION range table entry for it, add a 'lockmode' field to RangeTblEntry. It's set when the relation is opened for the first time in the parse/rewrite/plan pipeline and its range table entry created. --- src/backend/catalog/heap.c | 3 +- src/backend/commands/copy.c | 7 +- src/backend/commands/policy.c | 17 +- src/backend/commands/tablecmds.c | 2 +- src/backend/commands/trigger.c | 4 +- src/backend/commands/view.c | 6 +- src/backend/executor/execMain.c | 265 ++++++++--------------- src/backend/executor/execPartition.c | 4 +- src/backend/executor/execUtils.c | 24 +- src/backend/executor/nodeLockRows.c | 2 +- src/backend/executor/nodeModifyTable.c | 39 +++- 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/parser/analyze.c | 3 +- src/backend/parser/parse_clause.c | 3 +- src/backend/parser/parse_relation.c | 9 +- src/backend/parser/parse_utilcmd.c | 18 +- src/backend/replication/logical/tablesync.c | 2 +- src/backend/rewrite/rewriteHandler.c | 20 +- src/backend/storage/lmgr/lmgr.c | 6 +- src/backend/utils/cache/plancache.c | 30 +-- src/include/executor/executor.h | 2 +- src/include/nodes/parsenodes.h | 3 + src/include/parser/parse_relation.h | 4 +- src/include/storage/lmgr.h | 2 +- src/test/modules/test_rls_hooks/test_rls_hooks.c | 4 +- 28 files changed, 207 insertions(+), 276 deletions(-) diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index 9176f6280b..19b530ac94 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -2551,7 +2551,8 @@ AddRelationNewConstraints(Relation rel, rel, NULL, false, - true); + true, + NoLock); addRTEtoQuery(pstate, rte, true, true, true); /* diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index 9bc67ce60f..0f8be85303 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -837,16 +837,17 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt, List *attnums; ListCell *cur; RangeTblEntry *rte; + int lockmode = is_from ? RowExclusiveLock : AccessShareLock; 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, NULL, false, false, + lockmode); rte->requiredPerms = (is_from ? ACL_INSERT : ACL_SELECT); tupDesc = RelationGetDescr(rel); diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c index cee0ef915b..09a6a7f9d3 100644 --- a/src/backend/commands/policy.c +++ b/src/backend/commands/policy.c @@ -567,7 +567,8 @@ 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, NULL, false, false, + NoLock); qual_parse_rtable = qual_pstate->p_rtable; free_parsestate(qual_pstate); @@ -590,7 +591,7 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id) /* Add this rel to the parsestate's rangetable, for dependencies */ addRangeTableEntryForRelation(with_check_pstate, rel, NULL, false, - false); + false, NoLock); with_check_parse_rtable = with_check_pstate->p_rtable; free_parsestate(with_check_pstate); @@ -752,12 +753,12 @@ CreatePolicy(CreatePolicyStmt *stmt) /* Add for the regular security quals */ rte = addRangeTableEntryForRelation(qual_pstate, target_table, - NULL, false, false); + NULL, false, false, NoLock); addRTEtoQuery(qual_pstate, rte, false, true, true); /* Add for the with-check quals */ rte = addRangeTableEntryForRelation(with_check_pstate, target_table, - NULL, false, false); + NULL, false, false, NoLock); addRTEtoQuery(with_check_pstate, rte, false, true, true); qual = transformWhereClause(qual_pstate, @@ -928,7 +929,7 @@ AlterPolicy(AlterPolicyStmt *stmt) ParseState *qual_pstate = make_parsestate(NULL); rte = addRangeTableEntryForRelation(qual_pstate, target_table, - NULL, false, false); + NULL, false, false, NoLock); addRTEtoQuery(qual_pstate, rte, false, true, true); @@ -950,7 +951,7 @@ AlterPolicy(AlterPolicyStmt *stmt) ParseState *with_check_pstate = make_parsestate(NULL); rte = addRangeTableEntryForRelation(with_check_pstate, target_table, - NULL, false, false); + NULL, false, false, NoLock); addRTEtoQuery(with_check_pstate, rte, false, true, true); @@ -1097,7 +1098,7 @@ AlterPolicy(AlterPolicyStmt *stmt) /* Add this rel to the parsestate's rangetable, for dependencies */ addRangeTableEntryForRelation(qual_pstate, target_table, NULL, - false, false); + false, false, NoLock); qual_parse_rtable = qual_pstate->p_rtable; free_parsestate(qual_pstate); @@ -1138,7 +1139,7 @@ AlterPolicy(AlterPolicyStmt *stmt) /* Add this rel to the parsestate's rangetable, for dependencies */ addRangeTableEntryForRelation(with_check_pstate, target_table, NULL, - false, false); + false, false, NoLock); 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 e96512e051..dbdbbde9e8 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -13659,7 +13659,7 @@ 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, NULL, false, true, NoLock); 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 2436692eb8..ce9acef1c2 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -578,11 +578,11 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString, */ rte = addRangeTableEntryForRelation(pstate, rel, makeAlias("old", NIL), - false, false); + false, false, NoLock); addRTEtoQuery(pstate, rte, false, true, true); rte = addRangeTableEntryForRelation(pstate, rel, makeAlias("new", NIL), - false, false); + false, false, NoLock); addRTEtoQuery(pstate, rte, false, true, true); /* Transform expression. Copy to be sure we don't modify original */ diff --git a/src/backend/commands/view.c b/src/backend/commands/view.c index ffb71c0ea7..4c379fd83b 100644 --- a/src/backend/commands/view.c +++ b/src/backend/commands/view.c @@ -387,10 +387,12 @@ UpdateRangeTableOfViewParse(Oid viewOid, Query *viewParse) */ rt_entry1 = addRangeTableEntryForRelation(pstate, viewRel, makeAlias("old", NIL), - false, false); + false, false, + AccessShareLock); rt_entry2 = addRangeTableEntryForRelation(pstate, viewRel, makeAlias("new", NIL), - false, false); + false, false, + AccessShareLock); /* Must override addRangeTableEntry's default access-check flags */ rt_entry1->requiredPerms = 0; rt_entry2->requiredPerms = 0; diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index c583e020a0..f797cdfe7c 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -50,10 +50,12 @@ #include "mb/pg_wchar.h" #include "miscadmin.h" #include "optimizer/clauses.h" +#include "optimizer/prep.h" #include "parser/parsetree.h" #include "rewrite/rewriteManip.h" #include "storage/bufmgr.h" #include "storage/lmgr.h" +#include "storage/lockdefs.h" #include "tcop/utility.h" #include "utils/acl.h" #include "utils/lsyscache.h" @@ -818,6 +820,26 @@ InitPlan(QueryDesc *queryDesc, int eflags) */ ExecCheckRTPerms(rangeTable, true); +#ifdef USE_ASSERT_CHECKING + foreach(l, rangeTable) + { + RangeTblEntry *rte = lfirst(l); + + if (rte->rtekind == RTE_RELATION && OidIsValid(rte->relid)) + { + /* + * The following asserts that no new lock needed to be taken, + * meaning the upstream code already acquired the needed locks. + */ + Assert(rte->lockmode != NoLock); + if (LockRelationOid(rte->relid, rte->lockmode) && + !IsParallelWorker()) + elog(NOTICE, "InitPlan: lock on \"%s\" not already taken", + get_rel_name(rte->relid)); + } + } +#endif + /* * initialize the node's execution state */ @@ -832,85 +854,19 @@ InitPlan(QueryDesc *queryDesc, int eflags) */ if (plannedstmt->resultRelations) { - List *resultRelations = plannedstmt->resultRelations; - int numResultRelations = list_length(resultRelations); - ResultRelInfo *resultRelInfos; - ResultRelInfo *resultRelInfo; + int numResultRelations = list_length(plannedstmt->resultRelations); + int num_roots = list_length(plannedstmt->rootResultRelations); - resultRelInfos = (ResultRelInfo *) - palloc(numResultRelations * sizeof(ResultRelInfo)); - resultRelInfo = resultRelInfos; - foreach(l, resultRelations) - { - Index resultRelationIndex = lfirst_int(l); - Oid resultRelationOid; - Relation resultRelation; - - resultRelationOid = getrelid(resultRelationIndex, rangeTable); - resultRelation = heap_open(resultRelationOid, RowExclusiveLock); - - InitResultRelInfo(resultRelInfo, - resultRelation, - resultRelationIndex, - NULL, - estate->es_instrument); - resultRelInfo++; - } - estate->es_result_relations = resultRelInfos; + estate->es_result_relations = (ResultRelInfo *) + palloc(numResultRelations * sizeof(ResultRelInfo)); estate->es_num_result_relations = numResultRelations; /* es_result_relation_info is NULL except when within ModifyTable */ estate->es_result_relation_info = NULL; - /* - * In the partitioned result relation case, lock the non-leaf result - * relations too. A subset of these are the roots of respective - * partitioned tables, for which we also allocate ResultRelInfos. - */ - estate->es_root_result_relations = NULL; - estate->es_num_root_result_relations = 0; - if (plannedstmt->nonleafResultRelations) - { - int num_roots = list_length(plannedstmt->rootResultRelations); - - /* - * Firstly, build ResultRelInfos for all the partitioned table - * roots, because we will need them to fire the statement-level - * triggers, if any. - */ - resultRelInfos = (ResultRelInfo *) + /* For partitioned tables that are roots of their partition trees. */ + estate->es_root_result_relations = (ResultRelInfo *) palloc(num_roots * sizeof(ResultRelInfo)); - resultRelInfo = resultRelInfos; - foreach(l, plannedstmt->rootResultRelations) - { - Index resultRelIndex = lfirst_int(l); - Oid resultRelOid; - Relation resultRelDesc; - - resultRelOid = getrelid(resultRelIndex, rangeTable); - resultRelDesc = heap_open(resultRelOid, RowExclusiveLock); - InitResultRelInfo(resultRelInfo, - resultRelDesc, - lfirst_int(l), - NULL, - estate->es_instrument); - resultRelInfo++; - } - - estate->es_root_result_relations = resultRelInfos; - estate->es_num_root_result_relations = num_roots; - - /* Simply lock the rest of them. */ - foreach(l, plannedstmt->nonleafResultRelations) - { - Index resultRelIndex = lfirst_int(l); - - /* We locked the roots above. */ - if (!list_member_int(plannedstmt->rootResultRelations, - resultRelIndex)) - LockRelationOid(getrelid(resultRelIndex, rangeTable), - RowExclusiveLock); - } - } + estate->es_num_root_result_relations = num_roots; } else { @@ -924,73 +880,7 @@ InitPlan(QueryDesc *queryDesc, int eflags) estate->es_num_root_result_relations = 0; } - /* - * Similarly, we have to lock relations selected FOR [KEY] UPDATE/SHARE - * before we initialize the plan tree, else we'd be risking lock upgrades. - * While we are at it, build the ExecRowMark list. Any partitioned child - * tables are ignored here (because isParent=true) and will be locked by - * the first Append or MergeAppend node that references them. (Note that - * the RowMarks corresponding to partitioned child tables are present in - * the same list as the rest, i.e., plannedstmt->rowMarks.) - */ estate->es_rowMarks = NIL; - foreach(l, plannedstmt->rowMarks) - { - PlanRowMark *rc = (PlanRowMark *) lfirst(l); - Oid relid; - Relation relation; - ExecRowMark *erm; - - /* ignore "parent" rowmarks; they are irrelevant at runtime */ - if (rc->isParent) - continue; - - /* get relation's OID (will produce InvalidOid if subquery) */ - relid = getrelid(rc->rti, rangeTable); - - /* - * If you change the conditions under which rel locks are acquired - * here, be sure to adjust ExecOpenScanRelation to match. - */ - switch (rc->markType) - { - case ROW_MARK_EXCLUSIVE: - case ROW_MARK_NOKEYEXCLUSIVE: - case ROW_MARK_SHARE: - case ROW_MARK_KEYSHARE: - relation = heap_open(relid, RowShareLock); - break; - case ROW_MARK_REFERENCE: - relation = heap_open(relid, AccessShareLock); - break; - case ROW_MARK_COPY: - /* no physical table access is required */ - relation = NULL; - break; - default: - elog(ERROR, "unrecognized markType: %d", rc->markType); - relation = NULL; /* keep compiler quiet */ - break; - } - - /* Check that relation is a legal target for marking */ - if (relation) - CheckValidRowMarkRel(relation, rc->markType); - - erm = (ExecRowMark *) palloc(sizeof(ExecRowMark)); - erm->relation = relation; - erm->relid = relid; - erm->rti = rc->rti; - erm->prti = rc->prti; - erm->rowmarkId = rc->rowmarkId; - erm->markType = rc->markType; - erm->strength = rc->strength; - erm->waitPolicy = rc->waitPolicy; - erm->ermActive = false; - ItemPointerSetInvalid(&(erm->curCtid)); - erm->ermExtra = NULL; - estate->es_rowMarks = lappend(estate->es_rowMarks, erm); - } /* * Initialize the executor's tuple table to empty. @@ -1597,8 +1487,6 @@ ExecPostprocessPlan(EState *estate) static void ExecEndPlan(PlanState *planstate, EState *estate) { - ResultRelInfo *resultRelInfo; - int i; ListCell *l; /* @@ -1624,26 +1512,6 @@ ExecEndPlan(PlanState *planstate, EState *estate) */ ExecResetTupleTable(estate->es_tupleTable, false); - /* - * close the result relation(s) if any, but hold locks until xact commit. - */ - resultRelInfo = estate->es_result_relations; - for (i = estate->es_num_result_relations; i > 0; i--) - { - /* Close indices and then the relation itself */ - ExecCloseIndices(resultRelInfo); - heap_close(resultRelInfo->ri_RelationDesc, NoLock); - resultRelInfo++; - } - - /* Close the root target relation(s). */ - resultRelInfo = estate->es_root_result_relations; - for (i = estate->es_num_root_result_relations; i > 0; i--) - { - heap_close(resultRelInfo->ri_RelationDesc, NoLock); - resultRelInfo++; - } - /* likewise close any trigger target relations */ ExecCleanUpTriggerState(estate); @@ -2404,25 +2272,60 @@ ExecUpdateLockMode(EState *estate, ResultRelInfo *relinfo) } /* - * ExecFindRowMark -- find the ExecRowMark struct for given rangetable index - * - * If no such struct, either return NULL or throw error depending on missing_ok + * ExecBuildRowMark -- create ExecRowMark struct for given PlanRowMark */ ExecRowMark * -ExecFindRowMark(EState *estate, Index rti, bool missing_ok) +ExecBuildRowMark(EState *estate, PlanRowMark *rc) { - ListCell *lc; + ExecRowMark *erm; + Oid relid; + Relation relation; - foreach(lc, estate->es_rowMarks) + Assert(!rc->isParent); + + /* get relation's OID (will produce InvalidOid if subquery) */ + relid = getrelid(rc->rti, estate->es_range_table); + + switch (rc->markType) { - ExecRowMark *erm = (ExecRowMark *) lfirst(lc); - - if (erm->rti == rti) - return erm; + case ROW_MARK_EXCLUSIVE: + case ROW_MARK_NOKEYEXCLUSIVE: + case ROW_MARK_SHARE: + case ROW_MARK_KEYSHARE: + relation = heap_open(relid, NoLock); + break; + case ROW_MARK_REFERENCE: + relation = heap_open(relid, NoLock); + break; + case ROW_MARK_COPY: + /* no physical table access is required */ + relation = NULL; + break; + default: + elog(ERROR, "unrecognized markType: %d", rc->markType); + relation = NULL; /* keep compiler quiet */ + break; } - if (!missing_ok) - elog(ERROR, "failed to find ExecRowMark for rangetable index %u", rti); - return NULL; + + /* Check that relation is a legal target for marking */ + if (relation) + CheckValidRowMarkRel(relation, rc->markType); + + erm = (ExecRowMark *) palloc(sizeof(ExecRowMark)); + erm->relation = relation; + erm->relid = relid; + erm->rti = rc->rti; + erm->prti = rc->prti; + erm->rowmarkId = rc->rowmarkId; + erm->markType = rc->markType; + erm->strength = rc->strength; + erm->waitPolicy = rc->waitPolicy; + erm->ermActive = false; + ItemPointerSetInvalid(&(erm->curCtid)); + erm->ermExtra = NULL; + estate->es_rowMarks = lappend(estate->es_rowMarks, erm); + + return erm; } /* @@ -3154,7 +3057,7 @@ EvalPlanQualStart(EPQState *epqstate, EState *parentestate, Plan *planTree) } /* es_result_relation_info must NOT be copied */ /* es_trig_target_relations must NOT be copied */ - estate->es_rowMarks = parentestate->es_rowMarks; + /* es_rowMarks must NOT be copied */ estate->es_top_eflags = parentestate->es_top_eflags; estate->es_instrument = parentestate->es_instrument; /* es_auxmodifytables must NOT be copied */ @@ -3267,6 +3170,18 @@ EvalPlanQualEnd(EPQState *epqstate) ExecEndNode(subplanstate); } + /* + * close any relations selected FOR [KEY] UPDATE/SHARE, again keeping + * locks + */ + foreach(l, estate->es_rowMarks) + { + ExecRowMark *erm = (ExecRowMark *) lfirst(l); + + if (erm->relation) + heap_close(erm->relation, NoLock); + } + /* throw away the per-estate tuple table */ ExecResetTupleTable(estate->es_tupleTable, false); diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c index 1a9943c3aa..81c2f5cedc 100644 --- a/src/backend/executor/execPartition.c +++ b/src/backend/executor/execPartition.c @@ -1510,9 +1510,7 @@ ExecCreatePartitionPruneState(PlanState *planstate, /* * We need to hold a pin on the partitioned table's relcache entry * so that we can rely on its copies of the table's partition key - * and partition descriptor. We need not get a lock though; one - * should have been acquired already by InitPlan or - * ExecLockNonLeafAppendTables. + * and partition descriptor. */ context->partrel = relation_open(pinfo->reloid, NoLock); diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c index 5b3eaec80b..09942b34fb 100644 --- a/src/backend/executor/execUtils.c +++ b/src/backend/executor/execUtils.c @@ -42,6 +42,7 @@ #include "postgres.h" +#include "access/parallel.h" #include "access/relscan.h" #include "access/transam.h" #include "executor/executor.h" @@ -51,6 +52,7 @@ #include "parser/parsetree.h" #include "storage/lmgr.h" #include "utils/builtins.h" +#include "utils/lsyscache.h" #include "utils/memutils.h" #include "utils/rel.h" #include "utils/typcache.h" @@ -652,28 +654,10 @@ ExecOpenScanRelation(EState *estate, Index scanrelid, int eflags) { Relation rel; Oid reloid; - LOCKMODE lockmode; - /* - * 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. - */ - lockmode = AccessShareLock; - if (ExecRelationIsTargetRelation(estate, scanrelid)) - lockmode = NoLock; - else - { - /* Keep this check in sync with InitPlan! */ - ExecRowMark *erm = ExecFindRowMark(estate, scanrelid, true); - - if (erm != NULL && erm->relation != NULL) - lockmode = NoLock; - } - - /* Open the relation and acquire lock as needed */ + /* Open the relation. */ reloid = getrelid(scanrelid, estate->es_range_table); - rel = heap_open(reloid, lockmode); + rel = heap_open(reloid, NoLock); /* * Complain if we're attempting a scan of an unscannable relation, except diff --git a/src/backend/executor/nodeLockRows.c b/src/backend/executor/nodeLockRows.c index 30de8a95ab..8c80291a53 100644 --- a/src/backend/executor/nodeLockRows.c +++ b/src/backend/executor/nodeLockRows.c @@ -425,7 +425,7 @@ ExecInitLockRows(LockRows *node, EState *estate, int eflags) Assert(rc->rti > 0 && rc->rti <= lrstate->lr_ntables); /* find ExecRowMark and build ExecAuxRowMark */ - erm = ExecFindRowMark(estate, rc->rti, false); + erm = ExecBuildRowMark(estate, rc); aerm = ExecBuildAuxRowMark(erm, outerPlan->targetlist); /* diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index d8d89c7983..93e5bf7af6 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -46,6 +46,7 @@ #include "foreign/fdwapi.h" #include "miscadmin.h" #include "nodes/nodeFuncs.h" +#include "parser/parsetree.h" #include "storage/bufmgr.h" #include "storage/lmgr.h" #include "utils/builtins.h" @@ -2238,12 +2239,36 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) mtstate->mt_plans = (PlanState **) palloc0(sizeof(PlanState *) * nplans); mtstate->resultRelInfo = estate->es_result_relations + node->resultRelIndex; + resultRelInfo = mtstate->resultRelInfo; + foreach(l, node->resultRelations) + { + Index resultRelationIndex = lfirst_int(l); + RangeTblEntry *rte = rt_fetch(resultRelationIndex, + estate->es_range_table); + Relation rel = heap_open(rte->relid, NoLock); + + InitResultRelInfo(resultRelInfo, rel, resultRelationIndex, NULL, + estate->es_instrument); + resultRelInfo++; + } /* If modifying a partitioned table, initialize the root table info */ if (node->rootResultRelIndex >= 0) + { + Index root_rt_index = linitial_int(node->partitioned_rels); + RangeTblEntry *rte; + Relation rel; + mtstate->rootResultRelInfo = estate->es_root_result_relations + node->rootResultRelIndex; + Assert(root_rt_index > 0); + rte = rt_fetch(root_rt_index, estate->es_range_table); + rel = heap_open(rte->relid, NoLock); + InitResultRelInfo(mtstate->rootResultRelInfo, rel, root_rt_index, + NULL, estate->es_instrument); + } + mtstate->mt_arowmarks = (List **) palloc0(sizeof(List *) * nplans); mtstate->mt_nplans = nplans; @@ -2530,7 +2555,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) continue; /* find ExecRowMark (same for all subplans) */ - erm = ExecFindRowMark(estate, rc->rti, false); + erm = ExecBuildRowMark(estate, rc); /* build ExecAuxRowMark for each subplan */ for (i = 0; i < nplans; i++) @@ -2687,19 +2712,29 @@ ExecEndModifyTable(ModifyTableState *node) int i; /* - * Allow any FDWs to shut down + * close the result relation(s) if any, but hold locks until xact commit. */ for (i = 0; i < node->mt_nplans; i++) { ResultRelInfo *resultRelInfo = node->resultRelInfo + i; + /* Allow any FDWs to shut down. */ if (!resultRelInfo->ri_usesFdwDirectModify && resultRelInfo->ri_FdwRoutine != NULL && resultRelInfo->ri_FdwRoutine->EndForeignModify != NULL) resultRelInfo->ri_FdwRoutine->EndForeignModify(node->ps.state, resultRelInfo); + + /* Close indices and then the relation itself */ + ExecCloseIndices(resultRelInfo); + heap_close(resultRelInfo->ri_RelationDesc, NoLock); + resultRelInfo++; } + /* Close the root partitioned table, if any. */ + if (node->rootResultRelInfo) + heap_close(node->rootResultRelInfo->ri_RelationDesc, NoLock); + /* Close all the partitioned tables, leaf partitions, and their indices */ if (node->mt_partition_tuple_routing) ExecCleanupTupleRouting(node, node->mt_partition_tuple_routing); diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index 7c8220cf65..6d685db0ea 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(lockmode); 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..488fddb255 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(lockmode); 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 b5af904c18..53f209e170 100644 --- a/src/backend/nodes/outfuncs.c +++ b/src/backend/nodes/outfuncs.c @@ -3127,6 +3127,7 @@ _outRangeTblEntry(StringInfo str, const RangeTblEntry *node) case RTE_RELATION: WRITE_OID_FIELD(relid); WRITE_CHAR_FIELD(relkind); + WRITE_INT_FIELD(lockmode); WRITE_NODE_FIELD(tablesample); break; case RTE_SUBQUERY: diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c index 3254524223..7a1e839ef4 100644 --- a/src/backend/nodes/readfuncs.c +++ b/src/backend/nodes/readfuncs.c @@ -1350,6 +1350,7 @@ _readRangeTblEntry(void) case RTE_RELATION: READ_OID_FIELD(relid); READ_CHAR_FIELD(relkind); + READ_INT_FIELD(lockmode); READ_NODE_FIELD(tablesample); break; case RTE_SUBQUERY: diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c index c601b6d40d..d2f90c2de6 100644 --- a/src/backend/parser/analyze.c +++ b/src/backend/parser/analyze.c @@ -1038,7 +1038,8 @@ transformOnConflictClause(ParseState *pstate, exclRte = addRangeTableEntryForRelation(pstate, targetrel, makeAlias("excluded", NIL), - false, false); + false, false, + RowExclusiveLock); exclRte->relkind = RELKIND_COMPOSITE_TYPE; exclRte->requiredPerms = 0; /* other permissions fields in exclRte are already empty */ diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c index cfd4b91897..5358042f89 100644 --- a/src/backend/parser/parse_clause.c +++ b/src/backend/parser/parse_clause.c @@ -217,7 +217,8 @@ setTargetTable(ParseState *pstate, RangeVar *relation, * Now build an RTE. */ rte = addRangeTableEntryForRelation(pstate, pstate->p_target_relation, - relation->alias, inh, false); + relation->alias, inh, false, + RowExclusiveLock); pstate->p_target_rangetblentry = rte; /* assume new rte is at end */ diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c index bf5df26009..37b4c79126 100644 --- a/src/backend/parser/parse_relation.c +++ b/src/backend/parser/parse_relation.c @@ -1217,6 +1217,7 @@ addRangeTableEntry(ParseState *pstate, rel = parserOpenTable(pstate, relation, lockmode); rte->relid = RelationGetRelid(rel); rte->relkind = rel->rd_rel->relkind; + rte->lockmode = lockmode; /* * Build the list of effective column names using user-supplied aliases @@ -1262,13 +1263,18 @@ 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 taken on 'rel'. The caller may pass NoLock if the + * RTE won't be passed to the executor, that is, won't be part of a + * PlannedStmt. */ RangeTblEntry * addRangeTableEntryForRelation(ParseState *pstate, Relation rel, Alias *alias, bool inh, - bool inFromCl) + bool inFromCl, + LOCKMODE lockmode) { RangeTblEntry *rte = makeNode(RangeTblEntry); char *refname = alias ? alias->aliasname : RelationGetRelationName(rel); @@ -1279,6 +1285,7 @@ addRangeTableEntryForRelation(ParseState *pstate, rte->alias = alias; rte->relid = RelationGetRelid(rel); rte->relkind = rel->rd_rel->relkind; + rte->lockmode = 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..14adac8312 100644 --- a/src/backend/parser/parse_utilcmd.c +++ b/src/backend/parser/parse_utilcmd.c @@ -2526,7 +2526,8 @@ 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, NULL, false, true, + NoLock); /* no to join list, yes to namespaces */ addRTEtoQuery(pstate, rte, false, true, true); @@ -2636,10 +2637,12 @@ transformRuleStmt(RuleStmt *stmt, const char *queryString, */ oldrte = addRangeTableEntryForRelation(pstate, rel, makeAlias("old", NIL), - false, false); + false, false, + AccessShareLock); newrte = addRangeTableEntryForRelation(pstate, rel, makeAlias("new", NIL), - false, false); + false, false, + AccessShareLock); /* Must override addRangeTableEntry's default access-check flags */ oldrte->requiredPerms = 0; newrte->requiredPerms = 0; @@ -2734,10 +2737,12 @@ transformRuleStmt(RuleStmt *stmt, const char *queryString, */ oldrte = addRangeTableEntryForRelation(sub_pstate, rel, makeAlias("old", NIL), - false, false); + false, false, + AccessShareLock); newrte = addRangeTableEntryForRelation(sub_pstate, rel, makeAlias("new", NIL), - false, false); + false, false, + AccessShareLock); oldrte->requiredPerms = 0; newrte->requiredPerms = 0; addRTEtoQuery(sub_pstate, oldrte, false, true, false); @@ -2940,7 +2945,8 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt, rel, NULL, false, - true); + true, + NoLock); addRTEtoQuery(pstate, rte, false, true, true); /* Set up CreateStmtContext */ diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c index acc6498567..905a983074 100644 --- a/src/backend/replication/logical/tablesync.c +++ b/src/backend/replication/logical/tablesync.c @@ -795,7 +795,7 @@ copy_table(Relation rel) copybuf = makeStringInfo(); pstate = make_parsestate(NULL); - addRangeTableEntryForRelation(pstate, rel, NULL, false, false); + addRangeTableEntryForRelation(pstate, rel, NULL, false, false, NoLock); attnamelist = make_copy_attnamelist(relmapentry); cstate = BeginCopyFrom(pstate, rel, NULL, false, copy_read_data, attnamelist, NIL); diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index d830569641..d5a928671e 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -147,7 +147,6 @@ AcquireRewriteLocks(Query *parsetree, { RangeTblEntry *rte = (RangeTblEntry *) lfirst(l); Relation rel; - LOCKMODE lockmode; List *newaliasvars; Index curinputvarno; RangeTblEntry *curinputrte; @@ -162,21 +161,8 @@ AcquireRewriteLocks(Query *parsetree, * Grab the appropriate lock type for the relation, and do not * release it until end of transaction. This protects the * rewriter and planner against schema changes mid-query. - * - * Assuming forExecute is true, this logic must match what the - * executor will do, else we risk lock-upgrade deadlocks. */ - if (!forExecute) - lockmode = AccessShareLock; - else if (rt_index == parsetree->resultRelation) - lockmode = RowExclusiveLock; - else if (forUpdatePushedDown || - get_parse_rowmark(parsetree, rt_index) != NULL) - lockmode = RowShareLock; - else - lockmode = AccessShareLock; - - rel = heap_open(rte->relid, lockmode); + rel = heap_open(rte->relid, rte->lockmode); /* * While we have the relation open, update the RTE's relkind, @@ -2895,6 +2881,7 @@ rewriteTargetView(Query *parsetree, Relation view) * it changed since this view was made (cf. AcquireRewriteLocks). */ base_rte->relkind = base_rel->rd_rel->relkind; + base_rte->lockmode = RowExclusiveLock; /* * If the view query contains any sublink subqueries then we need to also @@ -3100,7 +3087,8 @@ rewriteTargetView(Query *parsetree, Relation view) base_rel, makeAlias("excluded", NIL), - false, false); + false, false, + RowExclusiveLock); new_exclRte->relkind = RELKIND_COMPOSITE_TYPE; new_exclRte->requiredPerms = 0; /* other permissions fields in new_exclRte are already empty */ diff --git a/src/backend/storage/lmgr/lmgr.c b/src/backend/storage/lmgr/lmgr.c index dc0a439638..b22d2d1457 100644 --- a/src/backend/storage/lmgr/lmgr.c +++ b/src/backend/storage/lmgr/lmgr.c @@ -100,8 +100,9 @@ SetLocktagRelationOid(LOCKTAG *tag, Oid relid) * * Lock a relation given only its OID. This should generally be used * before attempting to open the relation's relcache entry. + * Return true if we acquired a new lock, false if already held. */ -void +bool LockRelationOid(Oid relid, LOCKMODE lockmode) { LOCKTAG tag; @@ -132,7 +133,10 @@ LockRelationOid(Oid relid, LOCKMODE lockmode) { AcceptInvalidationMessages(); MarkLockClear(locallock); + return true; } + + return res == LOCKACQUIRE_ALREADY_HELD; } /* diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c index 7271b5880b..1876345de5 100644 --- a/src/backend/utils/cache/plancache.c +++ b/src/backend/utils/cache/plancache.c @@ -1493,7 +1493,6 @@ AcquireExecutorLocks(List *stmt_list, bool acquire) foreach(lc1, stmt_list) { PlannedStmt *plannedstmt = lfirst_node(PlannedStmt, lc1); - int rt_index; ListCell *lc2; if (plannedstmt->commandType == CMD_UTILITY) @@ -1512,14 +1511,9 @@ AcquireExecutorLocks(List *stmt_list, bool acquire) continue; } - rt_index = 0; foreach(lc2, plannedstmt->rtable) { RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc2); - LOCKMODE lockmode; - PlanRowMark *rc; - - rt_index++; if (rte->rtekind != RTE_RELATION) continue; @@ -1530,19 +1524,10 @@ AcquireExecutorLocks(List *stmt_list, bool acquire) * fail if it's been dropped entirely --- we'll just transiently * acquire a non-conflicting lock. */ - if (list_member_int(plannedstmt->resultRelations, rt_index) || - list_member_int(plannedstmt->nonleafResultRelations, rt_index)) - lockmode = RowExclusiveLock; - else if ((rc = get_plan_rowmark(plannedstmt->rowMarks, rt_index)) != NULL && - RowMarkRequiresRowShareLock(rc->markType)) - lockmode = RowShareLock; - else - lockmode = AccessShareLock; - if (acquire) - LockRelationOid(rte->relid, lockmode); + LockRelationOid(rte->relid, rte->lockmode); else - UnlockRelationOid(rte->relid, lockmode); + UnlockRelationOid(rte->relid, rte->lockmode); } } } @@ -1596,23 +1581,16 @@ ScanQueryForLocks(Query *parsetree, bool acquire) foreach(lc, parsetree->rtable) { RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc); - LOCKMODE lockmode; rt_index++; switch (rte->rtekind) { case RTE_RELATION: /* Acquire or release the appropriate type of lock */ - if (rt_index == parsetree->resultRelation) - lockmode = RowExclusiveLock; - else if (get_parse_rowmark(parsetree, rt_index) != NULL) - lockmode = RowShareLock; - else - lockmode = AccessShareLock; if (acquire) - LockRelationOid(rte->relid, lockmode); + LockRelationOid(rte->relid, rte->lockmode); else - UnlockRelationOid(rte->relid, lockmode); + UnlockRelationOid(rte->relid, rte->lockmode); break; case RTE_SUBQUERY: diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h index f82b51667f..4425b978f9 100644 --- a/src/include/executor/executor.h +++ b/src/include/executor/executor.h @@ -188,7 +188,7 @@ extern void ExecPartitionCheckEmitError(ResultRelInfo *resultRelInfo, extern void ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo, TupleTableSlot *slot, EState *estate); extern LockTupleMode ExecUpdateLockMode(EState *estate, ResultRelInfo *relinfo); -extern ExecRowMark *ExecFindRowMark(EState *estate, Index rti, bool missing_ok); +extern ExecRowMark *ExecBuildRowMark(EState *estate, PlanRowMark *rc); extern ExecAuxRowMark *ExecBuildAuxRowMark(ExecRowMark *erm, List *targetlist); extern TupleTableSlot *EvalPlanQual(EState *estate, EPQState *epqstate, Relation relation, Index rti, int lockmode, diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 07ab1a3dde..a0ce4109e2 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -27,6 +27,7 @@ #include "nodes/primnodes.h" #include "nodes/value.h" #include "partitioning/partdefs.h" +#include "storage/lockdefs.h" typedef enum OverridingKind @@ -977,6 +978,8 @@ typedef struct RangeTblEntry */ Oid relid; /* OID of the relation */ char relkind; /* relation kind (see pg_class.relkind) */ + LOCKMODE lockmode; /* lock level to obtain on the relation, or + * 0 if no lock required */ 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..99d1b4135c 100644 --- a/src/include/parser/parse_relation.h +++ b/src/include/parser/parse_relation.h @@ -15,6 +15,7 @@ #define PARSE_RELATION_H #include "parser/parse_node.h" +#include "storage/lockdefs.h" /* @@ -71,7 +72,8 @@ extern RangeTblEntry *addRangeTableEntryForRelation(ParseState *pstate, Relation rel, Alias *alias, bool inh, - bool inFromCl); + bool inFromCl, + LOCKMODE lockmode); extern RangeTblEntry *addRangeTableEntryForSubquery(ParseState *pstate, Query *subquery, Alias *alias, diff --git a/src/include/storage/lmgr.h b/src/include/storage/lmgr.h index a217de9716..69e6f7ffd6 100644 --- a/src/include/storage/lmgr.h +++ b/src/include/storage/lmgr.h @@ -37,7 +37,7 @@ typedef enum XLTW_Oper extern void RelationInitLockInfo(Relation relation); /* Lock a relation */ -extern void LockRelationOid(Oid relid, LOCKMODE lockmode); +extern bool LockRelationOid(Oid relid, LOCKMODE lockmode); extern bool ConditionalLockRelationOid(Oid relid, LOCKMODE lockmode); extern void UnlockRelationId(LockRelId *relid, LOCKMODE lockmode); extern void UnlockRelationOid(Oid relid, LOCKMODE lockmode); 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..d455c97ef6 100644 --- a/src/test/modules/test_rls_hooks/test_rls_hooks.c +++ b/src/test/modules/test_rls_hooks/test_rls_hooks.c @@ -81,7 +81,7 @@ test_rls_hooks_permissive(CmdType cmdtype, Relation relation) qual_pstate = make_parsestate(NULL); rte = addRangeTableEntryForRelation(qual_pstate, relation, NULL, false, - false); + false, NoLock); addRTEtoQuery(qual_pstate, rte, false, true, true); role = ObjectIdGetDatum(ACL_ID_PUBLIC); @@ -144,7 +144,7 @@ test_rls_hooks_restrictive(CmdType cmdtype, Relation relation) qual_pstate = make_parsestate(NULL); rte = addRangeTableEntryForRelation(qual_pstate, relation, NULL, false, - false); + false, NoLock); addRTEtoQuery(qual_pstate, rte, false, true, true); role = ObjectIdGetDatum(ACL_ID_PUBLIC); -- 2.11.0