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.

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?

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. 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.

Best regards,
Etsuro Fujita

[1] https://www.postgresql.org/message-id/CAFjFpRfwoSsJr9418b2jA7g0nwagjZSWhPQPUmK9M6z5XSOAqQ%40mail.gmail.com




--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to