Re: [sqlsmith] Failed assertion in parallel worker (ExecInitSubPlan)
От | Amit Kapila |
---|---|
Тема | Re: [sqlsmith] Failed assertion in parallel worker (ExecInitSubPlan) |
Дата | |
Msg-id | CAA4eK1JOXsjp4yW5D4y63=Mv3YB5WjDDyRmhjFveKHK92ptuLw@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: [sqlsmith] Failed assertion in parallel worker (ExecInitSubPlan) (Robert Haas <robertmhaas@gmail.com>) |
Ответы |
Re: [sqlsmith] Failed assertion in parallel worker (ExecInitSubPlan)
(Robert Haas <robertmhaas@gmail.com>)
|
Список | pgsql-hackers |
On Thu, Jun 9, 2016 at 7:44 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Thu, Jun 9, 2016 at 9:37 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > On Thu, Jun 9, 2016 at 8:47 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> >> On Mon, Jun 6, 2016 at 6:07 AM, Amit Kapila <amit.kapila16@gmail.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?
>
>
> On Thu, Jun 9, 2016 at 9:37 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > On Thu, Jun 9, 2016 at 8:47 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> >> On Mon, Jun 6, 2016 at 6:07 AM, Amit Kapila <amit.kapila16@gmail.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.
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Kevin GrittnerДата:
Сообщение: Re: Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold <