From: Robert Haas <rhaas@postgresql.org>
Date: Fri, 6 Jun 2025 17:38:47 +0000 (-0400)
Subject: attempt to fix handling of multiple elided nodes
X-Git-Url: http://git.postgresql.org/gitweb/static/gitweb.js?a=commitdiff_plain;h=73af1f2d27428e93627ce3dd22d189959983533b;p=users%2Frhaas%2Fpostgres.git

attempt to fix handling of multiple elided nodes

also add a bunch of comments to pgpa_plan_walker
---

diff --git a/contrib/pg_plan_advice/pgpa_walker.c b/contrib/pg_plan_advice/pgpa_walker.c
index af75dca87e..0471530208 100644
--- a/contrib/pg_plan_advice/pgpa_walker.c
+++ b/contrib/pg_plan_advice/pgpa_walker.c
@@ -18,25 +18,69 @@ pgpa_plan_walker(pgpa_plan_walker_context *context, Plan *plan,
 	pgpa_join_class	class;
 	ListCell   *lc;
 	List	   *extraplans = NIL;
+	List	   *elided_nodes = NIL;
 
+	/* Find all elided nodes for this Plan node. */
 	foreach_node(ElidedNode, n, context->pstmt->elidedNodes)
 	{
-		pgpa_clumped_join *cjoin;
-
-		if (n->plan_node_id != plan->plan_node_id)
-			continue;
+		if (n->plan_node_id == plan->plan_node_id)
+			elided_nodes = lappend(elided_nodes, n);
+	}
 
-		join_unroller = NULL;
+	/* If we found any elided_nodes, handle them. */
+	if (elided_nodes != NIL)
+	{
+		/*
+		 * If there are multiple relids for the elided node, a clumped join
+		 * should be built for it exactly once. When there's a join_unroller,
+		 * the join unrolling process will have already built any necessary
+		 * clumped join for the final elided node, so throw it out.
+		 */
+		if (join_unroller != NULL)
+			elided_nodes = list_truncate(elided_nodes,
+										 list_length(elided_nodes) - 1);
+
+		/*
+		 * Every element of elided_nodes is an ElidedNode for which any
+		 * necessary pgpa_clumped_join has not yet been created. Do that here,
+		 * and attach the resulting objects directly to the context object,
+		 * since we have nowhere else to put a reference to it.
+		 */
+		foreach_node(ElidedNode, n, elided_nodes)
+		{
+			if (bms_membership(n->relids) == BMS_MULTIPLE)
+			{
+				pgpa_clumped_join *cjoin;
 
-		if (bms_membership(n->relids) != BMS_MULTIPLE)
-				continue;
+				cjoin = pgpa_build_clumped_join(plan, n);
+				context->clumped_joins = lappend(context->clumped_joins, cjoin);
+			}
+		}
 
-		cjoin = pgpa_build_clumped_join(plan, n);
-		context->clumped_joins = lappend(context->clumped_joins, cjoin);
+		/*
+		 * Because we only want to unroll joins that the planner would have
+		 * considered as part of the same join problem, nodes elided during
+		 * setrefs processing act as boundaries.
+		 *
+		 * In more detail, if an Append or MergeAppend was elided, then
+	j	 * a partitionwise join was chosen and only a single child survived;
+		 * if a SubqueryScan was elided, the subquery was separately without
+		 * flattening it into the parent. In either case, the join order and
+		 * join methods beneath the elided node should be described separately
+		 * from the join order and methods above the elided node.
+		 */
+		join_unroller = NULL;
 	}
 
+	/* Check whether the Plan node is a join, and if so, which kind. */
 	class = pgpa_get_join_class(plan);
 
+	/*
+	 * If join_unroller == NULL, then either (a) this join should be unrolled
+	 * but we must create a new join unroller to do so, or (b) this join is
+	 * clumped and we must add it to the toplevel list of clumped joins
+	 * since there's no other place to attach it, or (c) this is not a join.
+	 */
 	if (join_unroller == NULL)
 	{
  		if (class == PGPA_UNROLLED_JOIN)
@@ -53,15 +97,25 @@ pgpa_plan_walker(pgpa_plan_walker_context *context, Plan *plan,
 		}
 	}
 
+	/*
+	 * If this join is to be unrolled, pgpa_unroll_join() will return the
+	 * join unroller object that should be passed down when we recurse into
+	 * the outer and inner sides of the plan.
+	 */
 	if (join_unroller != NULL)
 		pgpa_unroll_join(context->pstmt, plan, join_unroller,
 						 &outer_join_unroller, &inner_join_unroller);
 
+	/* Recurse into the outer and inner subtrees. */
 	if (plan->lefttree != NULL)
 		pgpa_plan_walker(context, plan->lefttree, outer_join_unroller);
 	if (plan->righttree != NULL)
 		pgpa_plan_walker(context, plan->righttree, inner_join_unroller);
 
+	/*
+	 * If we created a join unroller up above, then it's also our join to
+	 * use it to build the final pgpa_unrolled_join, and to destroy the object.
+	 */
 	if (join_unroller_toplevel)
 	{
 		pgpa_unrolled_join *ujoin;
@@ -71,6 +125,10 @@ pgpa_plan_walker(pgpa_plan_walker_context *context, Plan *plan,
 		pgpa_destroy_join_unroller(join_unroller);
 	}
 
+	/*
+	 * We still need to recuse into subtrees other than the inner and outer
+	 * subplan. Here, we recurse into each initPlan.
+	 */
 	foreach(lc, plan->initPlan)
 	{
 		Plan *subplan = lfirst(lc);
@@ -78,6 +136,11 @@ pgpa_plan_walker(pgpa_plan_walker_context *context, Plan *plan,
 		pgpa_plan_walker(context, subplan, NULL);
 	}
 
+	/*
+	 * Some plan types can have additional children. Nodes like Append that
+	 * can have any number of children store them in a List; a SubqueryScan
+	 * just has a field for a single additional Plan.
+	 */
 	switch (nodeTag(plan))
 	{
 		case T_Append:
@@ -102,6 +165,7 @@ pgpa_plan_walker(pgpa_plan_walker_context *context, Plan *plan,
 			break;
 	}
 
+	/* If we found a list of extra children, iterate over it. */
 	foreach(lc, extraplans)
 	{
 		Plan *subplan = lfirst(lc);