Thanks Amit for the patch.
I reviewed the code changes as well as performed more testing. Patch
looks good to me.
Here is the updated patch - where added test-case clean up.
On Thu, Sep 14, 2017 at 10:02 AM, Amit Kapila <[email protected]>
wrote:
> On Wed, Sep 13, 2017 at 5:30 PM, Rushabh Lathia
> <[email protected]> wrote:
> > On Wed, Sep 6, 2017 at 10:04 AM, Amit Kapila <[email protected]>
> > wrote:
> >>
> >
> >
> > This seems like a good optimization. I tried to simulate the test given
> > in the mail, initially wasn't able to generate the exact test - as index
> > creation is missing in the test shared.
> >
>
> Oops.
>
> > I also won't consider this as bug, but its definitely good optimization
> > for GatherMerge.
> >
> >>
> >>
> >> Note - If we agree on the problems and fix, then I can add regression
> >> tests to cover above cases in the patch.
> >
> >
> > Sure, once you do that - I will review the patch.
> >
>
> The attached patch contains regression test as well.
>
> Thanks for looking into it.
>
> --
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>
--
Rushabh Lathia
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 7f146d6..b12e57b 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -5028,7 +5028,7 @@ create_ordered_paths(PlannerInfo *root,
path = (Path *)
create_gather_merge_path(root, ordered_rel,
path,
- target, root->sort_pathkeys, NULL,
+ ordered_rel->reltarget, root->sort_pathkeys, NULL,
&total_groups);
/* Add projection step if needed */
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index 26567cb..516a396 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -2354,9 +2354,9 @@ create_projection_path(PlannerInfo *root,
* knows that the given path isn't referenced elsewhere and so can be modified
* in-place.
*
- * If the input path is a GatherPath, we try to push the new target down to
- * its input as well; this is a yet more invasive modification of the input
- * path, which create_projection_path() can't do.
+ * If the input path is a GatherPath or GatherMergePath, we try to push the
+ * new target down to its input as well; this is a yet more invasive
+ * modification of the input path, which create_projection_path() can't do.
*
* Note that we mustn't change the source path's parent link; so when it is
* add_path'd to "rel" things will be a bit inconsistent. So far that has
@@ -2393,31 +2393,44 @@ apply_projection_to_path(PlannerInfo *root,
(target->cost.per_tuple - oldcost.per_tuple) * path->rows;
/*
- * If the path happens to be a Gather path, we'd like to arrange for the
- * subpath to return the required target list so that workers can help
- * project. But if there is something that is not parallel-safe in the
- * target expressions, then we can't.
+ * If the path happens to be a Gather or GatherMerge path, we'd like to
+ * arrange for the subpath to return the required target list so that
+ * workers can help project. But if there is something that is not
+ * parallel-safe in the target expressions, then we can't.
*/
- if (IsA(path, GatherPath) &&
+ if ((IsA(path, GatherPath) || IsA(path, GatherMergePath)) &&
is_parallel_safe(root, (Node *) target->exprs))
{
- GatherPath *gpath = (GatherPath *) path;
-
/*
* We always use create_projection_path here, even if the subpath is
* projection-capable, so as to avoid modifying the subpath in place.
* It seems unlikely at present that there could be any other
* references to the subpath, but better safe than sorry.
*
- * Note that we don't change the GatherPath's cost estimates; it might
+ * Note that we don't change the parallel path's cost estimates; it might
* be appropriate to do so, to reflect the fact that the bulk of the
* target evaluation will happen in workers.
*/
- gpath->subpath = (Path *)
- create_projection_path(root,
- gpath->subpath->parent,
- gpath->subpath,
- target);
+ if (IsA(path, GatherPath))
+ {
+ GatherPath *gpath = (GatherPath *) path;
+
+ gpath->subpath = (Path *)
+ create_projection_path(root,
+ gpath->subpath->parent,
+ gpath->subpath,
+ target);
+ }
+ else
+ {
+ GatherMergePath *gmpath = (GatherMergePath *) path;
+
+ gmpath->subpath = (Path *)
+ create_projection_path(root,
+ gmpath->subpath->parent,
+ gmpath->subpath,
+ target);
+ }
}
else if (path->parallel_safe &&
!is_parallel_safe(root, (Node *) target->exprs))
diff --git a/src/test/regress/expected/select_parallel.out b/src/test/regress/expected/select_parallel.out
index 2ae600f..228b29e 100644
--- a/src/test/regress/expected/select_parallel.out
+++ b/src/test/regress/expected/select_parallel.out
@@ -355,6 +355,31 @@ select count(*) from tenk1 group by twenty;
500
(20 rows)
+--test expressions in targetlist are pushed down for gather merge
+create or replace function simple_func(var1 integer) returns integer
+as $$
+begin
+ return var1 + 10;
+end;
+$$ language plpgsql PARALLEL SAFE;
+explain (costs off, verbose)
+ select ten, simple_func(ten) from tenk1 where ten < 100 order by ten;
+ QUERY PLAN
+-----------------------------------------------------
+ Gather Merge
+ Output: ten, (simple_func(ten))
+ Workers Planned: 4
+ -> Result
+ Output: ten, simple_func(ten)
+ -> Sort
+ Output: ten
+ Sort Key: tenk1.ten
+ -> Parallel Seq Scan on public.tenk1
+ Output: ten
+ Filter: (tenk1.ten < 100)
+(11 rows)
+
+drop function simple_func(integer);
--test rescan behavior of gather merge
set enable_material = false;
explain (costs off)
diff --git a/src/test/regress/sql/select_parallel.sql b/src/test/regress/sql/select_parallel.sql
index 89fe80a..a218b22 100644
--- a/src/test/regress/sql/select_parallel.sql
+++ b/src/test/regress/sql/select_parallel.sql
@@ -138,6 +138,19 @@ explain (costs off)
select count(*) from tenk1 group by twenty;
+--test expressions in targetlist are pushed down for gather merge
+create or replace function simple_func(var1 integer) returns integer
+as $$
+begin
+ return var1 + 10;
+end;
+$$ language plpgsql PARALLEL SAFE;
+
+explain (costs off, verbose)
+ select ten, simple_func(ten) from tenk1 where ten < 100 order by ten;
+
+drop function simple_func(integer);
+
--test rescan behavior of gather merge
set enable_material = false;
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
