Horton

Thread

Commits

  1. Push target list evaluation through Gather Merge.

  1. GatherMerge misses to push target list

    Amit Kapila <amit.kapila16@gmail.com> — 2017-09-06T04:34:16Z

    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

  2. Re: GatherMerge misses to push target list

    Amit Kapila <amit.kapila16@gmail.com> — 2017-09-12T02:31:47Z

    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

  3. Re: GatherMerge misses to push target list

    Rushabh Lathia <rushabh.lathia@gmail.com> — 2017-09-13T12:00:02Z

    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

  4. Re: GatherMerge misses to push target list

    Amit Kapila <amit.kapila16@gmail.com> — 2017-09-14T04:32:14Z

    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

  5. Re: GatherMerge misses to push target list

    Rushabh Lathia <rushabh.lathia@gmail.com> — 2017-09-18T07:16:24Z

    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 <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.
>
> --
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>



-- 
Rushabh Lathia

  6. Re: GatherMerge misses to push target list

    Amit Kapila <amit.kapila16@gmail.com> — 2017-09-18T09:18:53Z

    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

  7. Re: GatherMerge misses to push target list

    Rushabh Lathia <rushabh.lathia@gmail.com> — 2017-09-18T11:02:18Z

    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/
>
> --
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>



-- 
Rushabh Lathia

  8. Re: GatherMerge misses to push target list

    Robert Haas <robertmhaas@gmail.com> — 2017-11-11T11:35:38Z

    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

  9. Re: GatherMerge misses to push target list

    Amit Kapila <amit.kapila16@gmail.com> — 2017-11-12T07:29:50Z

    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

  10. Re: [HACKERS] GatherMerge misses to push target list

    Robert Haas <robertmhaas@gmail.com> — 2017-11-13T21:38:16Z

    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

  11. Re: [HACKERS] GatherMerge misses to push target list

    Amit Kapila <amit.kapila16@gmail.com> — 2017-11-14T01:51:35Z

    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