On Mon, Feb 1, 2016 at 8:27 AM, Ashutosh Bapat <ashutosh.ba...@enterprisedb.com> wrote: > Here are patches rebased on recent commit > cc592c48c58d9c1920f8e2063756dcbcce79e4dd. Renamed original deparseSelectSql > as deparseSelectSqlForBaseRel and added deparseSelectSqlForJoinRel to > construct SELECT and FROM clauses for base and join relations. > > pg_fdw_core_v5.patch GetUserMappingById changes > pg_fdw_join_v5.patch: postgres_fdw changes for join pushdown including > suggestions as described below > pg_join_pd_v5.patch: combined patch for ease of testing. > > The patches also have following changes along with the changes described in > my last mail. > 1. Revised the way targetlists are handled. For a bare base relation the > SELECT clause is deparsed from fpinfo->attrs_used but for a base relation > which is part of join relation, the expected targetlist is passed down to > deparseSelectSqlForBaseRel(). This change removed 75 odd lines in > build_tlist_to_deparse() which were very similar to > deparseTargetListFromAttrsUsed() in the previous patch.
Nice! > 2. Refactored postgresGetForeignJoinPaths to be more readable moving the > code to assess safety of join pushdown into a separate function. That looks good. But maybe call the function foreign_join_ok() or something like that? is_foreign_join() isn't terrible but it sounds a little odd to me. The path-copying stuff in get_path_for_epq_recheck() looks a lot better now, but you neglected to add a comment explaining why you did it that way (e.g. "Make a shallow copy of the join path, because the planner might free the original structure after a future add_path(). We don't need to copy the substructure, though; that won't get freed." I would forget about setting merge_path->materialize_inner = false; that doesn't seem essential. Also, I would arrange things so that if you hit an unrecognized path type (like a custom join, or a gather) you skip that particular path instead of erroring out. I think this whole function should be moved to core, and I think the argument should be a RelOptInfo * rather than a List *. + * We can't know VERBOSE option is specified or not, so always add shcema We can't know "whether" VERBOSE... shcema -> schema + * the join relaiton is already considered, so that we won't waste time in Typo. + * judging safety of join pushdow and adding the same paths again if found Typo. More when I have a bit more time to look at this... -- 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
