Re: Push down more full joins in postgres_fdw

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