The patch applies cleanly, compiles. make check in regress as well as postgres_fdw works fine. Here are few comments
local-join should be local join. The comments should explain why. + /* Should be unparameterized */ + Assert(outer_path->param_info == NULL); + Assert(inner_path->param_info == NULL); + a suitable local join path, which can be used as the alternative local May be we should reword this as ... which can be used to create an alternative local ... This rewording is required even in the existing docs. + /* outer_path should not require rels from inner_path */ + Assert(!PATH_PARAM_BY_REL(outer_path, inner_path->parent)); Probably this should throw an error or return NULL in such case rather than Asserting. This function is callable from any FDW, and that FDW may provide such paths, may be because of an internal bug. Same case with + /* Neither path should require rels from the other path */ + Assert(!PATH_PARAM_BY_REL(outer_path, inner_path->parent) || + !PATH_PARAM_BY_REL(inner_path, outer_path->parent)); While the comment below mentions ON true, the testcase you have added is for ON false. Either the testcase should change or this comment. That raises another question, what happens when somebody does FULL JOIN ON false? + * If special case: for "x FULL JOIN y ON true", there Why JOIN_RIGHT is being treated differently from JOIN_LEFT? We should be able to create a nested loop join for JOIN_RIGHT? + case JOIN_RIGHT: + case JOIN_FULL: On Thu, Mar 23, 2017 at 5:50 PM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: > On 2017/03/21 18:40, Etsuro Fujita wrote: >> >> Ok, I'll update the patch. One thing I'd like to revise in addition to >> that is (1) add to JoinPathExtraData a flag member to indicate whether >> to give the FDW a chance to consider a remote join, which will be set to >> true if the joinrel's fdwroutine is not NULL and the fdwroutine's >> GetForeignJoinPaths is not NULL, and (2) if the flag is true, save info >> to create an alternative local join path, such as hashclauses and >> mergeclauses proposed in the patch, into JoinPathExtraData in >> add_paths_to_joinrel. This would avoid useless overhead in saving such >> info into JoinPathExtraData when we don't give the FDW that chance. > > > Done. Attached is a new version of the patch. > > Best regards, > Etsuro Fujita -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
