From a8a2d82d15991968d921339e6e7d63030cf61bce Mon Sep 17 00:00:00 2001 From: amit Date: Thu, 19 Jul 2018 16:14:51 +0900 Subject: [PATCH v7 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 | 3 +- src/backend/commands/trigger.c | 6 +- src/backend/commands/view.c | 6 +- src/backend/executor/execMain.c | 275 ++++++++--------------- src/backend/executor/execPartition.c | 4 +- src/backend/executor/execUtils.c | 24 +- src/backend/executor/nodeLockRows.c | 4 +- src/backend/executor/nodeModifyTable.c | 42 +++- 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 | 17 +- 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 | 15 ++ src/backend/storage/lmgr/lock.c | 24 ++ src/backend/utils/cache/plancache.c | 30 +-- src/include/executor/executor.h | 2 +- src/include/nodes/parsenodes.h | 5 + src/include/parser/parse_relation.h | 4 +- src/include/storage/lmgr.h | 1 + src/include/storage/lock.h | 1 + src/test/modules/test_rls_hooks/test_rls_hooks.c | 4 +- 30 files changed, 261 insertions(+), 283 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 d06662bd32..0c4a7b6f4f 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; + LOCKMODE 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..2949b4e6d7 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, + AccessExclusiveLock); 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, AccessExclusiveLock); 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 29d8353779..5afa54389a 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, 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 36778b6f4d..34f9db93d5 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -578,11 +578,13 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString, */ rte = addRangeTableEntryForRelation(pstate, rel, makeAlias("old", NIL), - false, false); + false, false, + ShareRowExclusiveLock); addRTEtoQuery(pstate, rte, false, true, true); rte = addRangeTableEntryForRelation(pstate, rel, makeAlias("new", NIL), - false, false); + false, false, + ShareRowExclusiveLock); 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 5443cbf67d..a9edc90a52 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -52,10 +52,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" @@ -835,99 +837,49 @@ 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 the necessary lock on the relation + * has already been taken. That can only however be true in the + * parallel leader. + */ + Assert(rte->lockmode != NoLock); + if (!IsParallelWorker() && + !CheckRelationLockedByUs(rte->relid, rte->lockmode)) + elog(WARNING, "InitPlan: lock on \"%s\" not already taken", + get_rel_name(rte->relid)); + } + } +#endif + /* * initialize the node's execution state */ estate->es_range_table = rangeTable; estate->es_plannedstmt = plannedstmt; - /* - * initialize result relation stuff, and open/lock the result rels. - * - * We must do this before initializing the plan tree, else we might try to - * do a lock upgrade if a result rel is also a source rel. - */ + /* Allocate memory required for result relations */ 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 { @@ -941,73 +893,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. @@ -1614,8 +1500,6 @@ ExecPostprocessPlan(EState *estate) static void ExecEndPlan(PlanState *planstate, EState *estate) { - ResultRelInfo *resultRelInfo; - int i; ListCell *l; /* @@ -1641,26 +1525,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); @@ -2421,25 +2285,64 @@ ExecUpdateLockMode(EState *estate, ResultRelInfo *relinfo) } /* - * ExecFindRowMark -- find the ExecRowMark struct for given rangetable index + * ExecBuildRowMark -- create and return an ExecRowMark struct for the given + * PlanRowMark. * - * If no such struct, either return NULL or throw error depending on missing_ok + * The created ExecRowMark is also added to the list of rowmarks in the given + * EState. */ 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; } /* @@ -3179,7 +3082,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 */ @@ -3315,6 +3218,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 ec7a5267c3..3be794b97d 100644 --- a/src/backend/executor/execPartition.c +++ b/src/backend/executor/execPartition.c @@ -1540,9 +1540,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..efbf4bd18a 100644 --- a/src/backend/executor/nodeLockRows.c +++ b/src/backend/executor/nodeLockRows.c @@ -424,8 +424,8 @@ ExecInitLockRows(LockRows *node, EState *estate, int eflags) /* safety check on size of lr_curtuples array */ Assert(rc->rti > 0 && rc->rti <= lrstate->lr_ntables); - /* find ExecRowMark and build ExecAuxRowMark */ - erm = ExecFindRowMark(estate, rc->rti, false); + /* build the ExecRowMark and ExecAuxRowMark */ + erm = ExecBuildRowMark(estate, rc); aerm = ExecBuildAuxRowMark(erm, outerPlan->targetlist); /* diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index bf0d5e8edb..f81769ea85 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; @@ -2529,8 +2554,8 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) if (rc->isParent) continue; - /* find ExecRowMark (same for all subplans) */ - erm = ExecFindRowMark(estate, rc->rti, false); + /* build the ExecRowMark (same for all subplans) */ + erm = ExecBuildRowMark(estate, rc); /* build ExecAuxRowMark for each subplan */ for (i = 0; i < nplans; i++) @@ -2686,20 +2711,27 @@ ExecEndModifyTable(ModifyTableState *node) { int i; - /* - * Allow any FDWs to shut down - */ + /* Perform cleanup. */ 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); } + /* 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 93f1e2c4eb..03d84aa0cd 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(lockmode); WRITE_NODE_FIELD(tablesample); break; case RTE_SUBQUERY: diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c index 519deab63a..fc006e2830 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(lockmode); READ_NODE_FIELD(tablesample); break; case RTE_SUBQUERY: diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c index c020600955..ce0262d170 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 d6b93f55df..689ac72c3f 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 60b8de0f95..cbee07aeb8 100644 --- a/src/backend/parser/parse_relation.c +++ b/src/backend/parser/parse_relation.c @@ -28,6 +28,7 @@ #include "parser/parse_enr.h" #include "parser/parse_relation.h" #include "parser/parse_type.h" +#include "storage/lmgr.h" #include "utils/builtins.h" #include "utils/lsyscache.h" #include "utils/rel.h" @@ -1217,6 +1218,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,23 +1264,36 @@ 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); Assert(pstate != NULL); +#ifdef USE_ASSERT_CHECKING + if (lockmode != NoLock && + !CheckRelationLockedByUs(RelationGetRelid(rel), lockmode)) + elog(WARNING, "lock on \"%s\" is not held", + RelationGetRelationName(rel)); +#endif + rte->rtekind = RTE_RELATION; 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..5df0348f1c 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, + AccessExclusiveLock); newrte = addRangeTableEntryForRelation(pstate, rel, makeAlias("new", NIL), - false, false); + false, false, + AccessExclusiveLock); /* 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, + AccessExclusiveLock); newrte = addRangeTableEntryForRelation(sub_pstate, rel, makeAlias("new", NIL), - false, false); + false, false, + AccessExclusiveLock); 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 327e5c33d7..6562ac60b1 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, @@ -2898,6 +2884,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 @@ -3103,7 +3090,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..2d8eae62e6 100644 --- a/src/backend/storage/lmgr/lmgr.c +++ b/src/backend/storage/lmgr/lmgr.c @@ -136,6 +136,21 @@ LockRelationOid(Oid relid, LOCKMODE lockmode) } /* + * CheckRelationLockedByUs + * + * Returns true we hold a lock on 'relid' with 'lockmode'. + */ +bool +CheckRelationLockedByUs(Oid relid, LOCKMODE lockmode) +{ + LOCKTAG tag; + + SetLocktagRelationOid(&tag, relid); + + return LocalLockExists(&tag, lockmode); +} + +/* * ConditionalLockRelationOid * * As above, but only lock if we can get the lock without blocking. diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c index 831ae62525..8c140fe4b0 100644 --- a/src/backend/storage/lmgr/lock.c +++ b/src/backend/storage/lmgr/lock.c @@ -564,6 +564,30 @@ DoLockModesConflict(LOCKMODE mode1, LOCKMODE mode2) } /* + * LocalLockExists -- check if a log with 'locktag' is held locally with + * 'lockmode' + */ +bool +LocalLockExists(const LOCKTAG *locktag, LOCKMODE lockmode) +{ + LOCALLOCKTAG localtag; + bool found; + + MemSet(&localtag, 0, sizeof(localtag)); /* must clear padding */ + localtag.lock = *locktag; + localtag.mode = lockmode; + + /* + * Find the LOCALLOCK entry for this lock and lockmode + */ + (void) hash_search(LockMethodLocalHash, + (void *) &localtag, + HASH_FIND, &found); + + return found; +} + +/* * LockHasWaiters -- look up 'locktag' and check if releasing this * lock would wake up other processes waiting for it. */ 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 62209a8f10..2a265d591d 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 @@ -976,6 +977,10 @@ 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; must + * be non-zero for all plain relation RTEs + * that will be passed to the executor via + * PlannedStmt. */ 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..abfafe5e40 100644 --- a/src/include/storage/lmgr.h +++ b/src/include/storage/lmgr.h @@ -38,6 +38,7 @@ extern void RelationInitLockInfo(Relation relation); /* Lock a relation */ extern void LockRelationOid(Oid relid, LOCKMODE lockmode); +extern bool CheckRelationLockedByUs(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/include/storage/lock.h b/src/include/storage/lock.h index ff4df7fec5..b4a71ced0b 100644 --- a/src/include/storage/lock.h +++ b/src/include/storage/lock.h @@ -540,6 +540,7 @@ extern void LockReleaseAll(LOCKMETHODID lockmethodid, bool allLocks); extern void LockReleaseSession(LOCKMETHODID lockmethodid); extern void LockReleaseCurrentOwner(LOCALLOCK **locallocks, int nlocks); extern void LockReassignCurrentOwner(LOCALLOCK **locallocks, int nlocks); +extern bool LocalLockExists(const LOCKTAG *locktag, LOCKMODE lockmode); extern bool LockHasWaiters(const LOCKTAG *locktag, LOCKMODE lockmode, bool sessionLock); extern VirtualTransactionId *GetLockConflicts(const LOCKTAG *locktag, 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