Re: [HACKERS] postgres_fdw bug in 9.6

2017-10-05 Thread Tom Lane
Robert Haas writes: > On Tue, Aug 29, 2017 at 5:14 PM, Tom Lane wrote: >> We'd definitely need to do things that way in 9.6. I'm not quite sure >> whether it's too late to adopt the clean solution in v10. > It probably is now. Are you still planning

Re: [HACKERS] postgres_fdw bug in 9.6

2017-10-05 Thread Robert Haas
On Tue, Aug 29, 2017 at 5:14 PM, Tom Lane wrote: > Etsuro Fujita writes: >> [ epqpath-for-foreignjoin-11.patch ] > > I started looking at this. I've not yet wrapped my head around what > CreateLocalJoinPath() is doing, but I noted that Robert's

Re: [HACKERS] postgres_fdw bug in 9.6

2017-09-19 Thread Ashutosh Bapat
On Tue, Sep 5, 2017 at 1:10 PM, Etsuro Fujita wrote: > > > I think Tom is reviewing this patch [1]. > I am marking this as ready for committer as I don't have any new comments and possibly other reviewers have also done reviewing it. -- Best Wishes, Ashutosh Bapat

Re: [HACKERS] postgres_fdw bug in 9.6

2017-09-05 Thread Ryan Murphy
> I tested that with HEAD, but couldn't reproduce this problem. Didn't you test that with HEAD? Yeah, I know it's not an issue other people are having - I think it's something specific about how my environment / postgres build is set up. In any case I knew that it wasn't caused by your patch.

Re: [HACKERS] postgres_fdw bug in 9.6

2017-09-05 Thread Etsuro Fujita
On 2017/09/05 13:43, Ryan Murphy wrote: The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: not tested Spec compliant: not tested Documentation:not tested This feature is obviously a

Re: [HACKERS] postgres_fdw bug in 9.6

2017-09-04 Thread Ryan Murphy
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: not tested Spec compliant: not tested Documentation:not tested This feature is obviously a pretty deep cut, and I don't understand the

Re: [HACKERS] postgres_fdw bug in 9.6

2017-08-29 Thread Tom Lane
Etsuro Fujita writes: > [ epqpath-for-foreignjoin-11.patch ] I started looking at this. I've not yet wrapped my head around what CreateLocalJoinPath() is doing, but I noted that Robert's concerns about ABI breakage in the back branches would not be very difficult to

Re: [HACKERS] postgres_fdw bug in 9.6

2017-08-29 Thread Etsuro Fujita
On 2017/08/21 20:37, Etsuro Fujita wrote: Attached is an updated version of the patch. I corrected some comments. New version attached. Best regards, Etsuro Fujita *** a/contrib/postgres_fdw/expected/postgres_fdw.out --- b/contrib/postgres_fdw/expected/postgres_fdw.out *** ***

Re: [HACKERS] postgres_fdw bug in 9.6

2017-08-21 Thread Etsuro Fujita
On 2017/04/08 4:24, Robert Haas wrote: Looking at the code itself, I find the changes to joinpath.c rather alarming. I missed this mail. Sorry about that, Robert. +/* Save hashclauses for possible use by the FDW */ +if (extra->consider_foreignjoin && hashclauses) +

Re: [HACKERS] postgres_fdw bug in 9.6

2017-08-17 Thread Etsuro Fujita
On 2017/08/18 8:16, Michael Paquier wrote: On Fri, Aug 18, 2017 at 3:59 AM, Tom Lane wrote: Robert Haas writes: On Thu, Aug 17, 2017 at 8:00 AM, Etsuro Fujita wrote: I rebased the patch to HEAD. PFA a new version of

Re: [HACKERS] postgres_fdw bug in 9.6

2017-08-17 Thread Michael Paquier
On Fri, Aug 18, 2017 at 3:59 AM, Tom Lane wrote: > Robert Haas writes: >> On Thu, Aug 17, 2017 at 8:00 AM, Etsuro Fujita >> wrote: >>> I rebased the patch to HEAD. PFA a new version of the patch. > >> Tom, you were

Re: [HACKERS] postgres_fdw bug in 9.6

2017-08-17 Thread Tom Lane
Robert Haas writes: > On Thu, Aug 17, 2017 at 8:00 AM, Etsuro Fujita > wrote: >> I rebased the patch to HEAD. PFA a new version of the patch. > Tom, you were instrumental in identifying what was going wrong here > initially. Any chance you'd

Re: [HACKERS] postgres_fdw bug in 9.6

2017-08-17 Thread Robert Haas
On Thu, Aug 17, 2017 at 8:00 AM, Etsuro Fujita wrote: > On 2017/04/04 18:01, Etsuro Fujita wrote: >> I rebased the patch also. Please find attached an updated version of the >> patch. > > I rebased the patch to HEAD. PFA a new version of the patch. Tom, you were

Re: [HACKERS] postgres_fdw bug in 9.6

2017-08-17 Thread Etsuro Fujita
On 2017/04/04 18:01, Etsuro Fujita wrote: I rebased the patch also. Please find attached an updated version of the patch. I rebased the patch to HEAD. PFA a new version of the patch. Best regards, Etsuro Fujita *** a/contrib/postgres_fdw/expected/postgres_fdw.out ---

Re: [HACKERS] postgres_fdw bug in 9.6

2017-04-11 Thread Etsuro Fujita
On 2017/04/08 3:33, Robert Haas wrote: On Mon, Apr 3, 2017 at 2:12 AM, Etsuro Fujita wrote: On 2017/04/01 1:32, Jeff Janes wrote: On Thu, Mar 23, 2017 at 5:20 AM, Etsuro Fujita > wrote: Done.

Re: [HACKERS] postgres_fdw bug in 9.6

2017-04-09 Thread Ashutosh Bapat
On Sat, Apr 8, 2017 at 12:03 AM, Robert Haas wrote: > On Mon, Apr 3, 2017 at 2:12 AM, Etsuro Fujita > wrote: >> On 2017/04/01 1:32, Jeff Janes wrote: >>> On Thu, Mar 23, 2017 at 5:20 AM, Etsuro Fujita >>>

Re: [HACKERS] postgres_fdw bug in 9.6

2017-04-08 Thread David Steele
On 4/7/17 3:24 PM, Robert Haas wrote: > > There's probably more to think about here, but those are my question > on an initial read-through. This bug has been moved to CF 2017-07. -- -David da...@pgmasters.net -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make

Re: [HACKERS] postgres_fdw bug in 9.6

2017-04-07 Thread Robert Haas
On Fri, Apr 7, 2017 at 2:33 PM, Robert Haas wrote: > On Mon, Apr 3, 2017 at 2:12 AM, Etsuro Fujita > wrote: >> On 2017/04/01 1:32, Jeff Janes wrote: >>> On Thu, Mar 23, 2017 at 5:20 AM, Etsuro Fujita >>>

Re: [HACKERS] postgres_fdw bug in 9.6

2017-04-07 Thread Robert Haas
On Mon, Apr 3, 2017 at 2:12 AM, Etsuro Fujita wrote: > On 2017/04/01 1:32, Jeff Janes wrote: >> On Thu, Mar 23, 2017 at 5:20 AM, Etsuro Fujita >> > wrote: >> Done. Attached is a new version of the

Re: [HACKERS] postgres_fdw bug in 9.6

2017-04-05 Thread Etsuro Fujita
On 2017/04/04 21:28, Ashutosh Bapat wrote: On Tue, Apr 4, 2017 at 2:31 PM, Etsuro Fujita > wrote: I rebased the patch also. Please find attached an updated version of the patch. Thanks, marking this as "ready for

Re: [HACKERS] postgres_fdw bug in 9.6

2017-04-04 Thread Ashutosh Bapat
On Tue, Apr 4, 2017 at 2:31 PM, Etsuro Fujita wrote: > > I rebased the patch also. Please find attached an updated version of the > patch. > Thanks, marking this as "ready for committer". -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres

Re: [HACKERS] postgres_fdw bug in 9.6

2017-04-04 Thread Etsuro Fujita
On 2017/04/04 14:38, Ashutosh Bapat wrote: Probably we should use "could not be created" instead of "was not created" in "... a local path suitable for EPQ checks was not created". Done. "outer_path should not require relations from inner_path" may be reworded as "outer paths should not be

Re: [HACKERS] postgres_fdw bug in 9.6

2017-04-03 Thread Ashutosh Bapat
Probably we should use "could not be created" instead of "was not created" in "... a local path suitable for EPQ checks was not created". "outer_path should not require relations from inner_path" may be reworded as "outer paths should not be parameterized by the inner relations". "neither path

Re: [HACKERS] postgres_fdw bug in 9.6

2017-04-03 Thread Etsuro Fujita
On 2017/04/01 1:32, Jeff Janes wrote: On Thu, Mar 23, 2017 at 5:20 AM, Etsuro Fujita > wrote: Done. Attached is a new version of the patch. Is the fix for 9.6.3 going to be just a back port of this, or will it look

Re: [HACKERS] postgres_fdw bug in 9.6

2017-03-31 Thread Jeff Janes
On Thu, Mar 23, 2017 at 5:20 AM, Etsuro Fujita wrote: > On 2017/03/21 18:40, Etsuro Fujita wrote: > >> Ok, I'll update the patch. One thing I'd like to revise in addition to >> that is (1) add to JoinPathExtraData a flag member to indicate whether >> to give the FDW

Re: [HACKERS] postgres_fdw bug in 9.6

2017-03-31 Thread Etsuro Fujita
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

Re: [HACKERS] postgres_fdw bug in 9.6

2017-03-30 Thread Ashutosh Bapat
The patch applies cleanly, compiles. make check in regress as well as postgres_fdw works fine. Here are few comments local-join should be local join. The comments should explain why. +/* Should be unparameterized */ +Assert(outer_path->param_info == NULL); +

Re: [HACKERS] postgres_fdw bug in 9.6

2017-03-23 Thread Etsuro Fujita
On 2017/03/21 18:40, Etsuro Fujita wrote: Ok, I'll update the patch. One thing I'd like to revise in addition to that is (1) add to JoinPathExtraData a flag member to indicate whether to give the FDW a chance to consider a remote join, which will be set to true if the joinrel's fdwroutine is

Re: [HACKERS] postgres_fdw bug in 9.6

2017-03-21 Thread Etsuro Fujita
On 2017/03/17 0:37, David Steele wrote: This patch does not apply cleanly at cccbdde: Marked "Waiting on Author". Ok, I'll update the patch. One thing I'd like to revise in addition to that is (1) add to JoinPathExtraData a flag member to indicate whether to give the FDW a chance to

Re: [HACKERS] postgres_fdw bug in 9.6

2017-03-16 Thread David Steele
On 1/23/17 4:56 AM, Etsuro Fujita wrote: > On 2017/01/20 14:24, Ashutosh Bapat wrote: >> On Thu, Jan 19, 2017 at 10:38 PM, Robert Haas >> wrote: >>> On Wed, Jan 18, 2017 at 10:26 PM, Ashutosh Bapat >>> wrote: > Yes, I think that's

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-31 Thread Michael Paquier
On Mon, Jan 23, 2017 at 6:56 PM, Etsuro Fujita wrote: > Other changes: > > * Also modified CreateLocalJoinPath so that we pass outer/inner paths, not > outer/inner rels, because it would be more flexible for the FDW to build the > local-join path from paths it chose.

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-23 Thread Etsuro Fujita
On 2017/01/20 14:24, Ashutosh Bapat wrote: On Thu, Jan 19, 2017 at 10:38 PM, Robert Haas wrote: On Wed, Jan 18, 2017 at 10:26 PM, Ashutosh Bapat wrote: Yes, I think that's broadly the approach Tom was recommending. I don't have

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-19 Thread Ashutosh Bapat
On Thu, Jan 19, 2017 at 10:38 PM, Robert Haas wrote: > On Wed, Jan 18, 2017 at 10:26 PM, Ashutosh Bapat > wrote: >>> Yes, I think that's broadly the approach Tom was recommending. >> >> I don't have problem with that approach, but the

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-19 Thread Etsuro Fujita
On 2017/01/19 12:26, Ashutosh Bapat wrote: On Thu, Jan 19, 2017 at 2:14 AM, Robert Haas wrote: On Fri, Jan 13, 2017 at 6:22 AM, Etsuro Fujita wrote: My biggest concern about GetExistingLocalJoinPath is that might not be extendable to the

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-19 Thread Etsuro Fujita
On 2017/01/18 20:34, Ashutosh Bapat wrote: On Wed, Jan 18, 2017 at 8:18 AM, Etsuro Fujita wrote: Done. Attached is the new version. I also adjusted the code a bit further. With this patch there are multiple cases, where CreateLocalJoinPath() would return NULL

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-19 Thread Robert Haas
On Wed, Jan 18, 2017 at 10:26 PM, Ashutosh Bapat wrote: >> Yes, I think that's broadly the approach Tom was recommending. > > I don't have problem with that approach, but the latest patch has > following problems. > 1. We are copying chunks of path creation logic,

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-18 Thread Ashutosh Bapat
On Thu, Jan 19, 2017 at 2:14 AM, Robert Haas wrote: > On Fri, Jan 13, 2017 at 6:22 AM, Etsuro Fujita > wrote: >> My biggest concern about GetExistingLocalJoinPath is that might not be >> extendable to the case of foreign-join paths with

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-18 Thread Robert Haas
On Fri, Jan 13, 2017 at 6:22 AM, Etsuro Fujita wrote: > My biggest concern about GetExistingLocalJoinPath is that might not be > extendable to the case of foreign-join paths with parameterization; in which > case, fdw_outerpath for a given foreign-join path would need

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-18 Thread Ashutosh Bapat
On Wed, Jan 18, 2017 at 8:18 AM, Etsuro Fujita wrote: > On 2017/01/16 11:38, Etsuro Fujita wrote: >> >> On 2017/01/14 6:39, Jeff Janes wrote: >>> >>> I do get a compiler warning: >>> >>> foreign.c: In function 'CreateLocalJoinPath': >>> foreign.c:832: warning:

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-17 Thread Etsuro Fujita
On 2017/01/16 11:38, Etsuro Fujita wrote: On 2017/01/14 6:39, Jeff Janes wrote: I do get a compiler warning: foreign.c: In function 'CreateLocalJoinPath': foreign.c:832: warning: implicit declaration of function 'pathkeys_contained_in' Will fix. Done. Attached is the new version. I also

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-15 Thread Etsuro Fujita
On 2017/01/14 6:39, Jeff Janes wrote: I've tested this patch against 9.6.stable-2d443ae1b0121e15265864d2b21 and 10.dev-0563a3a8b59150bf3cc8, and in both cases it fixes the original problem. Thanks for testing! I do get a compiler warning: foreign.c: In function 'CreateLocalJoinPath':

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-13 Thread Jeff Janes
On Fri, Jan 13, 2017 at 3:22 AM, Etsuro Fujita wrote: > On 2017/01/13 0:43, Robert Haas wrote: > >> On Wed, Jan 11, 2017 at 2:45 AM, Etsuro Fujita >> wrote: >> >>> As I said before, that might be fine for 9.6, but I don't think it's a

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-13 Thread Etsuro Fujita
On 2017/01/13 0:43, Robert Haas wrote: On Wed, Jan 11, 2017 at 2:45 AM, Etsuro Fujita wrote: As I said before, that might be fine for 9.6, but I don't think it's a good idea to search the pathlist because once we support parameterized foreign join paths, which is

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-12 Thread Robert Haas
On Wed, Jan 11, 2017 at 2:45 AM, Etsuro Fujita wrote: > As I said before, that might be fine for 9.6, but I don't think it's a good > idea to search the pathlist because once we support parameterized foreign > join paths, which is on my TODOs, we would have to

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-11 Thread Etsuro Fujita
On 2017/01/11 19:26, Ashutosh Bapat wrote: On Wed, Jan 11, 2017 at 3:30 PM, Etsuro Fujita wrote: On 2017/01/11 17:51, Ashutosh Bapat wrote: On Wed, Jan 11, 2017 at 1:15 PM, Etsuro Fujita wrote: On 2017/01/11 13:40, Ashutosh Bapat

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-11 Thread Ashutosh Bapat
On Wed, Jan 11, 2017 at 3:30 PM, Etsuro Fujita wrote: > On 2017/01/11 17:51, Ashutosh Bapat wrote: >> >> On Wed, Jan 11, 2017 at 1:15 PM, Etsuro Fujita >> wrote: >>> >>> On 2017/01/11 13:40, Ashutosh Bapat wrote:

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-11 Thread Etsuro Fujita
On 2017/01/11 17:51, Ashutosh Bapat wrote: On Wed, Jan 11, 2017 at 1:15 PM, Etsuro Fujita wrote: On 2017/01/11 13:40, Ashutosh Bapat wrote: CreateLocalJoinPath tries to construct a nestloop path for the given join relation because it wants to avoid merge/hash join

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-11 Thread Ashutosh Bapat
On Wed, Jan 11, 2017 at 1:15 PM, Etsuro Fujita wrote: > On 2017/01/11 13:40, Ashutosh Bapat wrote: >> >> CreateLocalJoinPath tries to construct a nestloop path for the given >> join relation because it wants to avoid merge/hash join paths which >> require expensive

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-10 Thread Etsuro Fujita
On 2017/01/11 13:40, Ashutosh Bapat wrote: CreateLocalJoinPath tries to construct a nestloop path for the given join relation because it wants to avoid merge/hash join paths which require expensive setup not required for EPQ. But it chooses cheap paths for joining relations which may not be

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-10 Thread Ashutosh Bapat
On Fri, Jan 6, 2017 at 6:31 PM, Etsuro Fujita wrote: > On 2017/01/05 12:10, Etsuro Fujita wrote: >> >> On 2016/12/28 17:34, Ashutosh Bapat wrote: >>> >>> Hmm. If I understand the patch correctly, it does not return any path >>> when merge join is allowed and there are

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-10 Thread Ashutosh Bapat
> >> If the inner and/or outer paths are not ordered as required, we will need >> to >> order them. Code below doesn't seem to handle that case. >> /* >> * If the paths are already well enough ordered, we >> can >> * skip doing an

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-09 Thread Etsuro Fujita
On 2017/01/09 22:36, Ashutosh Bapat wrote: I wrote: Done. Attached is the new version of the patch. Why is this so? * If the outer cheapest-total path is parameterized by the inner * rel, we can't generate a nestloop path. That parameterization means that for

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-09 Thread Ashutosh Bapat
> > Done. Attached is the new version of the patch. > > * I'm still not sure the search approach is the right way to go, so I > modified CreateLocalJoinPath so that it creates a mergejoin path that > explicitly sorts both the outer and inner relations as in > sort_inner_and_outer, by using the

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-06 Thread Etsuro Fujita
On 2017/01/05 12:10, Etsuro Fujita wrote: On 2016/12/28 17:34, Ashutosh Bapat wrote: Hmm. If I understand the patch correctly, it does not return any path when merge join is allowed and there are merge clauses but no hash clauses. In this case we will not create a foreign join path, loosing

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-04 Thread Etsuro Fujita
On 2017/01/05 13:19, Ashutosh Bapat wrote: Hmm. If I understand the patch correctly, it does not return any path when merge join is allowed and there are merge clauses but no hash clauses. In this case we will not create a foreign join path, loosing some optimization. If we remove

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-04 Thread Ashutosh Bapat
> >> Hmm. If I understand the patch correctly, it does not return any path >> when merge join is allowed and there are merge clauses but no hash >> clauses. In this case we will not create a foreign join path, loosing >> some optimization. If we remove GetExistingLocalJoinPath, which >> returns a

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-04 Thread Ashutosh Bapat
On Thu, Jan 5, 2017 at 8:20 AM, Etsuro Fujita wrote: > On 2016/12/27 16:41, Etsuro Fujita wrote: >> >> On 2016/12/22 1:04, Ashutosh Bapat wrote: >>> >>> 2. We should try to look for other not-so-cheap paths if the cheapest >>> one is >>> paramterized. You might want

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-04 Thread Etsuro Fujita
On 2016/12/28 17:34, Ashutosh Bapat wrote: On Wed, Dec 28, 2016 at 1:29 PM, Etsuro Fujita wrote: On 2016/12/28 15:54, Ashutosh Bapat wrote: On Wed, Dec 28, 2016 at 9:20 AM, Etsuro Fujita wrote: On 2016/12/27 22:03, Ashutosh Bapat

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-04 Thread Etsuro Fujita
On 2016/12/27 22:03, Ashutosh Bapat wrote: You wrote: 3. Talking about saving some CPU cycles - if a clauseless full join can be implemented only using merge join, probably that's the only path available in the list of paths for the given relation. Instead of building the same path anew, should

Re: [HACKERS] postgres_fdw bug in 9.6

2017-01-04 Thread Etsuro Fujita
On 2016/12/27 16:41, Etsuro Fujita wrote: On 2016/12/22 1:04, Ashutosh Bapat wrote: 2. We should try to look for other not-so-cheap paths if the cheapest one is paramterized. You might want to use get_cheapest_path_for_pathkeys() to find a suitable unparameterized path by passing NULL for

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-28 Thread Ashutosh Bapat
On Wed, Dec 28, 2016 at 1:29 PM, Etsuro Fujita wrote: > On 2016/12/28 15:54, Ashutosh Bapat wrote: >> >> On Wed, Dec 28, 2016 at 9:20 AM, Etsuro Fujita >> wrote: >>> >>> On 2016/12/27 22:03, Ashutosh Bapat wrote: If

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-28 Thread Etsuro Fujita
On 2016/12/28 15:54, Ashutosh Bapat wrote: On Wed, Dec 28, 2016 at 9:20 AM, Etsuro Fujita wrote: On 2016/12/27 22:03, Ashutosh Bapat wrote: If mergejoin_allowed is true and mergeclauselist is non-NIL but hashclauselist is NIL (that's rare but there can be types

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-27 Thread Ashutosh Bapat
On Wed, Dec 28, 2016 at 9:20 AM, Etsuro Fujita wrote: > On 2016/12/27 22:03, Ashutosh Bapat wrote: >> >> If mergejoin_allowed is true and mergeclauselist is non-NIL but >> hashclauselist is NIL (that's rare but there can be types has merge >> operators but not hash

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-27 Thread Etsuro Fujita
On 2016/12/27 22:03, Ashutosh Bapat wrote: If mergejoin_allowed is true and mergeclauselist is non-NIL but hashclauselist is NIL (that's rare but there can be types has merge operators but not hash operators), we will end up returning NULL. I think we want to create a merge join in that case. I

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-27 Thread Ashutosh Bapat
> >> 3. Talking about saving some CPU cycles - if a clauseless full join can be >> implemented only using merge join, probably that's the only path available >> in >> the list of paths for the given relation. Instead of building the same >> path >> anew, should we use the existing path like

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-26 Thread Etsuro Fujita
On 2016/12/22 1:04, Ashutosh Bapat wrote: Some review comments Thanks for the review! 2. We should try to look for other not-so-cheap paths if the cheapest one is paramterized. You might want to use get_cheapest_path_for_pathkeys() to find a suitable unparameterized path by passing NULL for

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-26 Thread Etsuro Fujita
On 2016/12/23 1:04, Robert Haas wrote: On Wed, Dec 21, 2016 at 11:04 AM, Ashutosh Bapat wrote: Some review comments 1. postgres_fdw doesn't push down semi and anti joins so you may want to discount these two too. + jointype == JOIN_SEMI || +

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-26 Thread Etsuro Fujita
On 2016/12/21 21:44, Etsuro Fujita wrote: On 2016/12/20 0:37, Tom Lane wrote: Etsuro Fujita writes: On 2016/12/17 1:13, Tom Lane wrote: So I think the rule could be "When first asked to produce a path for a given foreign joinrel, collect the cheapest paths for

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-22 Thread Robert Haas
On Wed, Dec 21, 2016 at 11:04 AM, Ashutosh Bapat wrote: > Some review comments > > 1. postgres_fdw doesn't push down semi and anti joins so you may want to > discount these two too. > + jointype == JOIN_SEMI || > + jointype == JOIN_ANTI); But

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-21 Thread Ashutosh Bapat
Some review comments 1. postgres_fdw doesn't push down semi and anti joins so you may want to discount these two too. + jointype == JOIN_SEMI || + jointype == JOIN_ANTI); 2. We should try to look for other not-so-cheap paths if the cheapest one is paramterized. You might want

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-21 Thread Etsuro Fujita
On 2016/12/20 0:37, Tom Lane wrote: Etsuro Fujita writes: On 2016/12/17 1:13, Tom Lane wrote: So I think the rule could be "When first asked to produce a path for a given foreign joinrel, collect the cheapest paths for its left and right inputs, and make a

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-19 Thread Tom Lane
Etsuro Fujita writes: > On 2016/12/17 1:13, Tom Lane wrote: >> So I think the rule could be >> "When first asked to produce a path for a given foreign joinrel, collect >> the cheapest paths for its left and right inputs, and make a nestloop path >> (or hashjoin path,

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-19 Thread Etsuro Fujita
On 2016/12/19 13:59, Ashutosh Bapat wrote: On Fri, Dec 16, 2016 at 9:43 PM, Tom Lane wrote: Etsuro Fujita writes: On 2016/12/16 11:25, Etsuro Fujita wrote: As I said upthread, an alternative I am thinking is (1) to create an equivalent

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-18 Thread Ashutosh Bapat
On Fri, Dec 16, 2016 at 9:43 PM, Tom Lane wrote: > Etsuro Fujita writes: >> On 2016/12/16 11:25, Etsuro Fujita wrote: >>> As I said upthread, an alternative I am thinking is (1) to create an >>> equivalent nestloop join path using inner/outer

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-18 Thread Etsuro Fujita
On 2016/12/17 1:13, Tom Lane wrote: Etsuro Fujita writes: On 2016/12/16 11:25, Etsuro Fujita wrote: As I said upthread, an alternative I am thinking is (1) to create an equivalent nestloop join path using inner/outer paths of a foreign join path, except when that

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-16 Thread Tom Lane
Etsuro Fujita writes: > On 2016/12/16 11:25, Etsuro Fujita wrote: >> As I said upthread, an alternative I am thinking is (1) to create an >> equivalent nestloop join path using inner/outer paths of a foreign join >> path, except when that join path implements a full

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-16 Thread Etsuro Fujita
On 2016/12/16 11:25, Etsuro Fujita wrote: As I said upthread, an alternative I am thinking is (1) to create an equivalent nestloop join path using inner/outer paths of a foreign join path, except when that join path implements a full join, in which case a merge/hash join path is used, (2) store

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-15 Thread Etsuro Fujita
On 2016/12/16 1:39, Tom Lane wrote: Etsuro Fujita writes: On 2016/12/13 23:13, Ashutosh Bapat wrote: A possible short-term fix may be this: instead of picking any random path to stick into fdw_outerpath, we choose a path which covers the pathkeys of ForeignPath.

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-15 Thread Tom Lane
Etsuro Fujita writes: > On 2016/12/13 23:13, Ashutosh Bapat wrote: >> On Tue, Dec 13, 2016 at 4:15 AM, Tom Lane wrote: >>> One way that we could make things better is to rely on the knowledge >>> that EPQ isn't asked to evaluate joins for more

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-15 Thread Etsuro Fujita
On 2016/12/13 23:13, Ashutosh Bapat wrote: On Tue, Dec 13, 2016 at 4:15 AM, Tom Lane wrote: I believe there are probably more problems here, or at least if there aren't, it's not clear why not. Because of GetExistingLocalJoinPath's lack of curiosity about what's underneath

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-13 Thread Robert Haas
On Mon, Dec 12, 2016 at 5:45 PM, Tom Lane wrote: > One way that we could make things better is to rely on the knowledge > that EPQ isn't asked to evaluate joins for more than one row per input > relation, and therefore using merge or hash join technology is really > overkill.

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-13 Thread Ashutosh Bapat
On Tue, Dec 13, 2016 at 4:15 AM, Tom Lane wrote: > Jeff Janes writes: >> I have a test case where I made the fdw connect back to itself, and >> stripped out all the objects that I could and still reproduce the case. It >> is large, 21MB compressed,

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-12 Thread Tom Lane
Jeff Janes writes: > I have a test case where I made the fdw connect back to itself, and > stripped out all the objects that I could and still reproduce the case. It > is large, 21MB compressed, 163MB uncompressed, so I am linking it here: >

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-10 Thread Jeff Janes
On Thu, Dec 8, 2016 at 10:28 AM, Tom Lane wrote: > Robert Haas writes: > > Maybe it would help for Jeff to use elog_node_display() to the nodes > > that are causing the problem - e.g. outerpathkeys and innerpathkeys > > and

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-08 Thread Tom Lane
Robert Haas writes: > Maybe it would help for Jeff to use elog_node_display() to the nodes > that are causing the problem - e.g. outerpathkeys and innerpathkeys > and best_path->path_mergeclauses, or just best_path - at the point > where the error is thrown. That might give

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-08 Thread Robert Haas
On Thu, Dec 8, 2016 at 12:50 PM, Tom Lane wrote: > Jeff Janes writes: >> I have a DML statement which triggers the error: >> ERROR: XX000: outer pathkeys do not match mergeclauses >> LOCATION: create_mergejoin_plan, createplan.c:3722 > > Hmm. > >> Any

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-08 Thread Tom Lane
Jeff Janes writes: > I have a DML statement which triggers the error: > ERROR: XX000: outer pathkeys do not match mergeclauses > LOCATION: create_mergejoin_plan, createplan.c:3722 Hmm. > Any tips on investigating this further in situ? Or is the best option just > to