Re: postgres_fdw bug in 9.6

On 2016/12/28 17:34, Ashutosh Bapat wrote:
> On Wed, Dec 28, 2016 at 1:29 PM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> On 2016/12/28 15:54, Ashutosh Bapat wrote:
>>> On Wed, Dec 28, 2016 at 9:20 AM, Etsuro Fujita
>>> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> 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 operators), we will end up returning NULL. I
>>>>> think we want to create a merge join in that case. I think the order
>>>>> of conditions should be 1. check hashclause_list then create hash join
>>>>> 2. else check if merge allowed, create merge join. It looks like that
>>>>> would cover all the cases, if there aren't any hash clauses, and also
>>>>> no merge clauses, we won't be able to implement a FULL join, so it
>>>>> will get rejected during path creation itself.

>>>> Right, maybe we can do that by doing similar things as in
>>>> match_unsort_outer
>>>> and/or sort_inner_and_outer. But as you mentioned, the case is rare, so
>>>> the
>>>> problem would be whether it's worth complicating the code (and if it's
>>>> worth, whether we should do that at the first version of the function).

>>> All I am requesting is changing the order of conditions. That doesn't
>>> complicate the code.

>> I might have misunderstood your words, but you are saying we should consider
>> mergejoin paths with some mergeclauses in the case where hashclauses is NIL,
>> right? To do so, we would need to consider the sort orders of outer/inner
>> paths, which would make the code complicated.

> 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 path in those cases as well, we have a regression in
> performance.

Ok, will revise, but as I mentioned upthread, I'm not sure it's a good
idea to search the pathlist to get a merge join even in this case. I'd
vote for creating a merge join path from the inner/outer paths in this
case as well. I think that would simplify the code as well.

Best regards,
Etsuro Fujita