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 d7c19e62a).  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

diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index f7cd738eeac158dc5ddf7f8522ba08140f8281c2..7bf1751e938d386dd1977b8c5e93083666e8972e 100644 (file)
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -1858,6 +1858,7 @@ create_projection_plan(PlannerInfo *root, ProjectionPath *best_path, int flags)
         */
        subplan = create_plan_recurse(root, best_path->subpath,
                                      CP_IGNORE_TLIST);
+       Assert(is_projection_capable_plan(subplan));
        tlist = build_path_tlist(root, &best_path->path);
    }
    else

diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index 5778f80c01e77de861dd58ed9572596781625e0f..d54de06a3df0eb835df5c0aa7b7da57321b9e347 100644 (file)
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -2556,7 +2556,23 @@ create_projection_path(PlannerInfo *root,
                       PathTarget *target)
 {
    ProjectionPath *pathnode = makeNode(ProjectionPath);
-   PathTarget *oldtarget = subpath->pathtarget;
+   PathTarget *oldtarget;
+
+   /*
+    * We mustn't put a ProjectionPath directly above another; it's useless
+    * and will confuse create_projection_plan.  Rather than making sure all
+    * callers handle that, let's implement it here, by stripping off any
+    * ProjectionPath in what we're given.  Given this rule, there won't be
+    * more than one.
+    */
+   if (IsA(subpath, ProjectionPath))
+   {
+       ProjectionPath *subpp = (ProjectionPath *) subpath;
+
+       Assert(subpp->path.parent == rel);
+       subpath = subpp->subpath;
+       Assert(!IsA(subpath, ProjectionPath));
+   }
 
    pathnode->path.pathtype = T_Result;
    pathnode->path.parent = rel;
@@ -2582,6 +2598,7 @@ create_projection_path(PlannerInfo *root,
     * Note: in the latter case, create_projection_plan has to recheck our
     * conclusion; see comments therein.
     */
+   oldtarget = subpath->pathtarget;
    if (is_projection_capable_path(subpath) ||
        equal(oldtarget->exprs, target->exprs))
    {

diff --git a/src/test/regress/expected/select_parallel.out b/src/test/regress/expected/select_parallel.out
index 9b0c418db715f83a2f3ddf83c91677202e741ad4..51514421a28406517338644bb508cbf65c2807a3 100644 (file)
--- a/src/test/regress/expected/select_parallel.out
+++ b/src/test/regress/expected/select_parallel.out
@@ -1128,6 +1128,29 @@ ORDER BY 1, 2, 3;
 ------------------------------+---------------------------+-------------+--------------
 (0 rows)
 
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT generate_series(1, two), array(select generate_series(1, two))
+  FROM tenk1 ORDER BY tenthous;
+                              QUERY PLAN                              
+----------------------------------------------------------------------
+ ProjectSet
+   Output: generate_series(1, tenk1.two), (SubPlan 1), tenk1.tenthous
+   ->  Gather Merge
+         Output: tenk1.two, tenk1.tenthous
+         Workers Planned: 4
+         ->  Result
+               Output: tenk1.two, tenk1.tenthous
+               ->  Sort
+                     Output: tenk1.tenthous, tenk1.two
+                     Sort Key: tenk1.tenthous
+                     ->  Parallel Seq Scan on public.tenk1
+                           Output: tenk1.tenthous, tenk1.two
+   SubPlan 1
+     ->  ProjectSet
+           Output: generate_series(1, tenk1.two)
+           ->  Result
+(16 rows)
+
 -- test passing expanded-value representations to workers
 CREATE FUNCTION make_some_array(int,int) returns int[] as
 $$declare x int[];

diff --git a/src/test/regress/sql/select_parallel.sql b/src/test/regress/sql/select_parallel.sql
index 5a01a98b268938b079c022f276c84ccb22421353..7853ae80d2ef83dbbc7cc748c50ac6d4becc2680 100644 (file)
--- a/src/test/regress/sql/select_parallel.sql
+++ b/src/test/regress/sql/select_parallel.sql
@@ -431,6 +431,10 @@ ORDER BY 1;
 SELECT * FROM information_schema.foreign_data_wrapper_options
 ORDER BY 1, 2, 3;
 
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT generate_series(1, two), array(select generate_series(1, two))
+  FROM tenk1 ORDER BY tenthous;
+
 -- test passing expanded-value representations to workers
 CREATE FUNCTION make_some_array(int,int) returns int[] as
 $$declare x int[];