On 2016/06/15 0:50, Robert Haas wrote:
> On Tue, Jun 14, 2016 at 4:06 AM, Amit Langote wrote:
>> You're right. It indeed should be possible to push down ft1-ft2 join.
>> However it could not be done without also modifying
>> build_tlist_to_deparse() a little (as Ashutosh proposed [1] to do
>> upthread). Attached new version of the patch with following changes:
>>
>> 1) Modified the check in foreign_join_ok() so that it is not overly
>> restrictive. Previously, it used ph_needed as the highest level at which
>> the PHV is needed (by definition) and checked using it whether it is above
>> the join under consideration, which ended up being an overkill. ISTM, we
>> can just decide from joinrel->relids of the join under consideration
>> whether we are above the lowest level where the PHV could be evaluated
>> (ph_eval_at) and stop if so. So in the example you provided, it won't now
>> reject {ft1, ft2}, but will {ft4, ft1, ft2}.
>>
>> 2) Modified build_tlist_to_deparse() to get rid of the PlaceHolderVars in
>> the targetlist to deparse by using PVC_RECURSE_PLACEHOLDER flag of
>> pull_var_clause(). That is because we do not yet support anything other
>> than Vars in deparseExplicitTargetList() (+1 to your patch to change the
>> Assert to elog).
>
> OK, I committed this version with some cosmetic changes. I ripped out
> the header comment to foreign_join_ok(), which is just a nuisance to
> maintain. It unnecessarily recapitulates the tests contained within
> the function, requiring us to update the comments in two places
> instead of one every time we make a change for no real gain. And I
> rewrote the comment you added.
Thanks.
Regards,
Amit
