On Thu, Jun 9, 2016 at 9:37 AM, Amit Kapila <amit.kapil...@gmail.com> wrote:
> On Thu, Jun 9, 2016 at 8:47 AM, Robert Haas <robertmh...@gmail.com> wrote:
>> On Mon, Jun 6, 2016 at 6:07 AM, Amit Kapila <amit.kapil...@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? I agree there may be a problem, but it would be easier to be
sure whether a given change fixes it if we had a good test case.
--
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
