Re: postgres_fdw join pushdown (was Re: Custom/Foreign-Join-APIs)

On Fri, Feb 5, 2016 at 9:09 AM, Ashutosh Bapat
<ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
>> Would it be nuts to set fdw_scan_tlist in all cases? Would the code
>> come out simpler than what we have now?
>
> PFA the patch that can be applied on v9 patches.
>
> If there is a whole-row reference for base relation, instead of adding that
> as an additional column deparseTargetList() creates a list of all the
> attributes of that foreign table . The whole-row reference gets constructed
> during projection. This saves some network bandwidth while transferring the
> data from the foreign server. If we build the target list for base relation,
> we should include Vars for all the columns (like deparseTargetList). Thus we
> borrow some code from deparseTargetList to get all the attributes of a
> relation. I included this logic in function build_tlist_from_attrs_used(),
> which is being called by build_tlist_to_deparse(). So, before calling
> deparseSelectStmtForRel() the callers can just call build_tlist_to_deparse()
> and pass the targetlist to deparseSelectStmtForRel() and use the same to be
> handed over to the core code as fdw_scan_tlist. build_tlist_to_deparse()
> also constructs retrieved_attrs list, so that doesn't need to be passed
> around in deparse routines.
>
> But we now have similar code in three places deparseTargetList(),
> deparseAnalyzeSql() and build_tlist_from_attrs_used(). So, I re-wrote
> deparseTargetList() (which is now used to deparse returning list) to call
> build_tlist_from_attrs_used() followed by deparseExplicitTargetList(). The
> later and its minion deparseVar requires a deparse_expr_cxt to be passed.
> deparse_expr_cxt has a member to store RelOptInfo of the relation being
> deparsed. The callers of deparseReturningList() do not have it and thus
> deparse_expr_cxt required changes, which in turn required changes in other
> places. All in all, a larger refactoring. It touches more places than
> necessary for foreign join push down. So, I think, we should try that as a
> separate refactoring work. We may just do the work described in the first
> paragraph for join pushdown, but we will then be left with duplicate code,
> which I don't think is worth the output.

Yeah, I'm not convinced this is actually simpler; at first look, it
seems like it is just moving the complexity around. I don't like the
fact that there are so many places where we have to do one thing for a
baserel and something totally different for a joinrel, which was the
point of my comment. But this seems to add one instance of that and
remove one instance of that, so I don't see how we're coming out
better than a wash. Maybe it's got more merit than I'm giving it
credit for, and I'm just not seeing it right at the moment...

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company