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
Appearance settings

Commit fe6f632

Browse filesBrowse files
tglsfdctglsfdc
committed
Fix mis-planning of repeated application of a projection.
create_projection_plan contains a hidden assumption (here made explicit by an Assert) that a projection-capable Path will yield a projection-capable Plan. Unfortunately, that assumption is violated only a few lines away, by create_projection_plan itself. This means that two stacked ProjectionPaths can yield an outcome where we try to jam the upper path's tlist into a non-projection-capable child node, resulting in an invalid plan. There isn't any good reason to have stacked ProjectionPaths; indeed the whole concept is faulty, since the set of Vars/Aggs/etc needed by the upper one wouldn't necessarily be available in the output of the lower one, nor could the lower one create such values if they weren't available from its input. Hence, we can fix this by adjusting create_projection_path to strip any top-level ProjectionPath from the subpath it's given. (This amounts to saying "oh, we changed our minds about what we need to project here".) The test case added here only fails in v13 and HEAD; before that, we don't attempt to shove the Sort into the parallel part of the plan, for reasons that aren't entirely clear to me. However, all the directly-related code looks generally the same as far back as v11, where the hazard was introduced (by d7c19e6). So I've got no faith that the same type of bug doesn't exist in v11 and v12, given the right test case. Hence, back-patch the code changes, but not the irrelevant test case, into those branches. Per report from Bas Poot. Discussion: https://postgr.es/m/534fca83789c4a378c7de379e9067d4f@politie.nl
1 parent 23059a0 commit fe6f632
Copy full SHA for fe6f632

File tree

Expand file treeCollapse file tree

4 files changed

+46
-1
lines changed
Filter options
Expand file treeCollapse file tree

4 files changed

+46
-1
lines changed

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

Copy file name to clipboardExpand all lines: src/backend/optimizer/plan/createplan.c
+1Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1858,6 +1858,7 @@ create_projection_plan(PlannerInfo *root, ProjectionPath *best_path, int flags)
18581858
*/
18591859
subplan = create_plan_recurse(root, best_path->subpath,
18601860
CP_IGNORE_TLIST);
1861+
Assert(is_projection_capable_plan(subplan));
18611862
tlist = build_path_tlist(root, &best_path->path);
18621863
}
18631864
else

‎src/backend/optimizer/util/pathnode.c

Copy file name to clipboardExpand all lines: src/backend/optimizer/util/pathnode.c
+18-1Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2556,7 +2556,23 @@ create_projection_path(PlannerInfo *root,
25562556
PathTarget *target)
25572557
{
25582558
ProjectionPath *pathnode = makeNode(ProjectionPath);
2559-
PathTarget *oldtarget = subpath->pathtarget;
2559+
PathTarget *oldtarget;
2560+
2561+
/*
2562+
* We mustn't put a ProjectionPath directly above another; it's useless
2563+
* and will confuse create_projection_plan. Rather than making sure all
2564+
* callers handle that, let's implement it here, by stripping off any
2565+
* ProjectionPath in what we're given. Given this rule, there won't be
2566+
* more than one.
2567+
*/
2568+
if (IsA(subpath, ProjectionPath))
2569+
{
2570+
ProjectionPath *subpp = (ProjectionPath *) subpath;
2571+
2572+
Assert(subpp->path.parent == rel);
2573+
subpath = subpp->subpath;
2574+
Assert(!IsA(subpath, ProjectionPath));
2575+
}
25602576

25612577
pathnode->path.pathtype = T_Result;
25622578
pathnode->path.parent = rel;
@@ -2582,6 +2598,7 @@ create_projection_path(PlannerInfo *root,
25822598
* Note: in the latter case, create_projection_plan has to recheck our
25832599
* conclusion; see comments therein.
25842600
*/
2601+
oldtarget = subpath->pathtarget;
25852602
if (is_projection_capable_path(subpath) ||
25862603
equal(oldtarget->exprs, target->exprs))
25872604
{

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

Copy file name to clipboardExpand all lines: src/test/regress/expected/select_parallel.out
+23Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1128,6 +1128,29 @@ ORDER BY 1, 2, 3;
11281128
------------------------------+---------------------------+-------------+--------------
11291129
(0 rows)
11301130

1131+
EXPLAIN (VERBOSE, COSTS OFF)
1132+
SELECT generate_series(1, two), array(select generate_series(1, two))
1133+
FROM tenk1 ORDER BY tenthous;
1134+
QUERY PLAN
1135+
----------------------------------------------------------------------
1136+
ProjectSet
1137+
Output: generate_series(1, tenk1.two), (SubPlan 1), tenk1.tenthous
1138+
-> Gather Merge
1139+
Output: tenk1.two, tenk1.tenthous
1140+
Workers Planned: 4
1141+
-> Result
1142+
Output: tenk1.two, tenk1.tenthous
1143+
-> Sort
1144+
Output: tenk1.tenthous, tenk1.two
1145+
Sort Key: tenk1.tenthous
1146+
-> Parallel Seq Scan on public.tenk1
1147+
Output: tenk1.tenthous, tenk1.two
1148+
SubPlan 1
1149+
-> ProjectSet
1150+
Output: generate_series(1, tenk1.two)
1151+
-> Result
1152+
(16 rows)
1153+
11311154
-- test passing expanded-value representations to workers
11321155
CREATE FUNCTION make_some_array(int,int) returns int[] as
11331156
$$declare x int[];

‎src/test/regress/sql/select_parallel.sql

Copy file name to clipboardExpand all lines: src/test/regress/sql/select_parallel.sql
+4Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -431,6 +431,10 @@ ORDER BY 1;
431431
SELECT * FROM information_schema.foreign_data_wrapper_options
432432
ORDER BY 1, 2, 3;
433433

434+
EXPLAIN (VERBOSE, COSTS OFF)
435+
SELECT generate_series(1, two), array(select generate_series(1, two))
436+
FROM tenk1 ORDER BY tenthous;
437+
434438
-- test passing expanded-value representations to workers
435439
CREATE FUNCTION make_some_array(int,int) returns int[] as
436440
$$declare x int[];

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.