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