Thread: fixing subplan/subquery confusion

On Sun, Jun 26, 2016 at 10:39 PM, Noah Misch <noah@leadboat.com> wrote:
> On Mon, Jun 13, 2016 at 04:46:19PM -0400, Tom Lane wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>> > In practice, we don't yet have the ability for
>> > parallel-safe paths from subqueries to affect planning at higher query
>> > levels, but that's because the pathification stuff landed too late in
>> > the cycle for me to fully react to it.
>>
>> I wonder if that's not just from confusion between subplans and
>> subqueries.  I don't believe any of the claims made in the comment
>> adjusted in the patch below (other than "Subplans currently aren't passed
>> to workers", which is true but irrelevant).  Nested Gather nodes is
>> something that you would need, and already have, a defense for anyway.
>
> [Action required within 72 hours.  This is a generic notification.]
>
> The above-described topic is currently a PostgreSQL 9.6 open item ("fix
> possible confusion between subqueries and subplans").  Robert, since you
> committed the patch believed to have created it, you own this open item.  If
> some other commit is more relevant or if this does not belong as a 9.6 open
> item, please let us know.  Otherwise, please observe the policy on open item
> ownership[1] and send a status update within 72 hours of this message.
> Include a date for your subsequent status update.  Testers may discover new
> open items at any time, and I want to plan to get them all fixed well in
> advance of shipping 9.6rc1.  Consequently, I will appreciate your efforts
> toward speedy resolution.  Thanks.

This open item comes with a patch submitted by Tom Lane.  If Tom wants
me to review and (if no problems are found) commit that patch to
resolve this open item, I'm willing to do that.  But generally I don't
commit patches submitted by other committers unless that person and I
have agreed on it in advance, which is not currently the case here.

Tom, do you want to commit this, or do you want me to handle it, or
something else?

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Jun 26, 2016 at 10:39 PM, Noah Misch <noah@leadboat.com> wrote:
>> On Mon, Jun 13, 2016 at 04:46:19PM -0400, Tom Lane wrote:
>> The above-described topic is currently a PostgreSQL 9.6 open item ("fix
>> possible confusion between subqueries and subplans").

> This open item comes with a patch submitted by Tom Lane.  If Tom wants
> me to review and (if no problems are found) commit that patch to
> resolve this open item, I'm willing to do that.  But generally I don't
> commit patches submitted by other committers unless that person and I
> have agreed on it in advance, which is not currently the case here.

> Tom, do you want to commit this, or do you want me to handle it, or
> something else?

I was not sure if you'd agreed that the patch was correct, and in any
case I thought you wanted to fold it into the upperrel consider_parallel
patch.  I feel no great need to commit it myself, if that's what you
meant.
        regards, tom lane

On Mon, Jun 27, 2016 at 4:03 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Sun, Jun 26, 2016 at 10:39 PM, Noah Misch <noah@leadboat.com> wrote:
>>> On Mon, Jun 13, 2016 at 04:46:19PM -0400, Tom Lane wrote:
>>> The above-described topic is currently a PostgreSQL 9.6 open item ("fix
>>> possible confusion between subqueries and subplans").
>
>> This open item comes with a patch submitted by Tom Lane.  If Tom wants
>> me to review and (if no problems are found) commit that patch to
>> resolve this open item, I'm willing to do that.  But generally I don't
>> commit patches submitted by other committers unless that person and I
>> have agreed on it in advance, which is not currently the case here.
>
>> Tom, do you want to commit this, or do you want me to handle it, or
>> something else?
>
> I was not sure if you'd agreed that the patch was correct, and in any
> case I thought you wanted to fold it into the upperrel consider_parallel
> patch.  I feel no great need to commit it myself, if that's what you
> meant.

OK, I'll set aside some time for further review and either commit it
or send an update by COB Thursday US time.

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

On Tue, Jun 28, 2016 at 1:42 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Jun 27, 2016 at 4:03 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> On Sun, Jun 26, 2016 at 10:39 PM, Noah Misch <noah@leadboat.com> wrote:
>>>> On Mon, Jun 13, 2016 at 04:46:19PM -0400, Tom Lane wrote:
>>>> The above-described topic is currently a PostgreSQL 9.6 open item ("fix
>>>> possible confusion between subqueries and subplans").
>>
>>> This open item comes with a patch submitted by Tom Lane.  If Tom wants
>>> me to review and (if no problems are found) commit that patch to
>>> resolve this open item, I'm willing to do that.  But generally I don't
>>> commit patches submitted by other committers unless that person and I
>>> have agreed on it in advance, which is not currently the case here.
>>
>>> Tom, do you want to commit this, or do you want me to handle it, or
>>> something else?
>>
>> I was not sure if you'd agreed that the patch was correct, and in any
>> case I thought you wanted to fold it into the upperrel consider_parallel
>> patch.  I feel no great need to commit it myself, if that's what you
>> meant.
>
> OK, I'll set aside some time for further review and either commit it
> or send an update by COB Thursday US time.
>

I had couple of questions [1] related to that patch.  See if you find
those as relevant?


[1] - https://www.postgresql.org/message-id/CAA4eK1%2ByGs-onuJDy%2BTTqnrnT0hty_QQPC1GipS%2Bce-W3872QQ%40mail.gmail.com

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

Amit Kapila <amit.kapila16@gmail.com> writes:
> I had couple of questions [1] related to that patch.  See if you find
> those as relevant?

I do not think those cases are directly relevant: you're talking about
appendrels not single, unflattened RTE_SUBQUERY rels.

In the subquery case, my view of how it ought to work is that Paths coming
up from the subquery would be marked as not-parallel-safe if they contain
references to unsafe functions.  It might be that that doesn't happen for
free, but my guess is that it would already work that way given a change
similar to what I proposed.

In the appendrel case, I tend to agree that the easiest solution is to
scan all the children of the appendrel and just mark the whole thing as
not consider_parallel if any of them have unsafe functions.
        regards, tom lane

On Tue, Jun 28, 2016 at 8:25 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Kapila <amit.kapila16@gmail.com> writes:
>> I had couple of questions [1] related to that patch.  See if you find
>> those as relevant?
>
> I do not think those cases are directly relevant: you're talking about
> appendrels not single, unflattened RTE_SUBQUERY rels.
>

Right, but still I think we shouldn't leave the appendrel case unattended.

> In the subquery case, my view of how it ought to work is that Paths coming
> up from the subquery would be marked as not-parallel-safe if they contain
> references to unsafe functions.  It might be that that doesn't happen for
> free, but my guess is that it would already work that way given a change
> similar to what I proposed.
>

This makes sense to me.

> In the appendrel case, I tend to agree that the easiest solution is to
> scan all the children of the appendrel and just mark the whole thing as
> not consider_parallel if any of them have unsafe functions.
>

Thats what I had in mind as well, but not sure which is the best place
to set it.  Shall we do it in set_append_rel_size() after setting the
size of each relation (after foreach loop) or is it better to do it in
set_append_rel_pathlist().  Is it better to do it as a separate patch
or to enhance your patch for this change?  If you are okay, I can
update the patch or write a new one based on what is preferred?


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

On Tue, Jun 28, 2016 at 5:22 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Tue, Jun 28, 2016 at 8:25 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>> In the appendrel case, I tend to agree that the easiest solution is to
>> scan all the children of the appendrel and just mark the whole thing as
>> not consider_parallel if any of them have unsafe functions.
>>
>
> Thats what I had in mind as well, but not sure which is the best place
> to set it.  Shall we do it in set_append_rel_size() after setting the
> size of each relation (after foreach loop) or is it better to do it in
> set_append_rel_pathlist().  Is it better to do it as a separate patch
> or to enhance your patch for this change?
>

I have done it as a separate patch.  I think doing it in
set_append_rel_size() has an advantage that we don't need to scan the
child rels separately.  If you think that attached patch is on right
lines, then I can add test cases as well.


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

On Mon, Jun 27, 2016 at 4:12 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> Tom, do you want to commit this, or do you want me to handle it, or
>>> something else?
>>
>> I was not sure if you'd agreed that the patch was correct, and in any
>> case I thought you wanted to fold it into the upperrel consider_parallel
>> patch.  I feel no great need to commit it myself, if that's what you
>> meant.
>
> OK, I'll set aside some time for further review and either commit it
> or send an update by COB Thursday US time.

I haven't had a chance to do this yet, so I'm going to do it tomorrow instead.

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

On Thu, Jun 30, 2016 at 6:49 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> I haven't had a chance to do this yet, so I'm going to do it tomorrow instead.

I dug into this a bit and found more problems.  I wondered why Tom's
patch did this:

!                       if (has_parallel_hazard((Node *) rte->subquery, false))
!                               return;
!                       break;

Instead of just doing this:

-            return;
+            break;

After all, the code that built subquery paths ought to be sufficient
to find any parallel hazards during subquery planning; there seems to
be no especially-good reason why we should walk the whole query tree
searching for the again.  So I changed that and ran the regression
tests.  With force_parallel_mode=on, things got unhappy on exactly one
query:

  SELECT * FROM
  (SELECT a || b AS ab FROM t1
   UNION ALL
   SELECT ab FROM t2) t
  ORDER BY 1 LIMIT 8;

This failed with a complaint about parallel workers trying to touch
temporary relations, which turns out to be pretty valid since t1 and
t2 are BOTH temporary relations.  This turns out to be another facet
of my ignorance about how subqueries can be pulled up to create
appendrels with crazy things in them.  set_rel_consider_parallel()
looks at the appendrel and thinks everything is fine, because the
reference to temporary tables are buried inside the appendrel members,
which are note examined.

I think it's hard to avoid the conclusion that
set_rel_consider_parallel() needs to run on other member rels as well
as baserels.  We might be able to do that only for cases where the
parent is a subquery RTE, since if the parent is a baserel then I
think we must have just a standard inheritance hierarchy and things
might be OK, but even then, I fear there might be problems with RLS.
Anyway, the attached patch solves the problem in a fairly "brute
force" fashion.  We loop over all basrels and other member rels and
set consider_parallel according to their properties.  Then, when
setting base rel sizes, we clear consider_parallel for any parents if
it's clear for any of the children.  Finally, before setting base rel
pathlists for appendrel children, we clear the flag for the child if
it's meanwhile been cleared for the parent.  Thus, the parents and
children always agree and only consider parallel paths for any of the
rels if they're all OK.  This seems a bit grotty to me so suggestions
are welcome.

(Official status update: I'm not prepared to commit this without some
review.  So I intend to wait for a while and see whether I get some
review.  I realize these status updates are supposed to contain a date
by which further action will be taken, but I don't know how to
meaningfully do that in this type of case.)

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

On Sat, Jul 2, 2016 at 12:29 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Jun 30, 2016 at 6:49 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> I haven't had a chance to do this yet, so I'm going to do it tomorrow instead.
>
> I dug into this a bit and found more problems.  I wondered why Tom's
> patch did this:
>
> !                       if (has_parallel_hazard((Node *) rte->subquery, false))
> !                               return;
> !                       break;
>
> Instead of just doing this:
>
> -            return;
> +            break;
>
> After all, the code that built subquery paths ought to be sufficient
> to find any parallel hazards during subquery planning; there seems to
> be no especially-good reason why we should walk the whole query tree
> searching for the again.  So I changed that and ran the regression
> tests.  With force_parallel_mode=on, things got unhappy on exactly one
> query:
>
>   SELECT * FROM
>   (SELECT a || b AS ab FROM t1
>    UNION ALL
>    SELECT ab FROM t2) t
>   ORDER BY 1 LIMIT 8;
>
> This failed with a complaint about parallel workers trying to touch
> temporary relations, which turns out to be pretty valid since t1 and
> t2 are BOTH temporary relations.  This turns out to be another facet
> of my ignorance about how subqueries can be pulled up to create
> appendrels with crazy things in them.  set_rel_consider_parallel()
> looks at the appendrel and thinks everything is fine, because the
> reference to temporary tables are buried inside the appendrel members,
> which are note examined.
>
> I think it's hard to avoid the conclusion that
> set_rel_consider_parallel() needs to run on other member rels as well
> as baserels.  We might be able to do that only for cases where the
> parent is a subquery RTE, since if the parent is a baserel then I
> think we must have just a standard inheritance hierarchy and things
> might be OK, but even then, I fear there might be problems with RLS.
> Anyway, the attached patch solves the problem in a fairly "brute
> force" fashion.  We loop over all basrels and other member rels and
> set consider_parallel according to their properties.  Then, when
> setting base rel sizes, we clear consider_parallel for any parents if
> it's clear for any of the children.  Finally, before setting base rel
> pathlists for appendrel children, we clear the flag for the child if
> it's meanwhile been cleared for the parent.  Thus, the parents and
> children always agree and only consider parallel paths for any of the
> rels if they're all OK.  This seems a bit grotty to me so suggestions
> are welcome.
>

@@ -966,8 +985,9 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel, continue; }
- /* Copy consider_parallel flag from parent. */
- childrel->consider_parallel = rel->consider_parallel;
+ /* If any childrel is not parallel-safe, neither is parent. */
+ if (!childrel->consider_parallel)
+ rel->consider_parallel = false;


Doing this way misses the point that adjust_appendrel_attrs() can
change the targetlist. We should consider it for parallel-safety after
it gets modified.  I think that point can be addressed if try to set
consider_parallel for child rels as I have done in patch [1].

 /*
+ * If the parent isn't eligible for parallelism, there's no point
+ * in considering it for the children.  (This might change someday
+ * if we have a way to build an Append plan where some of the child
+ * plans are forced to run in the parent and others can run in any
+ * process, but for now there's no point in expending cycles building
+ * childrel paths we can't use.)
+ */

+ if (!rel->consider_parallel)
+ childrel->consider_parallel = false;
+

What exactly makes Append plan to not able to run some of the child
nodes is other process?


[1] - https://www.postgresql.org/message-id/CAA4eK1Jg_GvaTEjJSaV5vZY6acDmi-B3iXWvdiXa%2BpUFbnkyTg%40mail.gmail.com

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

Robert Haas <robertmhaas@gmail.com> writes:
> I dug into this a bit and found more problems.  I wondered why Tom's
> patch did this:

> !                       if (has_parallel_hazard((Node *) rte->subquery, false))
> !                               return;
> !                       break;

> Instead of just doing this:

> -            return;
> +            break;

> After all, the code that built subquery paths ought to be sufficient
> to find any parallel hazards during subquery planning; there seems to
> be no especially-good reason why we should walk the whole query tree
> searching for the again.

Yeah, I think you are right.  I modeled my patch on the nearby handling of
function RTEs, wherein function_rte_parallel_ok checks the RTE contents
for safety.  But we must do that because create_functionscan_path relies
entirely on the rel's consider_parallel flag.  In contrast,
create_subqueryscan_path looks at both rel->consider_parallel and
subpath->parallel_safe, so we can rely on the subpath(s)' markings as an
indication of the subquery's safety.  (rel->consider_parallel then tells
us just about the restriction and projection expressions on the subquery
rel itself, and we'll end up with the right choice if those are unsafe.)

BTW, looking elsewhere in set_rel_consider_parallel, isn't there an extra
"return;" in the tablesample stanza, allpaths.c:541 as of HEAD?  Looks to
me like we're failing to ever treat tablesampling as parallel-safe.
I'm rather dubious about whether any of our tablesample methods actually
are parallel-safe, but that should be down to the method to say.  If this
code's been like this all along, we've never tested them.

> I think it's hard to avoid the conclusion that
> set_rel_consider_parallel() needs to run on other member rels as well
> as baserels.  We might be able to do that only for cases where the
> parent is a subquery RTE, since if the parent is a baserel then I
> think we must have just a standard inheritance hierarchy and things
> might be OK, but even then, I fear there might be problems with RLS.
> Anyway, the attached patch solves the problem in a fairly "brute
> force" fashion.

I agree, and I also agree that this way is pretty brute-force.
I think a cleaner way is to have set_append_rel_size() invoke
set_rel_consider_parallel() on the child rels and then propagate their
parallel-unsafety up to the parent.  That seems fairly analogous to
the way we already deal with their sizes: in particular, set_rel_size
is invoked on an appendrel child from there, not from an extra pass in
set_base_rel_sizes.  Then set_append_rel_pathlist can propagate unsafety
down again, as you've done here.

> (Official status update: I'm not prepared to commit this without some
> review.  So I intend to wait for a while and see whether I get some
> review.  I realize these status updates are supposed to contain a date
> by which further action will be taken, but I don't know how to
> meaningfully do that in this type of case.)

You mentioned that you'll be on vacation for much of July.  If you like,
I will take this open item off your hands, since I'll be around and can
deal with any bugs that pop up in it.
        regards, tom lane

On Sun, Jul 3, 2016 at 1:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> BTW, looking elsewhere in set_rel_consider_parallel, isn't there an extra
> "return;" in the tablesample stanza, allpaths.c:541 as of HEAD?  Looks to
> me like we're failing to ever treat tablesampling as parallel-safe.
> I'm rather dubious about whether any of our tablesample methods actually
> are parallel-safe, but that should be down to the method to say.  If this
> code's been like this all along, we've never tested them.

You are correct.

>> I think it's hard to avoid the conclusion that
>> set_rel_consider_parallel() needs to run on other member rels as well
>> as baserels.  We might be able to do that only for cases where the
>> parent is a subquery RTE, since if the parent is a baserel then I
>> think we must have just a standard inheritance hierarchy and things
>> might be OK, but even then, I fear there might be problems with RLS.
>> Anyway, the attached patch solves the problem in a fairly "brute
>> force" fashion.
>
> I agree, and I also agree that this way is pretty brute-force.
> I think a cleaner way is to have set_append_rel_size() invoke
> set_rel_consider_parallel() on the child rels and then propagate their
> parallel-unsafety up to the parent.  That seems fairly analogous to
> the way we already deal with their sizes: in particular, set_rel_size
> is invoked on an appendrel child from there, not from an extra pass in
> set_base_rel_sizes.  Then set_append_rel_pathlist can propagate unsafety
> down again, as you've done here.

I was reluctant to do it that way because of the relatively stupid way
that we have to go about finding the inheritance children to visit,
but maybe I shouldn't let that dominate my thinking.  We could always
replace that with some more efficient data structure at some point
down the road.

>> (Official status update: I'm not prepared to commit this without some
>> review.  So I intend to wait for a while and see whether I get some
>> review.  I realize these status updates are supposed to contain a date
>> by which further action will be taken, but I don't know how to
>> meaningfully do that in this type of case.)
>
> You mentioned that you'll be on vacation for much of July.  If you like,
> I will take this open item off your hands, since I'll be around and can
> deal with any bugs that pop up in it.

That would be much appreciated.

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Jul 3, 2016 at 1:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think a cleaner way is to have set_append_rel_size() invoke
>> set_rel_consider_parallel() on the child rels and then propagate their
>> parallel-unsafety up to the parent.  That seems fairly analogous to
>> the way we already deal with their sizes: in particular, set_rel_size
>> is invoked on an appendrel child from there, not from an extra pass in
>> set_base_rel_sizes.  Then set_append_rel_pathlist can propagate unsafety
>> down again, as you've done here.

> I was reluctant to do it that way because of the relatively stupid way
> that we have to go about finding the inheritance children to visit,
> but maybe I shouldn't let that dominate my thinking.  We could always
> replace that with some more efficient data structure at some point
> down the road.

That might be a reasonable objection if we needed to add a third such
loop, but I'm talking about putting the set_rel_consider_parallel call in
the loop that already exists.  So there shouldn't be any added overhead.

(It's definitely true that we could improve on the append_rel_list-based
search mechanism; but so far I have never seen any indication that that
was a bottleneck, so it's pretty far down my to-do list.)

>> You mentioned that you'll be on vacation for much of July.  If you like,
>> I will take this open item off your hands, since I'll be around and can
>> deal with any bugs that pop up in it.

> That would be much appreciated.

OK, will do.
        regards, tom lane

I wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Sun, Jul 3, 2016 at 1:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> You mentioned that you'll be on vacation for much of July.  If you like,
>>> I will take this open item off your hands, since I'll be around and can
>>> deal with any bugs that pop up in it.

>> That would be much appreciated.

> OK, will do.

I've reviewed, adjusted, and pushed this patch, and accordingly am
marking the open item closed.  (Any remaining bugs should become
new open items.)
        regards, tom lane

On Sun, Jul 3, 2016 at 6:46 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> On Sun, Jul 3, 2016 at 1:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> You mentioned that you'll be on vacation for much of July.  If you like,
>>>> I will take this open item off your hands, since I'll be around and can
>>>> deal with any bugs that pop up in it.
>
>>> That would be much appreciated.
>
>> OK, will do.
>
> I've reviewed, adjusted, and pushed this patch, and accordingly am
> marking the open item closed.  (Any remaining bugs should become
> new open items.)

Thanks.  Looks good to me.

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