On Fri, Nov 4, 2016 at 9:19 AM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: > On 2016/11/03 18:52, Ashutosh Bapat wrote: >>> >>> Here is the updated version, which includes the restructuring you >>> proposed. >>> Other than the above issue and the alias issue we discussed, I addressed >>> all >>> your comments except one on testing; I tried to add test cases where the >>> remote query is deparsed as nested subqueries, but I couldn't because >>> IIUC, >>> reduce_outer_joins reduced full joins to inner joins or left joins. > > >> No always. It will do so in some cases as explained in the prologue of >> reduce_outer_joins() >> >> * More generally, an outer join can be reduced in strength if there is a >> * strict qual above it in the qual tree that constrains a Var from the >> * nullable side of the join to be non-null. (For FULL joins this applies >> * to each side separately.) > > > Hmm. Would it be necessary for this patch to include test cases for nested > subqueries? As mentioned in a previous email, those test cases can be added > by the split patch that allow PHVs to be evaluated on the remote side.
A patch should have testcases testing the functionality added. This patch adds functionality to deparse nested subqueries, so there should be testcase to test it. If we can not come up with a testcase, then it's very much possible that the code related to that functionality is not needed. PHV is a separate patch and we do not know whether it will be committed or when it will be committed after this patch is committed. It's better to have self-sufficient patch. > >> This patch again has a lot of unrelated changes, esp. in >> deparseSelectStmtForRel(). What was earlier part of deparseSelectSql() >> and deparseFromExpr() is now flattened in deparseSelectStmtForRel(), >> which seems unnecessary. > > > IIUC, I think this change was done to address your comment (see the comment > #2 in [1]). Am I missing something? There is some misunderstanding here. That comment basically says, deparseRangeTblRef() duplicates code in deparseSelectStmtForRel(), so we should either remove deparseRangeTblRef() altogether or should keep it to minimal avoiding duplication of code. What might have confused you is the last sentence in that comment "This will also make the current changes to deparseSelectSql unnecessary." By current changes I meant changes to deparseSelectSql() in your patch, not the one that's in the repository. I don't think we should flatten deparseSelectStmtForRel() in this patch. > >> Or there are changes like >> >> -deparseSelectStmtForRel(StringInfo buf, PlannerInfo *root, RelOptInfo >> *rel, >> - List *tlist, List *remote_conds, List *pathkeys, >> +deparseSelectStmtForRel(StringInfo buf, PlannerInfo *root, >> + RelOptInfo *foreignrel, List *tlist, >> + List *remote_conds, List *pathkeys, >> >> which arise because rel has been renamed as foreignrel. The patch will >> work even without such renaming. > > > I did that because we have a Relation named rel (the same name!) within that > function, to execute deparseTargetList in a base-relation case. That's because you have flattened deparseSelectStmtForRel(). If we don't flatten it out, the variable name conflict doesn't arise. > Another > reason for that is because (1) that would match with other function > definitions in deparse.c and (2) that would be consistent with the existing > function definition for that function in postgres_fdw.h. > That would be a separate patch, I guess. -- 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
