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 af75dca87eba1de93a7d576211987d3e3979ab38..0471530208f0d29eea6afce6961d8a2c77df13c7 100644 (file)
--- 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);