|From:||Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>|
|To:||Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>|
|Subject:||Re: Push down more full joins in postgres_fdw|
|Views:||Raw Message | Whole Thread | Download mbox|
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:
>>>> Here is the updated version,
>>>> Other than the above issue and the alias issue we discussed, I addressed
>>>> 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
>>>> 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
>>> * 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.)
>> Would it be necessary for this patch to include test cases for nested
> 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 ). 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.
|Next Message||Etsuro Fujita||2016-11-04 11:20:16||Re: Typo in comment in contrib/postgres_fdw/deparse.c|
|Previous Message||Ashutosh Bapat||2016-11-04 10:52:06||Re: Partition-wise join for join between (declaratively) partitioned tables|