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.etsuro@lab.ntt.co.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