On 2016/11/21 22:02, Ashutosh Bapat wrote:
You wrote:
>>>> Instead we should calculate tlist, the
>>>> first time we need it and then add it to the fpinfo.
I wrote:
>> Having said that, I agree on that point. I'd like to propose (1) adding a
>> new member to fpinfo, to store a list of output Vars from the subquery, and
>> (2) creating a tlist from it in deparseRangeTblRef, then, which would allow
>> us to calculate the tlist only when we need it. The member added to fpinfo
>> would be also useful to address the comments on the DML/UPDATE pushdown
>> patch. See the attached patch in [1]. I named the member a bit differently
>> in that patch, though.
> Again the list of Vars will be wasted if we don't choose that path for
> final planning. So, I don't see the point of adding list of Vars
> there.
> If you think that the member is useful for DML/UDPATE pushdown,
> you may want to add it in the other patch.
OK, I'd like to propose referencing to foreignrel->reltarget->exprs
directly in deparseRangeTblRef and get_subselect_alias_id, then, which
is the same as what I proposed in the first version of the patch, but
I'd also like to change deparseRangeTblRef to use add_to_flat_tlist, not
make_tlist_from_pathtarget, to create a tlist of the subquery, as you
proposed.
>> You modified the comments I added to deparseLockingClause into this:
>>
>> /*
>> + * Ignore relation if it appears in a lower subquery. Locking clause
>> + * for such a relation is included in the subquery.
>> + */
>>
>> I don't think the second sentence is 100% correct because a locking clause
>> isn't always required for such a relation, so I modified the sentence a bit.
> I guess, "if required" part was implicit in construct "such a
> relation". Your version seems to make it explicit. I am fine with it.
OK, let's leave that for the committer's judge.
Please find attached an updated version of the patch.
Best regards,
Etsuro Fujita