From: | Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> |
---|---|
To: | Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com> |
Cc: | David Steele <david(at)pgmasters(dot)net>, Robert Haas <robertmhaas(at)gmail(dot)com>, Jeff Janes <jeff(dot)janes(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: postgres_fdw bug in 9.6 |
Date: | 2017-03-31 12:50:57 |
Message-ID: | b00c8024-b4f3-470b-f911-22a9c81d71ac@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 2017/03/30 20:16, Ashutosh Bapat wrote:
> The patch applies cleanly, compiles. make check in regress as well as
> postgres_fdw works fine. Here are few comments
Thanks for the review!
> local-join should be local join.
OK, done.
> The comments should explain why.
> + /* Should be unparameterized */
> + Assert(outer_path->param_info == NULL);
> + Assert(inner_path->param_info == NULL);
Done.
> + a suitable local join path, which can be used as the alternative local
> May be we should reword this as ... which can be used to create an alternative
> local ... This rewording is required even in the existing docs.
Done.
> + /* outer_path should not require rels from inner_path */
> + Assert(!PATH_PARAM_BY_REL(outer_path, inner_path->parent));
> Probably this should throw an error or return NULL in such case rather than
> Asserting. This function is callable from any FDW, and that FDW may provide
> such paths, may be because of an internal bug. Same case with
> + /* Neither path should require rels from the other path */
> + Assert(!PATH_PARAM_BY_REL(outer_path, inner_path->parent) ||
> + !PATH_PARAM_BY_REL(inner_path, outer_path->parent));
Good idea! I think it's better to throw an error because that is a bug
in the FDW; done that way.
> While the comment below mentions ON true, the testcase you have added is for ON
> false. Either the testcase should change or this comment. That raises another
> question, what happens when somebody does FULL JOIN ON false?
> + * If special case: for "x FULL JOIN y ON true", there
FULL JOIN ON FALSE would be handled the same way as FULL JOIN ON TRUE,
so I think we should rewrite that comment into something like this: If
special case: for "x FULL JOIN y ON true" or "x FULL JOIN y ON false"...
> Why JOIN_RIGHT is being treated differently from JOIN_LEFT? We should be able
> to create a nested loop join for JOIN_RIGHT?
> + case JOIN_RIGHT:
> + case JOIN_FULL:
I don't think so, because nestloop joins aren't supported for
JOIN_RIGHT. See ExecInitNestLoop().
Best regards,
Etsuro Fujita
Attachment | Content-Type | Size |
---|---|---|
epqpath-for-foreignjoin-7.patch | text/x-patch | 46.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Ashutosh Bapat | 2017-03-31 12:58:47 | Re: [bug fix] Savepoint-related statements terminates connection |
Previous Message | Rafia Sabih | 2017-03-31 11:53:43 | Re: TPC-H Q20 from 1 hour to 19 hours! |