On Fri, Feb 5, 2016 at 9:09 AM, Ashutosh Bapat <ashutosh.ba...@enterprisedb.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
