Re: Push down more full joins in postgres_fdw

On 2016/11/04 13:08, Ashutosh Bapat wrote:
> 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:

I wrote:
>>>> Here is the updated version,

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

Right.

>> Would it be necessary for this patch to include test cases for nested
>> subqueries?

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

OK, I added such a test case.

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

I noticed that I misunderstood your words. Sorry about that. I agree
with you, so I removed that change from the patch.

Attached is an updated version of the patch.

Best regards,
Etsuro Fujita