Change rewriter/planner/executor/plancache to depend on RTE rellockmode.

tglsfdc · tglsfdc · commit 6e35939febf8 · 2018-10-02T14:43:09.000-04:00
Instead of recomputing the required lock levels in all these places, just use what commit fdba460 made the parser store in the RTE fields. This already simplifies the code measurably in these places, and follow-on changes will remove a bunch of no-longer-needed infrastructure. In a few cases, this change causes us to acquire a higher lock level than we did before. This is OK primarily because said higher lock level should've been acquired already at query parse time; thus, we're saving a useless extra trip through the shared lock manager to acquire a lesser lock alongside the original lock. The only known exception to this is that re-execution of a previously planned SELECT FOR UPDATE/SHARE query, for a table that uses ROW_MARK_REFERENCE or ROW_MARK_COPY methods, might have gotten only AccessShareLock before. Now it will get RowShareLock like the first execution did, which seems fine. While there's more to do, push it in this state anyway, to let the buildfarm help verify that nothing bad happened. 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
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
@@ -970,26 +970,16 @@ 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
-		 * 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:
-				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);
+				rellockmode = rt_fetch(rc->rti, rangeTable)->rellockmode;
+				relation = heap_open(relid, rellockmode);
 				break;
 			case ROW_MARK_COPY:
 				/* no physical table access is required */
diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
@@ -641,10 +641,6 @@ ExecRelationIsTargetRelation(EState *estate, Index scanrelid)
  *
  *		Open the heap relation to be scanned by a base-level scan plan node.
  *		This should be called during the node's ExecInit routine.
- *
- * By default, this acquires AccessShareLock on the relation.  However,
- * if the relation was already locked by InitPlan, we don't need to acquire
- * any additional lock.  This saves trips to the shared lock manager.
  * ----------------------------------------------------------------
  */
 Relation
@@ -654,37 +650,9 @@ ExecOpenScanRelation(EState *estate, Index scanrelid, int eflags)
 	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.
-	 *
-	 * 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 = RowExclusiveLock;
-	else
-	{
-		/* Keep this check in sync with InitPlan! */
-		ExecRowMark *erm = ExecFindRowMark(estate, scanrelid, true);
-
-		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);
+	lockmode = rt_fetch(scanrelid, estate->es_range_table)->rellockmode;
 	rel = heap_open(reloid, lockmode);
 
 	/*
@@ -912,6 +880,7 @@ ExecLockNonLeafAppendTables(List *partitioned_rels, EState *estate)
 		if (!is_result_rel)
 		{
 			PlanRowMark *rc = NULL;
+			LOCKMODE	lockmode;
 
 			foreach(l, stmt->rowMarks)
 			{
@@ -923,9 +892,13 @@ ExecLockNonLeafAppendTables(List *partitioned_rels, EState *estate)
 			}
 
 			if (rc && RowMarkRequiresRowShareLock(rc->markType))
-				LockRelationOid(relid, RowShareLock);
+				lockmode = RowShareLock;
 			else
-				LockRelationOid(relid, AccessShareLock);
+				lockmode = AccessShareLock;
+
+			Assert(lockmode == rt_fetch(rti, estate->es_range_table)->rellockmode);
+
+			LockRelationOid(relid, lockmode);
 		}
 	}
 }
diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c
@@ -1515,7 +1515,6 @@ expand_inherited_tables(PlannerInfo *root)
 static void
 expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
 {
-	Query	   *parse = root->parse;
 	Oid			parentOID;
 	PlanRowMark *oldrc;
 	Relation	oldrelation;
@@ -1546,21 +1545,9 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
 	 * relation named in the query.  However, for each child relation we add
 	 * to the query, we must obtain an appropriate lock, because this will be
 	 * the first use of those relations in the parse/rewrite/plan pipeline.
-	 *
-	 * If the parent relation is the query's result relation, then we need
-	 * RowExclusiveLock.  Otherwise, if it's accessed FOR UPDATE/SHARE, we
-	 * need RowShareLock; otherwise AccessShareLock.  We can't just grab
-	 * AccessShareLock because then the executor would be trying to upgrade
-	 * the lock, leading to possible deadlocks.  (This code should match the
-	 * parser and rewriter.)
+	 * Child rels should use the same lockmode as their parent.
 	 */
-	oldrc = get_plan_rowmark(root->rowMarks, rti);
-	if (rti == parse->resultRelation)
-		lockmode = RowExclusiveLock;
-	else if (oldrc && RowMarkRequiresRowShareLock(oldrc->markType))
-		lockmode = RowShareLock;
-	else
-		lockmode = AccessShareLock;
+	lockmode = rte->rellockmode;
 
 	/* Scan for all members of inheritance set, acquire needed locks */
 	inhOIDs = find_all_inheritors(parentOID, lockmode, NULL);
@@ -1582,6 +1569,7 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
 	 * PlanRowMark as isParent = true, and generate a new PlanRowMark for each
 	 * child.
 	 */
+	oldrc = get_plan_rowmark(root->rowMarks, rti);
 	if (oldrc)
 		oldrc->isParent = true;
 
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
@@ -87,17 +87,14 @@ static Bitmapset *adjust_view_column_set(Bitmapset *cols, List *targetlist);
  * AcquireRewriteLocks -
  *	  Acquire suitable locks on all the relations mentioned in the Query.
  *	  These locks will ensure that the relation schemas don't change under us
- *	  while we are rewriting and planning the query.
+ *	  while we are rewriting, planning, and executing 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
- * AccessShareLock on all other relations mentioned.
- *
+ * forExecute indicates that the query is about to be executed.  If so,
+ * we'll acquire the lock modes specified in the RTE rellockmode fields.
  * If forExecute is false, AccessShareLock is acquired on all relations.
  * This case is suitable for ruleutils.c, for example, where we only need
  * schema stability and we don't intend to actually modify any relations.
@@ -162,31 +159,24 @@ 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.
+				 * release it until end of transaction.  This protects the
+				 * rewriter, planner, and executor 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 is false, ignore rellockmode and just use
+				 * AccessShareLock.
 				 */
 				if (!forExecute)
 					lockmode = AccessShareLock;
-				else if (rt_index == parsetree->resultRelation)
-					lockmode = RowExclusiveLock;
 				else if (forUpdatePushedDown)
 				{
-					lockmode = RowShareLock;
 					/* Upgrade RTE's lock mode to reflect pushed-down lock */
 					if (rte->rellockmode == AccessShareLock)
 						rte->rellockmode = RowShareLock;
+					lockmode = rte->rellockmode;
 				}
-				else if (get_parse_rowmark(parsetree, rt_index) != NULL)
-					lockmode = RowShareLock;
 				else
-					lockmode = AccessShareLock;
-
-				Assert(!forExecute || lockmode == rte->rellockmode ||
-					   (lockmode == AccessShareLock &&
-						rte->rellockmode == RowExclusiveLock));
+					lockmode = rte->rellockmode;
 
 				rel = heap_open(rte->relid, lockmode);
 
diff --git 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,21 +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;
-
-			Assert(lockmode == rte->rellockmode);
-
 			if (acquire)
-				LockRelationOid(rte->relid, lockmode);
+				LockRelationOid(rte->relid, rte->rellockmode);
 			else
-				UnlockRelationOid(rte->relid, lockmode);
+				UnlockRelationOid(rte->relid, rte->rellockmode);
 		}
 	}
 }
@@ -1586,37 +1569,25 @@ static void
 ScanQueryForLocks(Query *parsetree, bool acquire)
 {
 	ListCell   *lc;
-	int			rt_index;
 
 	/* Shouldn't get called on utility commands */
 	Assert(parsetree->commandType != CMD_UTILITY);
 
 	/*
 	 * First, process RTEs of the current query level.
 	 */
-	rt_index = 0;
 	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;
-				Assert(lockmode == rte->rellockmode ||
-					   (lockmode == AccessShareLock && rte->rellockmode == RowExclusiveLock));
 				if (acquire)
-					LockRelationOid(rte->relid, lockmode);
+					LockRelationOid(rte->relid, rte->rellockmode);
 				else
-					UnlockRelationOid(rte->relid, lockmode);
+					UnlockRelationOid(rte->relid, rte->rellockmode);
 				break;
 
 			case RTE_SUBQUERY:
