Fix some interrelated planner issues with initPlans and Param munging.

tglsfdc · tglsfdc · commit 7b67a0a49c19 · 2016-07-01T20:06:05.000-04:00
In commit 68fa28f I tried to teach SS_finalize_plan() to cope with initPlans attached anywhere in the plan tree, by dint of moving its handling of those into the recursion in finalize_plan(). It turns out that that doesn't really work: if a lower-level plan node emits an initPlan output parameter in its targetlist, it's legitimate for upper levels to reference those Params --- and at the point where this code runs, those references look just like the Param itself, so finalize_plan() quite properly rejects them as being in the wrong place. We could lobotomize the checks enough to allow that, probably, but then it's not clear that we'd have any meaningful check for misplaced Params at all. What seems better, at least in the near term, is to tweak standard_planner() a bit so that initPlans are never placed anywhere but the topmost plan node for a query level, restoring the behavior that occurred pre-9.6. Possibly we can do better if this code is ever merged into setrefs.c: then it would be possible to check a Param's placement only when we'd failed to replace it with a Var referencing a child plan node's targetlist. BTW, I'm now suspicious that finalize_plan is doing the wrong thing by returning the node's allParam rather than extParam to be incorporated in the parent node's set of used parameters. However, it makes no difference given that initPlans only appear at top level, so I'll leave that alone for now. Another thing that emerged from this is that standard_planner() needs to check for initPlans before deciding that it's safe to stick a Gather node on top in force_parallel_mode mode. We previously guarded against that by deciding the plan wasn't wholePlanParallelSafe if any subplans had been found, but after commit 5ce5e4a it's necessary to have this substitute test, because path parallel_safe markings don't account for initPlans. (Normally, we'd have decided the paths weren't safe anyway due to appearances of SubPlan nodes, Params, or CTE scans somewhere in the tree --- but it's possible for those all to be optimized away while initPlans still remain.) Per fuzz testing by Andreas Seltenreich. Report: <874m89rw7x.fsf@credativ.de>
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
@@ -314,9 +314,10 @@ create_plan(PlannerInfo *root, Path *best_path)
 
 	/*
 	 * Attach any initPlans created in this query level to the topmost plan
-	 * node.  (The initPlans could actually go in any plan node at or above
-	 * where they're referenced, but there seems no reason to put them any
-	 * lower than the topmost node for the query level.)
+	 * node.  (In principle the initplans could go in any plan node at or
+	 * above where they're referenced, but there seems no reason to put them
+	 * any lower than the topmost node for the query level.  Also, see
+	 * comments for SS_finalize_plan before you try to change this.)
 	 */
 	SS_attach_initplans(root, plan);
 
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
@@ -305,15 +305,33 @@ standard_planner(Query *parse, int cursorOptions, ParamListInfo boundParams)
 	if (cursorOptions & CURSOR_OPT_SCROLL)
 	{
 		if (!ExecSupportsBackwardScan(top_plan))
-			top_plan = materialize_finished_plan(top_plan);
+		{
+			Plan	   *sub_plan = top_plan;
+
+			top_plan = materialize_finished_plan(sub_plan);
+
+			/*
+			 * XXX horrid kluge: if there are any initPlans attached to the
+			 * formerly-top plan node, move them up to the Material node. This
+			 * prevents failure in SS_finalize_plan, which see for comments.
+			 * We don't bother adjusting the sub_plan's cost estimate for
+			 * this.
+			 */
+			top_plan->initPlan = sub_plan->initPlan;
+			sub_plan->initPlan = NIL;
+		}
 	}
 
 	/*
 	 * Optionally add a Gather node for testing purposes, provided this is
-	 * actually a safe thing to do.
+	 * 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 initPlans, which render the plan parallel-unsafe.)
 	 */
-	if (best_path->parallel_safe &&
-		force_parallel_mode != FORCE_PARALLEL_OFF)
+	if (force_parallel_mode != FORCE_PARALLEL_OFF &&
+		best_path->parallel_safe &&
+		top_plan->initPlan == NIL)
 	{
 		Gather	   *gather = makeNode(Gather);
 
diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c
@@ -2187,7 +2187,7 @@ SS_charge_for_initplans(PlannerInfo *root, RelOptInfo *final_rel)
  *
  * Attach any initplans created in the current query level to the specified
  * plan node, which should normally be the topmost node for the query level.
- * (The initPlans could actually go in any node at or above where they're
+ * (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
@@ -2226,9 +2226,22 @@ SS_finalize_plan(PlannerInfo *root, Plan *plan)
  * recursion.
  *
  * The return value is the computed allParam set for the given Plan node.
- * This is just an internal notational convenience.
+ * This is just an internal notational convenience: we can add a child
+ * plan's allParams to the set of param IDs of interest to this level
+ * in the same statement that recurses to that child.
  *
  * Do not scribble on caller's values of valid_params or scan_params!
+ *
+ * Note: although we attempt to deal with initPlans anywhere in the tree, the
+ * logic is not really right.  The problem is that a plan node might return an
+ * output Param of its initPlan as a targetlist item, in which case it's valid
+ * for the parent plan level to reference that same Param; the parent's usage
+ * will be converted into a Var referencing the child plan node by setrefs.c.
+ * But this function would see the parent's reference as out of scope and
+ * complain about it.  For now, this does not matter because the planner only
+ * attaches initPlans to the topmost plan node in a query level, so the case
+ * doesn't arise.  If we ever merge this processing into setrefs.c, maybe it
+ * can be handled more cleanly.
  */
 static Bitmapset *
 finalize_plan(PlannerInfo *root, Plan *plan, Bitmapset *valid_params,
diff --git a/src/test/regress/expected/portals.out b/src/test/regress/expected/portals.out
@@ -1285,3 +1285,68 @@ fetch all from c;
 (3 rows)
 
 rollback;
+-- Check handling of non-backwards-scan-capable plans with scroll cursors
+begin;
+explain (costs off) declare c1 cursor for select (select 42) as x;
+        QUERY PLAN         
+---------------------------
+ Result
+   InitPlan 1 (returns $0)
+     ->  Result
+(3 rows)
+
+explain (costs off) declare c1 scroll cursor for select (select 42) as x;
+        QUERY PLAN         
+---------------------------
+ Materialize
+   InitPlan 1 (returns $0)
+     ->  Result
+   ->  Result
+(4 rows)
+
+declare c1 scroll cursor for select (select 42) as x;
+fetch all in c1;
+ x  
+----
+ 42
+(1 row)
+
+fetch backward all in c1;
+ x  
+----
+ 42
+(1 row)
+
+rollback;
+begin;
+explain (costs off) declare c2 cursor for select generate_series(1,3) as g;
+ QUERY PLAN 
+------------
+ Result
+(1 row)
+
+explain (costs off) declare c2 scroll cursor for select generate_series(1,3) as g;
+  QUERY PLAN  
+--------------
+ Materialize
+   ->  Result
+(2 rows)
+
+declare c2 scroll cursor for select generate_series(1,3) as g;
+fetch all in c2;
+ g 
+---
+ 1
+ 2
+ 3
+(3 rows)
+
+fetch backward all in c2;
+ g 
+---
+ 3
+ 2
+ 1
+(3 rows)
+
+rollback;
diff --git a/src/test/regress/sql/portals.sql b/src/test/regress/sql/portals.sql
@@ -484,3 +484,19 @@ fetch all from c;
 move backward all in c;
 fetch all from c;
 rollback;
+
+-- Check handling of non-backwards-scan-capable plans with scroll cursors
+begin;
+explain (costs off) declare c1 cursor for select (select 42) as x;
+explain (costs off) declare c1 scroll cursor for select (select 42) as x;
+declare c1 scroll cursor for select (select 42) as x;
+fetch all in c1;
+fetch backward all in c1;
+rollback;
+begin;
+explain (costs off) declare c2 cursor for select generate_series(1,3) as g;
+explain (costs off) declare c2 scroll cursor for select generate_series(1,3) as g;
+declare c2 scroll cursor for select generate_series(1,3) as g;
+fetch all in c2;
+fetch backward all in c2;
+rollback;
