Re: Push down more full joins in postgres_fdw

On Fri, Nov 4, 2016 at 9:19 AM, Etsuro Fujita
<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)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