Обсуждение: [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,500000), '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 < 10000 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 < 10000) (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 < 10000 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 < 10000) -> Bitmap Index Scan on idx_t1 Index Cond: (t1.c1 < 10000) (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 < 10000 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 < 10000) (6 rows) Case-2 ----------- postgres=# explain (costs off, verbose) select c1,simple_func(c1) from t1 where c1 < 10000 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 < 10000) -> Bitmap Index Scan on idx_t1 Index Cond: (t1.c1 < 10000) (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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
On Wed, Sep 6, 2017 at 10:04 AM, Amit Kapila <amit.kapila16@gmail.com> 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
On Wed, Sep 6, 2017 at 10:04 AM, Amit Kapila <amit.kapila16@gmail.com> 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,500000), '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 < 10000 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 < 10000)
(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 < 10000 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 < 10000)
-> Bitmap Index Scan on idx_t1
Index Cond: (t1.c1 < 10000)
(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 < 10000 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 < 10000)
(6 rows)
Case-2
-----------
postgres=# explain (costs off, verbose) select c1,simple_func(c1) from
t1 where c1 < 10000 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 < 10000)
-> Bitmap Index Scan on idx_t1
Index Cond: (t1.c1 < 10000)
(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
On Wed, Sep 13, 2017 at 5:30 PM, Rushabh Lathia <rushabh.lathia@gmail.com> wrote: > On Wed, Sep 6, 2017 at 10:04 AM, Amit Kapila <amit.kapila16@gmail.com> > 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
Thanks Amit for the patch.
I reviewed the code changes as well as performed more testing. Patchlooks 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 <amit.kapila16@gmail.com> wrote:
On Wed, Sep 13, 2017 at 5:30 PM, Rushabh Lathia
<rushabh.lathia@gmail.com> wrote:
> On Wed, Sep 6, 2017 at 10:04 AM, Amit Kapila <amit.kapila16@gmail.com>
> 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.
--
Rushabh Lathia
Вложения
On Mon, Sep 18, 2017 at 12:46 PM, Rushabh Lathia <rushabh.lathia@gmail.com> 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
On Mon, Sep 18, 2017 at 2:48 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Mon, Sep 18, 2017 at 12:46 PM, Rushabh Lathia
<rushabh.lathia@gmail.com> 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/
Rushabh Lathia
On Mon, Sep 18, 2017 at 7:02 AM, Rushabh Lathia <rushabh.lathia@gmail.com> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
On Sat, Nov 11, 2017 at 5:05 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Sep 18, 2017 at 7:02 AM, Rushabh Lathia > <rushabh.lathia@gmail.com> 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
On Sun, Nov 12, 2017 at 2:29 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: > Agreed. Your change looks good to me. OK, committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Nov 14, 2017 at 3:08 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Sun, Nov 12, 2017 at 2:29 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: >> Agreed. Your change looks good to me. > > OK, committed. > Thank you. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com