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)