Re: [HACKERS] GatherMerge misses to push target list

[Amit Kapila]

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

[Robert Haas]

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

[Rushabh Lathia]

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

[Amit Kapila]

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

[Rushabh Lathia]

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,
 		 _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
+++ 

Re: [HACKERS] GatherMerge misses to push target list

[Amit Kapila]

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

[Rushabh Lathia]

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

[Amit Kapila]

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

[Amit Kapila]

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