diff options
| -rw-r--r-- | src/backend/optimizer/plan/createplan.c | 7 | ||||
| -rw-r--r-- | src/backend/optimizer/plan/planner.c | 26 | ||||
| -rw-r--r-- | src/backend/optimizer/plan/subselect.c | 17 | ||||
| -rw-r--r-- | src/test/regress/expected/portals.out | 65 | ||||
| -rw-r--r-- | src/test/regress/sql/portals.sql | 16 |
5 files changed, 122 insertions, 9 deletions
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index 71ed6a48803..cebfe197346 100644 --- 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 index 0ece4f2aace..a66317367c3 100644 --- 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 index 0849b1d5634..a46cc108203 100644 --- 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 index 462ad231c36..3ae918a63c5 100644 --- 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 index 01c3b85da9a..a1c812e937f 100644 --- 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; |