Fix parallel-safety marking when moving initplans to another node.

Our policy since commit ab77a5a45 has been that a plan node having
any initplans is automatically not parallel-safe.  (This could be
relaxed, but not today.)  clean_up_removed_plan_level neglected
this, and could attach initplans to a parallel-safe child plan
node without clearing the plan's parallel-safe flag.  That could
lead to "subplan was not initialized" errors at runtime, in case
an initplan referenced another one and only the referencing one
got transmitted to parallel workers.

The fix in clean_up_removed_plan_level is trivial enough.
materialize_finished_plan also moves initplans from one node
to another, but it's okay because it already copies the source
node's parallel_safe flag.  The other place that does this kind
of thing is standard_planner's hack to inject a top-level Gather
when debug_parallel_query is active.  But that's actually dead
code given that we're correctly enforcing the "initplans aren't
parallel safe" rule, so just replace it with an Assert that
there are no initplans.

Also improve some related comments.

Normally we'd add a regression test case for this sort of bug.
The mistake itself is already reached by existing tests, but there
is accidentally no visible problem.  The only known test case that
creates an actual failure seems too indirect and fragile to justify
keeping it as a regression test (not least because it fails to fail
in v11, though the bug is clearly present there too).

Per report from Justin Pryzby.  Back-patch to all supported branches.

Discussion: https://postgr.es/m/ZDVt6MaNWkRDO1LQ@telsasoft.com

diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index c1535cbb3f29b1ce3a3ca7602443b22620e11ed2..c588693dc43e943fcfbd5cdd3dbe87d7aadcc2e9 100644 (file)
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -431,12 +431,10 @@ standard_planner(Query *parse, const char *query_string, int cursorOptions,
        Gather     *gather = makeNode(Gather);
 
        /*
-        * If there are any initPlans attached to the formerly-top plan node,
-        * move them up to the Gather node; same as we do for Material node in
-        * materialize_finished_plan.
+        * Top plan must not have any initPlans, else it shouldn't have been
+        * marked parallel-safe.
         */
-       gather->plan.initPlan = top_plan->initPlan;
-       top_plan->initPlan = NIL;
+       Assert(top_plan->initPlan == NIL);
 
        gather->plan.targetlist = top_plan->targetlist;
        gather->plan.qual = NIL;

diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 9cef92cab241a7b33eb78cb0c2d565711d4de5a4..4830cd97a2c22d627ffac085c614ae18c01541d3 100644 (file)
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -1440,7 +1440,19 @@ trivial_subqueryscan(SubqueryScan *plan)
 static Plan *
 clean_up_removed_plan_level(Plan *parent, Plan *child)
 {
-   /* We have to be sure we don't lose any initplans */
+   /*
+    * We have to be sure we don't lose any initplans, so move any that were
+    * attached to the parent plan to the child.  If we do move any, the child
+    * is no longer parallel-safe.
+    */
+   if (parent->initPlan)
+       child->parallel_safe = false;
+
+   /*
+    * Attach plans this way so that parent's initplans are processed before
+    * any pre-existing initplans of the child.  Probably doesn't matter, but
+    * let's preserve the ordering just in case.
+    */
    child->initPlan = list_concat(parent->initPlan,
                                  child->initPlan);
 

diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c
index df4ca1291912b7aca7be1fc644fab50aef59cad7..19dd6deff65ce93c05215a65c16653f75865f317 100644 (file)
--- a/src/backend/optimizer/plan/subselect.c
+++ b/src/backend/optimizer/plan/subselect.c
@@ -2121,7 +2121,7 @@ SS_identify_outer_params(PlannerInfo *root)
  * This is separate from SS_attach_initplans because we might conditionally
  * create more initPlans during create_plan(), depending on which Path we
  * select.  However, Paths that would generate such initPlans are expected
- * to have included their cost already.
+ * to have included their cost and parallel-safety effects already.
  */
 void
 SS_charge_for_initplans(PlannerInfo *root, RelOptInfo *final_rel)
@@ -2177,8 +2177,10 @@ SS_charge_for_initplans(PlannerInfo *root, RelOptInfo *final_rel)
  * (In principle the initPlans could go in any node at or above where they're
  * referenced; but there seems no reason to put them any lower than the
  * topmost node, so we don't bother to track exactly where they came from.)
- * We do not touch the plan node's cost; the initplans should have been
- * accounted for in path costing.
+ *
+ * We do not touch the plan node's cost or parallel_safe flag.  The initplans
+ * must have been accounted for in SS_charge_for_initplans, or by any later
+ * code that adds initplans via SS_make_initplan_from_plan.
  */
 void
 SS_attach_initplans(PlannerInfo *root, Plan *plan)

diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index 43d54e8f3dc445ab27942cbca84cf639d1d57ca5..5aa6c02c44d9557c27e929047c612ae475debbdb 100644 (file)
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -3339,7 +3339,7 @@ create_minmaxagg_path(PlannerInfo *root,
    /* For now, assume we are above any joins, so no parameterization */
    pathnode->path.param_info = NULL;
    pathnode->path.parallel_aware = false;
-   /* A MinMaxAggPath implies use of subplans, so cannot be parallel-safe */
+   /* A MinMaxAggPath implies use of initplans, so cannot be parallel-safe */
    pathnode->path.parallel_safe = false;
    pathnode->path.parallel_workers = 0;
    /* Result is one unordered row */