print_path is missing GatherMerge and CustomScan support

On Wed, Jul 18, 2018 at 02:35:23PM +0900, Masahiko Sawada wrote:

Hi,

While debugging planner I realized that print_path() function is not
aware of both GatherMerge path and CustomScan path. Attached small
patch fixes it.

Good catch. Those should be backpatched. While I am looking at this
stuff, I have noticed that pathnode.c/reparameterize_path_by_child uses
T_MergeAppend and not T_MergeAppendPath.

--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -3817,7 +3817,7 @@ do { \
            }
            break;

-       case T_MergeAppend:
+       case T_MergeAppendPath:
            {
                 MergeAppendPath *mapath

This is new as of f49842d1 in v11. Robert, Ashutosh, am I missing
something?
--
Michael

On Wed, Jul 18, 2018 at 11:52 AM, Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Jul 18, 2018 at 02:35:23PM +0900, Masahiko Sawada wrote:

Hi,

While debugging planner I realized that print_path() function is not
aware of both GatherMerge path and CustomScan path. Attached small
patch fixes it.

Good catch. Those should be backpatched. While I am looking at this
stuff, I have noticed that pathnode.c/reparameterize_path_by_child uses
T_MergeAppend and not T_MergeAppendPath.

--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -3817,7 +3817,7 @@ do { \
}
break;

-       case T_MergeAppend:
+       case T_MergeAppendPath:
{
MergeAppendPath *mapath

This is new as of f49842d1 in v11.

Yes that's right. Thanks for taking care of it.

Robert, Ashutosh, am I missing
something?

You used my personal email id by mistake, I think. I have removed it
and added by EDB email address.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On Wed, Jul 18, 2018 at 12:15:25PM +0530, Ashutosh Bapat wrote:

On Wed, Jul 18, 2018 at 11:52 AM, Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Jul 18, 2018 at 02:35:23PM +0900, Masahiko Sawada wrote:

Hi,

While debugging planner I realized that print_path() function is not
aware of both GatherMerge path and CustomScan path. Attached small
patch fixes it.

Good catch. Those should be backpatched. While I am looking at this
stuff, I have noticed that pathnode.c/reparameterize_path_by_child uses
T_MergeAppend and not T_MergeAppendPath.

This is new as of f49842d1 in v11.

Yes that's right. Thanks for taking care of it.

Thanks for the confirmation. Robert, do you want to take care of this
issue or should I?

Robert, Ashutosh, am I missing
something?

You used my personal email id by mistake, I think. I have removed it
and added by EDB email address.

My sincere apologies. I have not been careful.
--
Michael

On Wed, Jul 18, 2018 at 12:15:25PM +0530, Ashutosh Bapat wrote:

Yes that's right. Thanks for taking care of it.

Okay, I have pushed a fix for this one as that's wrong and
back-patched to v11. The coverage of reparameterize_path_by_child is
actually quite poor if you look at the reports:
https://coverage.postgresql.org/src/backend/optimizer/util/pathnode.c.gcov.html

Could it be possible to stress that more? This way mistakes like this
one could have been avoided from the start.
--
Michael

On Wed, Jul 18, 2018 at 03:22:02PM +0900, Michael Paquier wrote:

Good catch. Those should be backpatched. While I am looking at this
stuff, I have noticed that pathnode.c/reparameterize_path_by_child uses
T_MergeAppend and not T_MergeAppendPath.

Okay, I have checked the full list of path nodes and the two ones you
mentioned are the only missing. CustomPath has been added in 9.5, so
this has been patched down to this version. GatherMergePath is new as
of 10.

The order of the items in print_path and nodes.h was a bit messed up as
well which made unnecessarily harder to check the list, so I fixed the
order at the same time to ease future lookups and back-patching effort.
--
Michael

On Thu, Jul 19, 2018 at 5:37 AM, Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Jul 18, 2018 at 12:15:25PM +0530, Ashutosh Bapat wrote:

Yes that's right. Thanks for taking care of it.

Okay, I have pushed a fix for this one as that's wrong and
back-patched to v11. The coverage of reparameterize_path_by_child is
actually quite poor if you look at the reports:
https://coverage.postgresql.org/src/backend/optimizer/util/pathnode.c.gcov.html

Could it be possible to stress that more? This way mistakes like this
one could have been avoided from the start.

I had extensive testcases in my original patch-set to exercise that
code but 1. that testset was too extensive; even today
partition_join.sql is a separate testcase and it's quite large. 2.
that function returns NULL rather than throwing an error, if it can
not produce a parameterized path. So, unless we check whether each of
those paths get created no test is useful and that can only be done
through an EXPLAIN OUTPUT which means that the testcase becomes
fragile. I fine if we want to add more tests just to cover the code if
those are not as fragile and do not blow up partition_join too much.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On Thu, Jul 19, 2018 at 10:41:10AM +0530, Ashutosh Bapat wrote:

I had extensive testcases in my original patch-set to exercise that
code but 1. that testset was too extensive; even today
partition_join.sql is a separate testcase and it's quite large. 2.
that function returns NULL rather than throwing an error, if it can
not produce a parameterized path. So, unless we check whether each of
those paths get created no test is useful and that can only be done
through an EXPLAIN OUTPUT which means that the testcase becomes
fragile. I fine if we want to add more tests just to cover the code if
those are not as fragile and do not blow up partition_join too much.

Did those really check the reparameterization of a MergeAppendPath for a
child of the parent? I'd be surprised if this one was stressed as there
was no match in the list.
--
Michael

On Thu, Jul 19, 2018 at 11:56 AM, Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Jul 19, 2018 at 10:41:10AM +0530, Ashutosh Bapat wrote:

I had extensive testcases in my original patch-set to exercise that
code but 1. that testset was too extensive; even today
partition_join.sql is a separate testcase and it's quite large. 2.
that function returns NULL rather than throwing an error, if it can
not produce a parameterized path. So, unless we check whether each of
those paths get created no test is useful and that can only be done
through an EXPLAIN OUTPUT which means that the testcase becomes
fragile. I fine if we want to add more tests just to cover the code if
those are not as fragile and do not blow up partition_join too much.

Did those really check the reparameterization of a MergeAppendPath for a
child of the parent? I'd be surprised if this one was stressed as there
was no match in the list.

I don't remember but given this evident, it was not. But there are
some testcases in partition_join which test this code using LATERAL
joins. But those do not cover the entire function.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On Thu, Jul 19, 2018 at 9:58 AM, Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Jul 18, 2018 at 03:22:02PM +0900, Michael Paquier wrote:

Good catch. Those should be backpatched. While I am looking at this
stuff, I have noticed that pathnode.c/reparameterize_path_by_child uses
T_MergeAppend and not T_MergeAppendPath.

Okay, I have checked the full list of path nodes and the two ones you
mentioned are the only missing. CustomPath has been added in 9.5, so
this has been patched down to this version. GatherMergePath is new as
of 10.

The order of the items in print_path and nodes.h was a bit messed up as
well which made unnecessarily harder to check the list, so I fixed the
order at the same time to ease future lookups and back-patching effort.

Thank you for committing the patch!

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

(2018/07/19 14:11), Ashutosh Bapat wrote:

On Thu, Jul 19, 2018 at 5:37 AM, Michael Paquier<michael@paquier.xyz> wrote:

On Wed, Jul 18, 2018 at 12:15:25PM +0530, Ashutosh Bapat wrote:

Yes that's right. Thanks for taking care of it.

Okay, I have pushed a fix for this one as that's wrong and
back-patched to v11. The coverage of reparameterize_path_by_child is
actually quite poor if you look at the reports:
https://coverage.postgresql.org/src/backend/optimizer/util/pathnode.c.gcov.html

Could it be possible to stress that more? This way mistakes like this
one could have been avoided from the start.

I had extensive testcases in my original patch-set to exercise that
code but 1. that testset was too extensive; even today
partition_join.sql is a separate testcase and it's quite large. 2.
that function returns NULL rather than throwing an error, if it can
not produce a parameterized path. So, unless we check whether each of
those paths get created no test is useful and that can only be done
through an EXPLAIN OUTPUT which means that the testcase becomes
fragile. I fine if we want to add more tests just to cover the code if
those are not as fragile and do not blow up partition_join too much.

It would not be possible to cover these cases:

case T_GatherPath:
{
GatherPath *gpath;

FLAT_COPY_PATH(gpath, path, GatherPath);
REPARAMETERIZE_CHILD_PATH(gpath->subpath);
new_path = (Path *) gpath;
}
break;

case T_GatherMergePath:
{
GatherMergePath *gmpath;

FLAT_COPY_PATH(gmpath, path, GatherMergePath);
REPARAMETERIZE_CHILD_PATH(gmpath->subpath);
new_path = (Path *) gmpath;
}
break;

because we currently don't consider gathering partial child-scan or
child-join paths. I think we might consider that in future, though.

Best regards,
Etsuro Fujita

On Thu, Jul 26, 2018 at 1:14 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:

because we currently don't consider gathering partial child-scan or
child-join paths. I think we might consider that in future, though.

You generally want to put the Gather node as high up in the plan tree
as possible. I think the only case in which this is beneficial is if
you can't put the Gather or Gather Merge node above the Append because
only some of the children are parallel-safe. In that case, a separate
Gather per child can be better than no parallelism at all. It's a
rare case, but it can happen. Actually, I thought we had code for this
already: see the end of apply_scanjoin_target_to_paths().

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

(2018/07/27 4:50), Robert Haas wrote:

On Thu, Jul 26, 2018 at 1:14 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:

because we currently don't consider gathering partial child-scan or
child-join paths. I think we might consider that in future, though.

You generally want to put the Gather node as high up in the plan tree
as possible. I think the only case in which this is beneficial is if
you can't put the Gather or Gather Merge node above the Append because
only some of the children are parallel-safe. In that case, a separate
Gather per child can be better than no parallelism at all. It's a
rare case, but it can happen. Actually, I thought we had code for this
already: see the end of apply_scanjoin_target_to_paths().

Agreed. Thanks for the explanation!

(I think that at least currently, there is no need for the Gather and
GatherMerge cases in reparameterize_path_by_child, but I don't object to
keeping those as-is there.)

Best regards,
Etsuro Fujita

On Fri, Jul 27, 2018 at 03:40:48PM +0900, Etsuro Fujita wrote:

(I think that at least currently, there is no need for the Gather and
GatherMerge cases in reparameterize_path_by_child, but I don't object to
keeping those as-is there.)

Let's keep them. As far as my understanding goes, which is way lower
than any of you by the way, those don't hurt and would automatically
help.
--
Michael