Re: [sqlsmith] Failed assertion in parallel worker (ExecInitSubPlan)

On Thu, Jun 9, 2016 at 7:44 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Thu, Jun 9, 2016 at 9:37 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> > On Thu, Jun 9, 2016 at 8:47 AM, Robert Haas <robertmhaas(at)gmail(dot)com>
wrote:
> >> On Mon, Jun 6, 2016 at 6:07 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> >> wrote:
> >> > That seems doable, as for such rels we can only have Vars and
> >> > PlaceHolderVars in targetlist. Basically, whenever we are adding
> >> > PlaceHolderVars to a relation, just remember that information and
use it
> >> > later. The patch doing the same is attached with this mail. Now
still,
> >> > this won't cover the case of ChildRels for an Append Relation as for
> >> > that we
> >> > adjust target list separately in set_append_rel_size. I think we
need
> >> > to
> >> > deal with it separately.
> >>
> >> This approach looks pretty good to me. Here's a revised version of
> >> your patch, with some renaming and other adjustments.
> >
> > Your version looks good to me.
> >
> >> I'm not sure
> >> exactly what you're referring to in set_append_rel_size,
> >
> > The below code:
> >
> > set_append_rel_size()
> > {
> > ..
> >
> > /*
> >
> > * CE failed, so finish copying/modifying targetlist and join quals.
> >
> > *
> > * NB: the resulting childrel->reltarget->exprs may contain arbitrary
> > * expressions, which otherwise would not occur in a rel's targetlist.
> > ..
> > */
> >
> > childrel->reltarget->exprs = (List *)
> > adjust_appendrel_attrs(root,
> > (Node *) rel->reltarget->exprs,
> > appinfo);
> >
> > What I mean to say is if above code lead to some additional expressions
in
> > childrel targetlist, then those can lead to some unsafe expressions in
child
> > rel target list. This can cause some problem as we are blindly
considering
> > that if parent rel is parallel safe, then child rel will also be
parallel
> > safe (in below code:)
> >
> > set_append_rel_size()
> > {
> > ..
> > /* Copy consider_parallel flag from parent. */
> > childrel->consider_parallel = rel->consider_parallel;
> >
> >
> >> but I did add
> >> a line of code there that might be relevant.
> >>
> >
> > I am not sure if that can address the above problem. I have posted my
> > analysis on the same in mail [1].
> >
> >
> > [1] -
> >
https://www.postgresql.org/message-id/CAA4eK1Jz5tG2D2PCNYqYob2cb4dKowKYwVJ7OmP5vwyRyCMx4g%40mail.gmail.com
>
> Do you have a test case that demonstrates the problem with this patch
> applied?
>

No, I don't have a test case. I have tried a bit, but it seems even if
there is a problem, one needs to spend quite some time to generate an exact
test. I think the current patch fixes the reproducible problem [1], so lets
go with it. We can look into Append case problem if some one has a
reproducible case or if I could come up with one. In any case, I think it
is a separate problem then what is reported in this thread.

[1] -
https://www.postgresql.org/message-id/CAFiTN-vzg5BkK6kAh3OMhvgRu-uJvkjz47ybtopMAfGJp%3DzWqA%40mail.gmail.com

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