Fix incorrect logic for choosing the next Parallel Append subplan

In 499be013de support for pruning unneeded Append subnodes was added.
The logic in that commit was not correctly checking if the next subplan
was in fact a valid subplan. This could cause parallel workers processes
to be given a subplan to work on which didn't require any work.

Per code review following an otherwise unexplained regression failure in
buildfarm member Pademelon.  (We haven't been able to reproduce the
failure, so this is a bit of a blind fix in terms of whether it'll
actually fix it; but it is a clear bug nonetheless).

In passing, also add a comment to explain what first_partial_plan means.

Author: David Rowley
Discussion: https://postgr.es/m/CAKJS1f_E5r05hHUVG3UmCQJ49DGKKHtN=SHybD44LdzBn+CJng@mail.gmail.com

diff --git a/src/backend/executor/nodeAppend.c b/src/backend/executor/nodeAppend.c
index b135b61324efd700afe34a1a7164dce37d1f70ba..d062cfddaccaaedb2e5b1e84ebbc5ba4724005d8 100644 (file)
--- a/src/backend/executor/nodeAppend.c
+++ b/src/backend/executor/nodeAppend.c
@@ -547,6 +547,11 @@ choose_next_subplan_for_leader(AppendState *node)
                        LWLockRelease(&pstate->pa_lock);
                        return false;
                }
+
+               /*
+                * We needn't pay attention to as_valid_subplans here as all invalid
+                * plans have been marked as finished.
+                */
                node->as_whichplan--;
        }
 
@@ -612,18 +617,28 @@ choose_next_subplan_for_worker(AppendState *node)
        /* Save the plan from which we are starting the search. */
        node->as_whichplan = pstate->pa_next_plan;
 
-       /* Loop until we find a subplan to execute. */
+       /* Loop until we find a valid subplan to execute. */
        while (pstate->pa_finished[pstate->pa_next_plan])
        {
-               if (pstate->pa_next_plan < node->as_nplans - 1)
+               int                     nextplan;
+
+               nextplan = bms_next_member(node->as_valid_subplans,
+                                                                  pstate->pa_next_plan);
+               if (nextplan >= 0)
                {
-                       /* Advance to next plan. */
-                       pstate->pa_next_plan++;
+                       /* Advance to the next valid plan. */
+                       pstate->pa_next_plan = nextplan;
                }
                else if (node->as_whichplan > append->first_partial_plan)
                {
-                       /* Loop back to first partial plan. */
-                       pstate->pa_next_plan = append->first_partial_plan;
+                       /*
+                        * Try looping back to the first valid partial plan, if there is
+                        * one.  If there isn't, arrange to bail out below.
+                        */
+                       nextplan = bms_next_member(node->as_valid_subplans,
+                                                                          append->first_partial_plan - 1);
+                       pstate->pa_next_plan =
+                               nextplan < 0 ? node->as_whichplan : nextplan;
                }
                else
                {
@@ -644,16 +659,27 @@ choose_next_subplan_for_worker(AppendState *node)
        }
 
        /* Pick the plan we found, and advance pa_next_plan one more time. */
-       node->as_whichplan = pstate->pa_next_plan++;
-       if (pstate->pa_next_plan >= node->as_nplans)
+       node->as_whichplan = pstate->pa_next_plan;
+       pstate->pa_next_plan = bms_next_member(node->as_valid_subplans,
+                                                                                  pstate->pa_next_plan);
+
+       /*
+        * If there are no more valid plans then try setting the next plan to the
+        * first valid partial plan.
+        */
+       if (pstate->pa_next_plan < 0)
        {
-               if (append->first_partial_plan < node->as_nplans)
-                       pstate->pa_next_plan = append->first_partial_plan;
+               int                     nextplan = bms_next_member(node->as_valid_subplans,
+                                                                                          append->first_partial_plan - 1);
+
+               if (nextplan >= 0)
+                       pstate->pa_next_plan = nextplan;
                else
                {
                        /*
-                        * We have only non-partial plans, and we already chose the last
-                        * one; so arrange for the other workers to immediately bail out.
+                        * There are no valid partial plans, and we already chose the last
+                        * non-partial plan; so flag that there's nothing more for our
+                        * fellow workers to do.
                         */
                        pstate->pa_next_plan = INVALID_SUBPLAN_INDEX;
                }

diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index c3e5c2c79f87975f7db94c0a534592ef42c08cf0..c5c33cd336043b7c4683cafc4890aac799a56e45 100644 (file)
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -255,6 +255,11 @@ typedef struct Append
        /* RT indexes of non-leaf tables in a partition tree */
        List       *partitioned_rels;
        List       *appendplans;
+
+       /*
+        * All 'appendplans' preceding this index are non-partial plans. All
+        * 'appendplans' from this index onwards are partial plans.
+        */
        int                     first_partial_plan;
 
        /*