Re: postgres_fdw bug in 9.6

On Fri, Jan 13, 2017 at 3:22 AM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
wrote:

> On 2017/01/13 0:43, Robert Haas wrote:
>
>> On Wed, Jan 11, 2017 at 2:45 AM, Etsuro Fujita
>> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> 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 traverse through the
>>> possibly-lengthy pathlist to find a local-join path, as mentioned in [3].
>>>
>>
> I'm not sure that's really going to be a problem. The number of
>> possible parameterizations that need to be considered isn't usually
>> very big. I bet the path list will have ten or a few tens of entries
>> in it, not a hundred or a thousand. Traversing it isn't that
>> expensive.
>>
>> That having been said, I haven't read the patches, so I'm not really
>> up to speed on the bigger issues here. But surely it's more important
>> to get the overall design right than to worry about the cost of
>> walking the pathlist or worrying about the cost of an extra function
>> call (?!).
>>
>
> 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 to have
> the same parameterization as the foreign-join path, but there might not be
> any existing paths with the same parameterization in the path list. You
> might think we could get the fdw_outerpath by getting an existing path with
> no parameterization as in GetExistingLocalJoinPath and then modifying the
> path's param_info to match the parameterization of the foreign-join path.
> I don't know that really works, but that might be inefficient.
>
> What I have in mind to support foreign-join paths with parameterization
> for postgres_fdw like this: (1) generate parameterized paths from any
> joinable combination of the outer/inner cheapest-parameterized paths that
> have pushed down the outer/inner relation to the remote server, in a
> similar way as postgresGetForeignJoinPaths creates unparameterized paths,
> and (2) create fdw_outerpath for each parameterized path from the
> outer/inner paths used to generate the parameterized path, by
> create_nestloop_path (or, create_hashjoin_path or create_mergejoin_path if
> full join), so that the resulting fdw_outerpath has the same
> parameterization as the paramterized path. This would probably work and
> might be more efficient. And the patch I proposed would be easily extended
> to this, by replacing the outer/inner cheapest-total paths with the
> outer/inner cheapest-parameterized paths. Attached is the latest version
> of the patch.
>

I'm afraid this discussion and the C code here are over my head. But I've
tested this patch against 9.6.stable-2d443ae1b0121e15265864d2b21 and
10.dev-0563a3a8b59150bf3cc8, and in both cases it fixes the original
problem.

I do get a compiler warning:

foreign.c: In function 'CreateLocalJoinPath':
foreign.c:832: warning: implicit declaration of function
'pathkeys_contained_in'

Cheers,

Jeff