Re: [HACKERS] GatherMerge misses to push target list
On Sat, Nov 11, 2017 at 5:05 PM, Robert Haas wrote: > On Mon, Sep 18, 2017 at 7:02 AM, Rushabh Lathia > wrote: >>> In that case, can you please mark the patch [1] as ready for committer in >>> CF app >> >> Done. > > I think this patch is mostly correct, but I think the change to > planner.c isn't quite right. ordered_rel->reltarget is just a dummy > target list that produces nothing. Instead, I think we should pass > path->pathtarget, representing the idea that whatever Gather Merge > produces as output is the same as what you put into it. > Agreed. Your change looks good to me. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GatherMerge misses to push target list
On Mon, Sep 18, 2017 at 7:02 AM, Rushabh Lathia wrote: >> In that case, can you please mark the patch [1] as ready for committer in >> CF app > > Done. I think this patch is mostly correct, but I think the change to planner.c isn't quite right. ordered_rel->reltarget is just a dummy target list that produces nothing. Instead, I think we should pass path->pathtarget, representing the idea that whatever Gather Merge produces as output is the same as what you put into it. See attached. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company pushdown-gathermerge-tlist.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GatherMerge misses to push target list
On Mon, Sep 18, 2017 at 2:48 PM, Amit Kapila wrote: > On Mon, Sep 18, 2017 at 12:46 PM, Rushabh Lathia > wrote: > > Thanks Amit for the patch. > > > > I reviewed the code changes as well as performed more testing. Patch > > looks good to me. > > > > In that case, can you please mark the patch [1] as ready for committer in > CF app > Done. > > > Here is the updated patch - where added test-case clean up. > > > > oops, missed dropping the function. > > > Thanks for the review. > > > [1] - https://commitfest.postgresql.org/15/1293/ > > -- > With Regards, > Amit Kapila. > EnterpriseDB: http://www.enterprisedb.com > -- Rushabh Lathia
Re: [HACKERS] GatherMerge misses to push target list
On Mon, Sep 18, 2017 at 12:46 PM, Rushabh Lathia wrote: > Thanks Amit for the patch. > > I reviewed the code changes as well as performed more testing. Patch > looks good to me. > In that case, can you please mark the patch [1] as ready for committer in CF app > Here is the updated patch - where added test-case clean up. > oops, missed dropping the function. Thanks for the review. [1] - https://commitfest.postgresql.org/15/1293/ -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GatherMerge misses to push target list
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 wrote: > On Wed, Sep 13, 2017 at 5:30 PM, Rushabh Lathia > wrote: > > On Wed, Sep 6, 2017 at 10:04 AM, Amit Kapila > > 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 tenk
Re: [HACKERS] GatherMerge misses to push target list
On Wed, Sep 13, 2017 at 5:30 PM, Rushabh Lathia wrote: > On Wed, Sep 6, 2017 at 10:04 AM, Amit Kapila > 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 pushdown_target_gathermerge_v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GatherMerge misses to push target list
On Wed, Sep 6, 2017 at 10:04 AM, Amit Kapila wrote: > During my recent work on costing of parallel paths [1], I noticed that > we are missing to push target list below GatherMerge in some simple > cases like below. > > Test prepration > - > create or replace function simple_func(var1 integer) returns integer > as $$ > begin > return var1 + 10; > end; > $$ language plpgsql PARALLEL SAFE; > > create table t1(c1 int, c2 char(5)); > insert into t1 values(generate_series(1,50), 'aaa'); > set parallel_setup_cost=0; > set parallel_tuple_cost=0; > set min_parallel_table_scan_size=0; > set max_parallel_workers_per_gather=4; > set cpu_operator_cost=0; set min_parallel_index_scan_size=0; > > Case-1 > - > postgres=# explain (costs off, verbose) select c1,simple_func(c1) from > t1 where c1 < 1 order by c1; > QUERY PLAN > - > Gather Merge >Output: c1, simple_func(c1) >Workers Planned: 4 >-> Parallel Index Scan using idx_t1 on public.t1 > Output: c1 > Index Cond: (t1.c1 < 1) > (6 rows) > > In the above case, I don't see any reason why we can't push the target > list expression (simple_func(c1)) down to workers. > > Case-2 > -- > set enable_indexonlyscan=off; > set enable_indexscan=off; > postgres=# explain (costs off, verbose) select c1,simple_func(c1) from > t1 where c1 < 1 order by c1; > QUERY PLAN > > Gather Merge >Output: c1, simple_func(c1) >Workers Planned: 4 >-> Sort > Output: c1 > Sort Key: t1.c1 > -> Parallel Bitmap Heap Scan on public.t1 >Output: c1 >Recheck Cond: (t1.c1 < 1) >-> Bitmap Index Scan on idx_t1 > Index Cond: (t1.c1 < 1) > (11 rows) > > It is different from above case (Case-1) because sort node can't > project, but I think adding a Result node on top of it can help to > project and serve our purpose. > > The attached patch allows pushing the target list expression in both > the cases. I think we should handle GatherMerge case in > apply_projection_to_path similar to Gather so that target list can be > pushed. Another problem was during the creation of ordered paths > where we don't allow target expressions to be pushed below gather > merge. > > Plans after patch: > > Case-1 > -- > postgres=# explain (costs off, verbose) select c1,simple_func(c1) from > t1 where c1 < 1 order by c1; > QUERY PLAN > -- > Gather Merge >Output: c1, (simple_func(c1)) >Workers Planned: 4 >-> Parallel Index Only Scan using idx_t1 on public.t1 > Output: c1, simple_func(c1) > Index Cond: (t1.c1 < 1) > (6 rows) > > Case-2 > --- > postgres=# explain (costs off, verbose) select c1,simple_func(c1) from > t1 where c1 < 1 order by c1; > QUERY PLAN > -- > Gather Merge >Output: c1, (simple_func(c1)) >Workers Planned: 4 >-> Result > Output: c1, simple_func(c1) > -> Sort >Output: c1 >Sort Key: t1.c1 >-> Parallel Bitmap Heap Scan on public.t1 > Output: c1 > Recheck Cond: (t1.c1 < 1) > -> Bitmap Index Scan on idx_t1 >Index Cond: (t1.c1 < 1) > (13 rows) > > Note, that simple_func() is pushed down to workers in both the cases. > > Thoughts? > 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. 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. > > [1] - https://www.postgresql.org/message-id/CAA4eK1Ji_ > 0pgrjFotAyvvfxGikxJQEKcxD863VQ-xYtfQBy0uQ%40mail.gmail.com Sure, once you do that - I will review the patch. Thanks, > > > -- > With Regards, > Amit Kapila. > EnterpriseDB: http://www.enterprisedb.com > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > > -- Rushabh Lathia
Re: [HACKERS] GatherMerge misses to push target list
On Wed, Sep 6, 2017 at 10:04 AM, Amit Kapila wrote: > During my recent work on costing of parallel paths [1], I noticed that > we are missing to push target list below GatherMerge in some simple > cases like below. > I think this should be considered as a bug-fix for 10.0, but it doesn't block any functionality or give wrong results, so we might consider it as an optimization for GatherMerge. In any case, I have added this to next CF. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] GatherMerge misses to push target list
During my recent work on costing of parallel paths [1], I noticed that we are missing to push target list below GatherMerge in some simple cases like below. Test prepration - create or replace function simple_func(var1 integer) returns integer as $$ begin return var1 + 10; end; $$ language plpgsql PARALLEL SAFE; create table t1(c1 int, c2 char(5)); insert into t1 values(generate_series(1,50), 'aaa'); set parallel_setup_cost=0; set parallel_tuple_cost=0; set min_parallel_table_scan_size=0; set max_parallel_workers_per_gather=4; set cpu_operator_cost=0; set min_parallel_index_scan_size=0; Case-1 - postgres=# explain (costs off, verbose) select c1,simple_func(c1) from t1 where c1 < 1 order by c1; QUERY PLAN - Gather Merge Output: c1, simple_func(c1) Workers Planned: 4 -> Parallel Index Scan using idx_t1 on public.t1 Output: c1 Index Cond: (t1.c1 < 1) (6 rows) In the above case, I don't see any reason why we can't push the target list expression (simple_func(c1)) down to workers. Case-2 -- set enable_indexonlyscan=off; set enable_indexscan=off; postgres=# explain (costs off, verbose) select c1,simple_func(c1) from t1 where c1 < 1 order by c1; QUERY PLAN Gather Merge Output: c1, simple_func(c1) Workers Planned: 4 -> Sort Output: c1 Sort Key: t1.c1 -> Parallel Bitmap Heap Scan on public.t1 Output: c1 Recheck Cond: (t1.c1 < 1) -> Bitmap Index Scan on idx_t1 Index Cond: (t1.c1 < 1) (11 rows) It is different from above case (Case-1) because sort node can't project, but I think adding a Result node on top of it can help to project and serve our purpose. The attached patch allows pushing the target list expression in both the cases. I think we should handle GatherMerge case in apply_projection_to_path similar to Gather so that target list can be pushed. Another problem was during the creation of ordered paths where we don't allow target expressions to be pushed below gather merge. Plans after patch: Case-1 -- postgres=# explain (costs off, verbose) select c1,simple_func(c1) from t1 where c1 < 1 order by c1; QUERY PLAN -- Gather Merge Output: c1, (simple_func(c1)) Workers Planned: 4 -> Parallel Index Only Scan using idx_t1 on public.t1 Output: c1, simple_func(c1) Index Cond: (t1.c1 < 1) (6 rows) Case-2 --- postgres=# explain (costs off, verbose) select c1,simple_func(c1) from t1 where c1 < 1 order by c1; QUERY PLAN -- Gather Merge Output: c1, (simple_func(c1)) Workers Planned: 4 -> Result Output: c1, simple_func(c1) -> Sort Output: c1 Sort Key: t1.c1 -> Parallel Bitmap Heap Scan on public.t1 Output: c1 Recheck Cond: (t1.c1 < 1) -> Bitmap Index Scan on idx_t1 Index Cond: (t1.c1 < 1) (13 rows) Note, that simple_func() is pushed down to workers in both the cases. Thoughts? Note - If we agree on the problems and fix, then I can add regression tests to cover above cases in the patch. [1] - https://www.postgresql.org/message-id/CAA4eK1Ji_0pgrjFotAyvvfxGikxJQEKcxD863VQ-xYtfQBy0uQ%40mail.gmail.com -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com pushdown_target_gathermerge_v1.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers