Still another try at fixing scanjoin_target insertion into parallel plans.

The previous code neglected the fact that the scanjoin_target might
carry sortgroupref labelings that we need to absorb.  Instead, do
create_projection_path() unconditionally, and tweak the path's cost
estimate after the fact.  (I'm now convinced that we ought to refactor
the way we account for sometimes not needing a separate projection step,
but right now is not the time for that sort of cleanup.)

Problem identified by Amit Kapila, patch by me.

diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index dad35e56f58aa157fee38e673de4033d0a60d216..094c7082b141faf6956337a21b9561e6f7ac0030 100644 (file)
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -1788,36 +1788,39 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
                                Path       *subpath = (Path *) lfirst(lc);
                                Path       *newpath;
 
+                               /* Shouldn't have any parameterized paths anymore */
+                               Assert(subpath->param_info == NULL);
+
                                /*
                                 * We can't use apply_projection_to_path() here, because there
                                 * could already be pointers to these paths, and therefore we
-                                * cannot modify them in place.  Instead, we must use
-                                * create_projection_path().  The good news is this won't
-                                * actually insert a Result node into the final plan unless
-                                * it's needed, but the bad news is that it will charge for
-                                * the node whether it's needed or not.  Therefore, if the
-                                * target list is already what we need it to be, just leave
-                                * this partial path alone.
+                                * dare not modify them in place.  Instead, we must use
+                                * create_projection_path() unconditionally.
                                 */
-                               if (equal(scanjoin_target->exprs, subpath->pathtarget->exprs))
-                                       continue;
-
-                               Assert(subpath->param_info == NULL);
                                newpath = (Path *) create_projection_path(root,
                                                                                                                  current_rel,
                                                                                                                  subpath,
                                                                                                                  scanjoin_target);
-                               if (is_projection_capable_path(subpath))
+
+                               /*
+                                * Although create_projection_path() inserts a ProjectionPath
+                                * unconditionally, create_projection_plan() will only insert
+                                * a Result node if the subpath is not projection-capable, so
+                                * we should discount the cost of that node if it will not
+                                * actually get inserted.  (This is pretty grotty but we can
+                                * improve it later if it seems important.)
+                                */
+                               if (equal(scanjoin_target->exprs, subpath->pathtarget->exprs))
                                {
-                                       /*
-                                        * Since the target lists differ, a projection path is
-                                        * essential, but it will disappear at plan creation time
-                                        * because the subpath is projection-capable.  So avoid
-                                        * charging anything for the disappearing node.
-                                        */
+                                       /* at most we need a relabeling of the subpath */
                                        newpath->startup_cost = subpath->startup_cost;
                                        newpath->total_cost = subpath->total_cost;
                                }
+                               else if (is_projection_capable_path(subpath))
+                               {
+                                       /* need to project, but we don't need a Result */
+                                       newpath->total_cost -= cpu_tuple_cost * subpath->rows;
+                               }
 
                                lfirst(lc) = newpath;
                        }

diff --git a/src/test/regress/expected/select_parallel.out b/src/test/regress/expected/select_parallel.out
index 3c906407df2005d711663cdae2a928d4cd37394f..2286fafab399f72397440f82d8fff7a85a37ad5e 100644 (file)
--- a/src/test/regress/expected/select_parallel.out
+++ b/src/test/regress/expected/select_parallel.out
@@ -6,9 +6,10 @@ create or replace function parallel_restricted(int) returns int as
 -- Serializable isolation would disable parallel query, so explicitly use an
 -- arbitrary other level.
 begin isolation level repeatable read;
--- setup parallel test
+-- encourage use of parallel plans
 set parallel_setup_cost=0;
 set parallel_tuple_cost=0;
+set min_parallel_relation_size=0;
 set max_parallel_workers_per_gather=4;
 explain (costs off)
   select count(*) from a_star;
@@ -71,6 +72,21 @@ select length(stringu1) from tenk1 group by length(stringu1);
       6
 (1 row)
 
+explain (costs off)
+       select stringu1, count(*) from tenk1 group by stringu1 order by stringu1;
+                     QUERY PLAN                     
+----------------------------------------------------
+ Sort
+   Sort Key: stringu1
+   ->  Finalize HashAggregate
+         Group Key: stringu1
+         ->  Gather
+               Workers Planned: 4
+               ->  Partial HashAggregate
+                     Group Key: stringu1
+                     ->  Parallel Seq Scan on tenk1
+(9 rows)
+
 -- test that parallel plan for aggregates is not selected when
 -- target list contains parallel restricted clause.
 explain (costs off)

diff --git a/src/test/regress/sql/select_parallel.sql b/src/test/regress/sql/select_parallel.sql
index a8cd99333297ed445750b72116f835f96ea1039c..38d3166742c950c1357505314e93d30f67e7b5e5 100644 (file)
--- a/src/test/regress/sql/select_parallel.sql
+++ b/src/test/regress/sql/select_parallel.sql
@@ -9,9 +9,10 @@ create or replace function parallel_restricted(int) returns int as
 -- arbitrary other level.
 begin isolation level repeatable read;
 
--- setup parallel test
+-- encourage use of parallel plans
 set parallel_setup_cost=0;
 set parallel_tuple_cost=0;
+set min_parallel_relation_size=0;
 set max_parallel_workers_per_gather=4;
 
 explain (costs off)
@@ -29,6 +30,9 @@ explain (costs off)
        select length(stringu1) from tenk1 group by length(stringu1);
 select length(stringu1) from tenk1 group by length(stringu1);
 
+explain (costs off)
+       select stringu1, count(*) from tenk1 group by stringu1 order by stringu1;
+
 -- test that parallel plan for aggregates is not selected when
 -- target list contains parallel restricted clause.
 explain (costs off)