Change lock acquisition order in expand_inherited_rtentry.

robertmhaas · robertmhaas · commit f4b6341d5fcb · 2019-02-26T12:22:57.000-05:00
Previously, this function acquired locks in the order using find_all_inheritors(), which locks the children of each table that it processes in ascending OID order, and which processes the inheritance hierarchy as a whole in a breadth-first fashion. Now, it processes the inheritance hierarchy in a depth-first fashion, and at each level it proceeds in the order in which tables appear in the PartitionDesc. If table inheritance rather than table partitioning is used, the old order is preserved. This change moves the locking of any given partition much closer to the code that actually expands that partition. This seems essential if we ever want to allow concurrent DDL to add or remove partitions, because if the set of partitions can change, we must use the same data to decide which partitions to lock as we do to decide which partitions to expand; otherwise, we might expand a partition that we haven't locked. It should hopefully also facilitate efforts to postpone inheritance expansion or locking for performance reasons, because there's really no way to postpone locking some partitions if we're blindly locking them all using find_all_inheritors(). The only downside of this change which is known to me is that it further deviates from the principle that we should always lock the inheritance hierarchy in find_all_inheritors() order to avoid deadlock risk. However, we've already crossed that bridge in commit 9eefba1 and there are futher patches pending that make similar changes, so this isn't really giving up anything that we haven't surrendered already -- and it seems entirely worth it, given the performance benefits some of those changes seem likely to bring. Patch by me; thanks to David Rowley for discussion of these issues. Discussion: http://postgr.es/m/CAKJS1f_eEYVEq5tM8sm1k-HOwG0AyCPwX54XG9x4w0zy_N4Q_Q@mail.gmail.com Discussion: http://postgr.es/m/CA+TgmoZUwPf_uanjF==gTGBMJrn8uCq52XYvAEorNkLrUdoawg@mail.gmail.com
diff --git a/src/backend/optimizer/util/inherit.c b/src/backend/optimizer/util/inherit.c
@@ -124,28 +124,15 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
 
 	/*
 	 * The rewriter should already have obtained an appropriate lock on each
-	 * 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.
-	 * Child rels should use the same lockmode as their parent.
+	 * relation named in the query, so we can open the parent relation without
+	 * locking it.  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.  Child rels should
+	 * use the same lockmode as their parent.
 	 */
+	oldrelation = table_open(parentOID, NoLock);
 	lockmode = rte->rellockmode;
 
-	/* Scan for all members of inheritance set, acquire needed locks */
-	inhOIDs = find_all_inheritors(parentOID, lockmode, NULL);
-
-	/*
-	 * Check that there's at least one descendant, else treat as no-child
-	 * case.  This could happen despite above has_subclass() check, if table
-	 * once had a child but no longer does.
-	 */
-	if (list_length(inhOIDs) < 2)
-	{
-		/* Clear flag before returning */
-		rte->inh = false;
-		return;
-	}
-
 	/*
 	 * If parent relation is selected FOR UPDATE/SHARE, we need to mark its
 	 * PlanRowMark as isParent = true, and generate a new PlanRowMark for each
@@ -155,21 +142,15 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
 	if (oldrc)
 		oldrc->isParent = true;
 
-	/*
-	 * Must open the parent relation to examine its tupdesc.  We need not lock
-	 * it; we assume the rewriter already did.
-	 */
-	oldrelation = table_open(parentOID, NoLock);
-
 	/* Scan the inheritance set and expand it */
-	if (RelationGetPartitionDesc(oldrelation) != NULL)
+	if (oldrelation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
 	{
 		Assert(rte->relkind == RELKIND_PARTITIONED_TABLE);
 
 		/*
-		 * If this table has partitions, recursively expand them in the order
-		 * in which they appear in the PartitionDesc.  While at it, also
-		 * extract the partition key columns of all the partitioned tables.
+		 * If this table has partitions, recursively expand and lock them.
+		 * While at it, also extract the partition key columns of all the
+		 * partitioned tables.
 		 */
 		expand_partitioned_rtentry(root, rte, rti, oldrelation, oldrc,
 								   lockmode, &root->append_rel_list);
@@ -180,6 +161,22 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
 		RangeTblEntry *childrte;
 		Index		childRTindex;
 
+		/* Scan for all members of inheritance set, acquire needed locks */
+		inhOIDs = find_all_inheritors(parentOID, lockmode, NULL);
+
+		/*
+		 * Check that there's at least one descendant, else treat as no-child
+		 * case.  This could happen despite above has_subclass() check, if the
+		 * table once had a child but no longer does.
+		 */
+		if (list_length(inhOIDs) < 2)
+		{
+			/* Clear flag before returning */
+			rte->inh = false;
+			heap_close(oldrelation, NoLock);
+			return;
+		}
+
 		/*
 		 * This table has no partitions.  Expand any plain inheritance
 		 * children in the order the OIDs were returned by
@@ -289,8 +286,8 @@ expand_partitioned_rtentry(PlannerInfo *root, RangeTblEntry *parentrte,
 		Oid			childOID = partdesc->oids[i];
 		Relation	childrel;
 
-		/* Open rel; we already have required locks */
-		childrel = table_open(childOID, NoLock);
+		/* Open rel, acquiring required locks */
+		childrel = table_open(childOID, lockmode);
 
 		/*
 		 * Temporary partitions belonging to other sessions should have been
