Re: Push down more full joins in postgres_fdw

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