pgsql: Try again to fix the way the scanjoin_target is used with partia

Robert Haas <rhaas@postgresql.org> writes:

Try again to fix the way the scanjoin_target is used with partial paths.

Was this supposed to fix the "ORDER/GROUP BY expression not found in
targetlist" problem? If so, it hasn't ...

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

On Sat, Jun 18, 2016 at 6:11 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <rhaas@postgresql.org> writes:

Try again to fix the way the scanjoin_target is used with partial paths.

Was this supposed to fix the "ORDER/GROUP BY expression not found in
targetlist" problem?

Yes.

If so, it hasn't ...

Can you please share why you think so?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Amit Kapila <amit.kapila16@gmail.com> writes:

On Sat, Jun 18, 2016 at 6:11 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Was this supposed to fix the "ORDER/GROUP BY expression not found in
targetlist" problem?
If so, it hasn't ...

Can you please share why you think so?

Run the regression tests with the settings mentioned in
/messages/by-id/22782.1466100870@sss.pgh.pa.us

You'll get a lot of cosmetic diffs (parallelized plans in EXPLAIN
and/or varying output row orders), but also half a dozen of the
aforesaid failure.

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

On Sat, Jun 18, 2016 at 7:06 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Amit Kapila <amit.kapila16@gmail.com> writes:

On Sat, Jun 18, 2016 at 6:11 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Was this supposed to fix the "ORDER/GROUP BY expression not found in
targetlist" problem?
If so, it hasn't ...

Can you please share why you think so?

Run the regression tests with the settings mentioned in
/messages/by-id/22782.1466100870@sss.pgh.pa.us

You'll get a lot of cosmetic diffs (parallelized plans in EXPLAIN
and/or varying output row orders), but also half a dozen of the
aforesaid failure.

I think I got the reason of those failures.

+               /*
+                * We can't use apply_projection_to_path() here, because
there
+                * could already be pointers to these paths, and therefore
we
+                * cannot modify them in place.  Instead, we must use
+                * create_projection_path().  The good news is this won't
+                * actually insert a Result node into the final plan unless
+                * it's needed, but the bad news is that it will charge for
+                * the node whether it's needed or not.  Therefore, if the
+                * target list is already what we need it to be, just leave
+                * this partial path alone.
+                */
+               if (equal(scanjoin_target->exprs,
subpath->pathtarget->exprs))
+                   continue;

This condition is incomplete with respect to PathTarget. What if target
exprs are same, but sortgrouprefs doesn't tally? I will try with by
modifying above condition to include sortgrouprefs as well.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Sat, Jun 18, 2016 at 7:44 AM, Amit Kapila <amit.kapila16@gmail.com>
wrote:

On Sat, Jun 18, 2016 at 7:06 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Amit Kapila <amit.kapila16@gmail.com> writes:

On Sat, Jun 18, 2016 at 6:11 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Was this supposed to fix the "ORDER/GROUP BY expression not found in
targetlist" problem?
If so, it hasn't ...

Can you please share why you think so?

Run the regression tests with the settings mentioned in
/messages/by-id/22782.1466100870@sss.pgh.pa.us

You'll get a lot of cosmetic diffs (parallelized plans in EXPLAIN
and/or varying output row orders), but also half a dozen of the
aforesaid failure.

I think I got the reason of those failures.

Patch fixing the failures along with a new test case is attached with this
mail

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Amit Kapila <amit.kapila16@gmail.com> writes:

Patch fixing the failures along with a new test case is attached with this
mail

That can't possibly be right, because the PathTarget.sortgrouprefs field
is not a pointer to a Node.

I think the two lines in question are just a poorly-thought-through case
of premature optimization. If removing them makes the failures go away
for me, that's what I plan to do.

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

On Sat, Jun 18, 2016 at 8:56 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Amit Kapila <amit.kapila16@gmail.com> writes:

Patch fixing the failures along with a new test case is attached with

this

mail

That can't possibly be right, because the PathTarget.sortgrouprefs field
is not a pointer to a Node.

Right.

I think the two lines in question are just a poorly-thought-through case
of premature optimization. If removing them makes the failures go away
for me, that's what I plan to do.

That should make the failure go-away.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Amit Kapila <amit.kapila16@gmail.com> writes:

On Sat, Jun 18, 2016 at 8:56 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I think the two lines in question are just a poorly-thought-through case
of premature optimization. If removing them makes the failures go away
for me, that's what I plan to do.

That should make the failure go-away.

Well, it did make the errors go away, but it also made the first test
case in select_parallel.sql change plan, because the parallel plan is
only a whisker cheaper than non-parallel, and the extra charge for the
nonexistent Result node changed the decision. I'm not exactly convinced
that that test case represents behavior we need to preserve, but for
the moment I rejiggered the cost adjustment so the test case stays the
same.

If we keep it like this, we definitely ought to refactor things so that
fewer places are aware of the possibility of the Result getting omitted.
Maybe push that logic into create_projection_path? If we did, we might
not need a separate apply_projection_to_path function at all? Too tired
to decide about it right now.

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

On Sat, Jun 18, 2016 at 10:06 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Amit Kapila <amit.kapila16@gmail.com> writes:

On Sat, Jun 18, 2016 at 8:56 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I think the two lines in question are just a poorly-thought-through

case

of premature optimization. If removing them makes the failures go away
for me, that's what I plan to do.

That should make the failure go-away.

Well, it did make the errors go away, but it also made the first test
case in select_parallel.sql change plan, because the parallel plan is
only a whisker cheaper than non-parallel, and the extra charge for the
nonexistent Result node changed the decision. I'm not exactly convinced
that that test case represents behavior we need to preserve,

True. I have initially proposed not to change code for adjusting the cost,
rather change test [1]/messages/by-id/CAA4eK1KnbezhNWHzUrA0cj1Wh5K1ay-wRQr8SYOdrPt9D80ZFw@mail.gmail.com. We can easily change that test.

but for
the moment I rejiggered the cost adjustment so the test case stays the
same.

If we keep it like this, we definitely ought to refactor things so that
fewer places are aware of the possibility of the Result getting omitted.
Maybe push that logic into create_projection_path?

Sounds sensible. I have noticed that for the cases when there are many
rows to be projected, the projection cost makes reasonable difference which
is quite obvious.

[1]: /messages/by-id/CAA4eK1KnbezhNWHzUrA0cj1Wh5K1ay-wRQr8SYOdrPt9D80ZFw@mail.gmail.com
/messages/by-id/CAA4eK1KnbezhNWHzUrA0cj1Wh5K1ay-wRQr8SYOdrPt9D80ZFw@mail.gmail.com

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

I wrote:

If we keep it like this, we definitely ought to refactor things so that
fewer places are aware of the possibility of the Result getting omitted.
Maybe push that logic into create_projection_path? If we did, we might
not need a separate apply_projection_to_path function at all? Too tired
to decide about it right now.

After some contemplation, it seems to me that the right thing is to make
ProjectionPath explicitly aware of the possibility that it's just a
placeholder used for the purpose of not modifying the input Path node,
and have create_projection_path calculate the correct costing either way.
In this design, the principal difference between create_projection_path
and apply_projection_to_path is that the latter assumes it can scribble
directly on the given Path while the former doesn't modify that Path.

There is one other difference: I did not do anything about making
create_projection_path push the target below a Gather. In principle,
it could do that by cloning the given GatherPath, but right now I think
that'd be unreachable code so I did not bother.

I had hoped that this would result in simplifying create_projection_plan
so that it just makes a Result or not according to what
create_projection_path decided, but there's one regression test case that
fails (in the sense of showing a Result in the plan that isn't really
needed). That happens because create_merge_append_plan adds sort columns
to the tlist and so a tlist match is possible after that happens when it
didn't match before. For the moment I kluged create_projection_plan so
that that keeps working, but I wonder if it'd be better to just accept an
extra Result in that case.

Also, I got rid of the target_parallel argument to
apply_projection_to_path, as I thought that that was just way too much
interconnection between apply_projection_to_path and its callers than
is justified for what it saves (namely one call of has_parallel_hazard
when planning a Gather). In general, having that argument could *add*
extra calls of has_parallel_hazard, since callers might have to do
that computation whether or not a Gather is present.

Any objections?

regards, tom lane

On Tue, Jun 21, 2016 at 1:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wrote:

If we keep it like this, we definitely ought to refactor things so that
fewer places are aware of the possibility of the Result getting omitted.
Maybe push that logic into create_projection_path? If we did, we might
not need a separate apply_projection_to_path function at all? Too tired
to decide about it right now.

After some contemplation, it seems to me that the right thing is to make
ProjectionPath explicitly aware of the possibility that it's just a
placeholder used for the purpose of not modifying the input Path node,
and have create_projection_path calculate the correct costing either way.
In this design, the principal difference between create_projection_path
and apply_projection_to_path is that the latter assumes it can scribble
directly on the given Path while the former doesn't modify that Path.

+1.

There is one other difference: I did not do anything about making
create_projection_path push the target below a Gather. In principle,
it could do that by cloning the given GatherPath, but right now I think
that'd be unreachable code so I did not bother.

Works for me.

I had hoped that this would result in simplifying create_projection_plan
so that it just makes a Result or not according to what
create_projection_path decided, but there's one regression test case that
fails (in the sense of showing a Result in the plan that isn't really
needed). That happens because create_merge_append_plan adds sort columns
to the tlist and so a tlist match is possible after that happens when it
didn't match before. For the moment I kluged create_projection_plan so
that that keeps working, but I wonder if it'd be better to just accept an
extra Result in that case.

Not sure if it's better to accept an extra Result in that case, but I
agree that's ugly.

Also, I got rid of the target_parallel argument to
apply_projection_to_path, as I thought that that was just way too much
interconnection between apply_projection_to_path and its callers than
is justified for what it saves (namely one call of has_parallel_hazard
when planning a Gather). In general, having that argument could *add*
extra calls of has_parallel_hazard, since callers might have to do
that computation whether or not a Gather is present.

I had a feeling you weren't going to like that, but it also didn't
seem great to redo that computation for every path. Right now, we
only need it for Gather paths, but if we start marking subquery RTEs
as parallel-safe and fix upper rels to correctly set
consider_parallel, I have a feeling this is going to be needed more.
But feel free to ignore that for now since I don't have a completely
well-thought-out theory on this.

--
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

I wrote:

I had hoped that this would result in simplifying create_projection_plan
so that it just makes a Result or not according to what
create_projection_path decided, but there's one regression test case that
fails (in the sense of showing a Result in the plan that isn't really
needed). That happens because create_merge_append_plan adds sort columns
to the tlist and so a tlist match is possible after that happens when it
didn't match before. For the moment I kluged create_projection_plan so
that that keeps working, but I wonder if it'd be better to just accept an
extra Result in that case.

After further thought about that, I realized the issue is bigger than just
MergeAppend nodes: in general, if createplan.c alters the tlist of a node
by adding resjunk columns, that could break the earlier decision about
whether the tlist expressions are equal, *in either direction*. So we
have to consider that create_projection_path's decision is just tentative
anytime we are dealing with a non-projection-capable subpath. I think
this means that apply_projection_to_path is actually broken right now,
because it supposes that any tlist equality that it sees can't change
later. (It might be that we don't use it in a way that would cause that
to manifest, but I don't have a lot of faith in that, since this code is
all pretty new.) Revised patch attached.

regards, tom lane

Robert Haas <robertmhaas@gmail.com> writes:

On Tue, Jun 21, 2016 at 1:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Also, I got rid of the target_parallel argument to
apply_projection_to_path, as I thought that that was just way too much
interconnection between apply_projection_to_path and its callers than
is justified for what it saves (namely one call of has_parallel_hazard
when planning a Gather). In general, having that argument could *add*
extra calls of has_parallel_hazard, since callers might have to do
that computation whether or not a Gather is present.

I had a feeling you weren't going to like that, but it also didn't
seem great to redo that computation for every path. Right now, we
only need it for Gather paths, but if we start marking subquery RTEs
as parallel-safe and fix upper rels to correctly set
consider_parallel, I have a feeling this is going to be needed more.
But feel free to ignore that for now since I don't have a completely
well-thought-out theory on this.

If that does start being a problem, I'd be inclined to address it by
teaching PathTarget to track whether its contents are parallel-safe.
For now, though, I think we don't need the complication.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers