Mark a query's topmost Paths parallel-unsafe if they will have initPlans.

tglsfdc · tglsfdc · commit 474de765a800 · 2016-11-25T16:20:31.000-05:00
Andreas Seltenreich found another case where we were being too optimistic about allowing a plan to be considered parallelizable despite it containing initPlans. It seems like the real issue here is that if we know we are going to tack initPlans onto the topmost Plan node for a subquery, we had better mark that subquery's result Paths as not-parallel-safe. That fixes this problem and allows reversion of a kluge (added in commit 7b67a0a and extended in f24cf96) to not trust the parallel_safe flag at top level. Discussion: <874m2w4k5d.fsf@ex.ansel.ydns.eu>
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
@@ -326,12 +326,9 @@ standard_planner(Query *parse, int cursorOptions, ParamListInfo boundParams)
 	 * Optionally add a Gather node for testing purposes, provided this is
 	 * actually a safe thing to do.  (Note: we assume adding a Material node
 	 * above did not change the parallel safety of the plan, so we can still
-	 * rely on best_path->parallel_safe.  However, that flag doesn't account
-	 * for subplans, which we are unable to transmit to workers presently.)
+	 * rely on best_path->parallel_safe.)
 	 */
-	if (force_parallel_mode != FORCE_PARALLEL_OFF &&
-		best_path->parallel_safe &&
-		glob->subplans == NIL)
+	if (force_parallel_mode != FORCE_PARALLEL_OFF && best_path->parallel_safe)
 	{
 		Gather	   *gather = makeNode(Gather);
 
@@ -782,10 +779,10 @@ subquery_planner(PlannerGlobal *glob, Query *parse,
 	SS_identify_outer_params(root);
 
 	/*
-	 * If any initPlans were created in this query level, increment the
-	 * surviving Paths' costs to account for them.  They won't actually get
-	 * attached to the plan tree till create_plan() runs, but we want to be
-	 * sure their costs are included now.
+	 * If any initPlans were created in this query level, adjust the surviving
+	 * Paths' costs and parallel-safety flags to account for them.  The
+	 * initPlans won't actually get attached to the plan tree till
+	 * create_plan() runs, but we must include their effects now.
 	 */
 	final_rel = fetch_upper_rel(root, UPPERREL_FINAL, NULL);
 	SS_charge_for_initplans(root, final_rel);
diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c
@@ -2134,11 +2134,13 @@ SS_identify_outer_params(PlannerInfo *root)
 }
 
 /*
- * SS_charge_for_initplans - account for cost of initplans in Path costs
+ * SS_charge_for_initplans - account for initplans in Path costs & parallelism
  *
  * If any initPlans have been created in the current query level, they will
  * get attached to the Plan tree created from whichever Path we select from
- * the given rel; so increment all the rel's Paths' costs to account for them.
+ * the given rel.  Increment all that rel's Paths' costs to account for them,
+ * and make sure the paths get marked as parallel-unsafe, since we can't
+ * currently transmit initPlans to parallel workers.
  *
  * This is separate from SS_attach_initplans because we might conditionally
  * create more initPlans during create_plan(), depending on which Path we
@@ -2170,14 +2172,15 @@ SS_charge_for_initplans(PlannerInfo *root, RelOptInfo *final_rel)
 	}
 
 	/*
-	 * Now adjust the costs.
+	 * Now adjust the costs and parallel_safe flags.
 	 */
 	foreach(lc, final_rel->pathlist)
 	{
 		Path	   *path = (Path *) lfirst(lc);
 
 		path->startup_cost += initplan_cost;
 		path->total_cost += initplan_cost;
+		path->parallel_safe = false;
 	}
 
 	/* We needn't do set_cheapest() here, caller will do it */

Original file line number	Diff line number	Diff line change
`@@ -2134,11 +2134,13 @@ SS_identify_outer_params(PlannerInfo *root)`
`2134`	`2134`	`}`
`2135`	`2135`
`2136`	`2136`	`/*`
`2137`		`- * SS_charge_for_initplans - account for cost of initplans in Path costs`
	`2137`	`+ * SS_charge_for_initplans - account for initplans in Path costs & parallelism`
`2138`	`2138`	`*`
`2139`	`2139`	`* If any initPlans have been created in the current query level, they will`
`2140`	`2140`	`* get attached to the Plan tree created from whichever Path we select from`
`2141`		`- * the given rel; so increment all the rel's Paths' costs to account for them.`
	`2141`	`+ * the given rel. Increment all that rel's Paths' costs to account for them,`
	`2142`	`+ * and make sure the paths get marked as parallel-unsafe, since we can't`
	`2143`	`+ * currently transmit initPlans to parallel workers.`
`2142`	`2144`	`*`
`2143`	`2145`	`* This is separate from SS_attach_initplans because we might conditionally`
`2144`	`2146`	`* create more initPlans during create_plan(), depending on which Path we`
`@@ -2170,14 +2172,15 @@ SS_charge_for_initplans(PlannerInfo root, RelOptInfo final_rel)`
`2170`	`2172`	`}`
`2171`	`2173`
`2172`	`2174`	`/*`
`2173`		`- * Now adjust the costs.`
	`2175`	`+ * Now adjust the costs and parallel_safe flags.`
`2174`	`2176`	`*/`
`2175`	`2177`	`foreach(lc, final_rel->pathlist)`
`2176`	`2178`	`{`
`2177`	`2179`	`Path path = (Path ) lfirst(lc);`
`2178`	`2180`
`2179`	`2181`	`path->startup_cost += initplan_cost;`
`2180`	`2182`	`path->total_cost += initplan_cost;`
	`2183`	`+ path->parallel_safe = false;`
`2181`	`2184`	`}`
`2182`	`2185`
`2183`	`2186`	`/* We needn't do set_cheapest() here, caller will do it */`