Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit7b67a0a

Browse files
committed
Fix some interrelated planner issues with initPlans and Param munging.
In commit68fa28f I tried to teach SS_finalize_plan() to cope withinitPlans attached anywhere in the plan tree, by dint of moving itshandling of those into the recursion in finalize_plan(). It turns out thatthat doesn't really work: if a lower-level plan node emits an initPlanoutput parameter in its targetlist, it's legitimate for upper levels toreference those Params --- and at the point where this code runs, thosereferences look just like the Param itself, so finalize_plan() quiteproperly rejects them as being in the wrong place. We could lobotomizethe checks enough to allow that, probably, but then it's not clear thatwe'd have any meaningful check for misplaced Params at all. What seemsbetter, at least in the near term, is to tweak standard_planner() a bitso that initPlans are never placed anywhere but the topmost plan nodefor a query level, restoring the behavior that occurred pre-9.6. Possiblywe can do better if this code is ever merged into setrefs.c: then it wouldbe possible to check a Param's placement only when we'd failed to replaceit with a Var referencing a child plan node's targetlist.BTW, I'm now suspicious that finalize_plan is doing the wrong thing byreturning the node's allParam rather than extParam to be incorporatedin the parent node's set of used parameters. However, it makes nodifference given that initPlans only appear at top level, so I'll leavethat alone for now.Another thing that emerged from this is that standard_planner() needsto check for initPlans before deciding that it's safe to stick a Gathernode on top in force_parallel_mode mode. We previously guarded againstthat by deciding the plan wasn't wholePlanParallelSafe if any subplanshad been found, but after commit5ce5e4a it's necessary to have thissubstitute test, because path parallel_safe markings don't account forinitPlans. (Normally, we'd have decided the paths weren't safe anywaydue to appearances of SubPlan nodes, Params, or CTE scans somewhere inthe tree --- but it's possible for those all to be optimized away whileinitPlans still remain.)Per fuzz testing by Andreas Seltenreich.Report: <874m89rw7x.fsf@credativ.de>
1 parent48bfeb2 commit7b67a0a

File tree

5 files changed

+122
-9
lines changed

5 files changed

+122
-9
lines changed

‎src/backend/optimizer/plan/createplan.c

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -314,9 +314,10 @@ create_plan(PlannerInfo *root, Path *best_path)
314314

315315
/*
316316
* Attach any initPlans created in this query level to the topmost plan
317-
* node. (The initPlans could actually go in any plan node at or above
318-
* where they're referenced, but there seems no reason to put them any
319-
* lower than the topmost node for the query level.)
317+
* node. (In principle the initplans could go in any plan node at or
318+
* above where they're referenced, but there seems no reason to put them
319+
* any lower than the topmost node for the query level. Also, see
320+
* comments for SS_finalize_plan before you try to change this.)
320321
*/
321322
SS_attach_initplans(root,plan);
322323

‎src/backend/optimizer/plan/planner.c

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -305,15 +305,33 @@ standard_planner(Query *parse, int cursorOptions, ParamListInfo boundParams)
305305
if (cursorOptions&CURSOR_OPT_SCROLL)
306306
{
307307
if (!ExecSupportsBackwardScan(top_plan))
308-
top_plan=materialize_finished_plan(top_plan);
308+
{
309+
Plan*sub_plan=top_plan;
310+
311+
top_plan=materialize_finished_plan(sub_plan);
312+
313+
/*
314+
* XXX horrid kluge: if there are any initPlans attached to the
315+
* formerly-top plan node, move them up to the Material node. This
316+
* prevents failure in SS_finalize_plan, which see for comments.
317+
* We don't bother adjusting the sub_plan's cost estimate for
318+
* this.
319+
*/
320+
top_plan->initPlan=sub_plan->initPlan;
321+
sub_plan->initPlan=NIL;
322+
}
309323
}
310324

311325
/*
312326
* Optionally add a Gather node for testing purposes, provided this is
313-
* actually a safe thing to do.
327+
* actually a safe thing to do. (Note: we assume adding a Material node
328+
* above did not change the parallel safety of the plan, so we can still
329+
* rely on best_path->parallel_safe. However, that flag doesn't account
330+
* for initPlans, which render the plan parallel-unsafe.)
314331
*/
315-
if (best_path->parallel_safe&&
316-
force_parallel_mode!=FORCE_PARALLEL_OFF)
332+
if (force_parallel_mode!=FORCE_PARALLEL_OFF&&
333+
best_path->parallel_safe&&
334+
top_plan->initPlan==NIL)
317335
{
318336
Gather*gather=makeNode(Gather);
319337

‎src/backend/optimizer/plan/subselect.c

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2187,7 +2187,7 @@ SS_charge_for_initplans(PlannerInfo *root, RelOptInfo *final_rel)
21872187
*
21882188
* Attach any initplans created in the current query level to the specified
21892189
* plan node, which should normally be the topmost node for the query level.
2190-
* (TheinitPlans could actually go in any node at or above where they're
2190+
* (In principle theinitPlans could go in any node at or above where they're
21912191
* referenced; but there seems no reason to put them any lower than the
21922192
* topmost node, so we don't bother to track exactly where they came from.)
21932193
* 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)
22262226
* recursion.
22272227
*
22282228
* The return value is the computed allParam set for the given Plan node.
2229-
* This is just an internal notational convenience.
2229+
* This is just an internal notational convenience: we can add a child
2230+
* plan's allParams to the set of param IDs of interest to this level
2231+
* in the same statement that recurses to that child.
22302232
*
22312233
* Do not scribble on caller's values of valid_params or scan_params!
2234+
*
2235+
* Note: although we attempt to deal with initPlans anywhere in the tree, the
2236+
* logic is not really right. The problem is that a plan node might return an
2237+
* output Param of its initPlan as a targetlist item, in which case it's valid
2238+
* for the parent plan level to reference that same Param; the parent's usage
2239+
* will be converted into a Var referencing the child plan node by setrefs.c.
2240+
* But this function would see the parent's reference as out of scope and
2241+
* complain about it. For now, this does not matter because the planner only
2242+
* attaches initPlans to the topmost plan node in a query level, so the case
2243+
* doesn't arise. If we ever merge this processing into setrefs.c, maybe it
2244+
* can be handled more cleanly.
22322245
*/
22332246
staticBitmapset*
22342247
finalize_plan(PlannerInfo*root,Plan*plan,Bitmapset*valid_params,

‎src/test/regress/expected/portals.out

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1285,3 +1285,68 @@ fetch all from c;
12851285
(3 rows)
12861286

12871287
rollback;
1288+
-- Check handling of non-backwards-scan-capable plans with scroll cursors
1289+
begin;
1290+
explain (costs off) declare c1 cursor for select (select 42) as x;
1291+
QUERY PLAN
1292+
---------------------------
1293+
Result
1294+
InitPlan 1 (returns $0)
1295+
-> Result
1296+
(3 rows)
1297+
1298+
explain (costs off) declare c1 scroll cursor for select (select 42) as x;
1299+
QUERY PLAN
1300+
---------------------------
1301+
Materialize
1302+
InitPlan 1 (returns $0)
1303+
-> Result
1304+
-> Result
1305+
(4 rows)
1306+
1307+
declare c1 scroll cursor for select (select 42) as x;
1308+
fetch all in c1;
1309+
x
1310+
----
1311+
42
1312+
(1 row)
1313+
1314+
fetch backward all in c1;
1315+
x
1316+
----
1317+
42
1318+
(1 row)
1319+
1320+
rollback;
1321+
begin;
1322+
explain (costs off) declare c2 cursor for select generate_series(1,3) as g;
1323+
QUERY PLAN
1324+
------------
1325+
Result
1326+
(1 row)
1327+
1328+
explain (costs off) declare c2 scroll cursor for select generate_series(1,3) as g;
1329+
QUERY PLAN
1330+
--------------
1331+
Materialize
1332+
-> Result
1333+
(2 rows)
1334+
1335+
declare c2 scroll cursor for select generate_series(1,3) as g;
1336+
fetch all in c2;
1337+
g
1338+
---
1339+
1
1340+
2
1341+
3
1342+
(3 rows)
1343+
1344+
fetch backward all in c2;
1345+
g
1346+
---
1347+
3
1348+
2
1349+
1
1350+
(3 rows)
1351+
1352+
rollback;

‎src/test/regress/sql/portals.sql

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -484,3 +484,19 @@ fetch all from c;
484484
move backward allin c;
485485
fetch allfrom c;
486486
rollback;
487+
488+
-- Check handling of non-backwards-scan-capable plans with scroll cursors
489+
begin;
490+
explain (costs off) declare c1 cursor forselect (select42)as x;
491+
explain (costs off) declare c1 scroll cursor forselect (select42)as x;
492+
declare c1 scroll cursor forselect (select42)as x;
493+
fetch allin c1;
494+
fetch backward allin c1;
495+
rollback;
496+
begin;
497+
explain (costs off) declare c2 cursor forselect generate_series(1,3)as g;
498+
explain (costs off) declare c2 scroll cursor forselect generate_series(1,3)as g;
499+
declare c2 scroll cursor forselect generate_series(1,3)as g;
500+
fetch allin c2;
501+
fetch backward allin c2;
502+
rollback;

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp