From 44ae64c388bde6e4b077272570c84dedfb17bed3 Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Mon, 13 Nov 2017 16:33:56 -0500 Subject: [PATCH] Push target list evaluation through Gather Merge. We already do this for Gather, but it got overlooked for Gather Merge. Amit Kapila, with review and minor revisions by Rushabh Lathia and by me. Discussion: http://postgr.es/m/CAA4eK1KUC5Uyu7qaifxrjpHxbSeoQh3yzwN3bThnJsmJcZ-qtA@mail.gmail.com --- src/backend/optimizer/plan/planner.c | 3 +- src/backend/optimizer/util/pathnode.c | 45 ++++++++++++------- src/test/regress/expected/select_parallel.out | 25 +++++++++++ src/test/regress/sql/select_parallel.sql | 13 ++++++ 4 files changed, 69 insertions(+), 17 deletions(-) diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 607f7cd251..90fd9cc959 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -5060,7 +5060,8 @@ create_ordered_paths(PlannerInfo *root, path = (Path *) create_gather_merge_path(root, ordered_rel, path, - target, root->sort_pathkeys, NULL, + path->pathtarget, + 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 36ec025b05..68dee0f501 100644 --- a/src/backend/optimizer/util/pathnode.c +++ b/src/backend/optimizer/util/pathnode.c @@ -2368,9 +2368,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 @@ -2407,31 +2407,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 ac9ad0668d..6f04769e3e 100644 --- a/src/test/regress/expected/select_parallel.out +++ b/src/test/regress/expected/select_parallel.out @@ -375,6 +375,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 495f0335dc..9c1b87abdf 100644 --- a/src/test/regress/sql/select_parallel.sql +++ b/src/test/regress/sql/select_parallel.sql @@ -144,6 +144,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; -- 2.40.0