Re: [HACKERS] postgres_fdw bug in 9.6 - Mailing list pgsql-hackers

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 <robertmhaas@gmail.com>
>> wrote:
>>> On Wed, Jan 18, 2017 at 10:26 PM, Ashutosh Bapat
>>> <ashutosh.bapat@enterprisedb.com> 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.
> 
>>>> 2. There are many cases where the new function would return no local
>>>> path and hence postgres_fdw doesn't push down the join [1]. This will
>>>> be performance regression compared to 9.6.
> 
>>> Some, or many?  How many?
> 
>> AFAIU, the problem esp. with parameterized paths is this: If rel1 is
>> required to be parameterized by rel2 (because of lateral references?),
>> a nested loop join with rel2 as outer relation and rel1 as inner
>> relation is possible. postgres_fdw and hence this function, however
>> doesn't consider all the possible join combinations and thus when this
>> function is presented with rel1 as outer and rel2 as inner would
>> refuse to create a path. More generally, while creating local paths,
>> we try many combinations of participating relations, some of which do
>> not produce local paths and some of them do (AFAIK, it happens in case
>> of lateral references, but there might be other cases). postgres_fdw,
>> however considers only a single combination, which may or may not have
>> produced local path. In such a case, postgres_fdw would create a
>> foreign join path but won't get a local path and thus bail out.
> 
> I had second thoughts; one idea how to build parameterized paths to
> avoid this issue is: as in postgresGetForeignPaths, to (1) identify
> which outer relations could supply additional safe-to-send-to-remote
> join clauses, and (2) build a parameterized path and its alternative
> local-join path for each such outer relation.  In #1, we could look at
> the join relation's ppilist to identify interesting outer relations.  In
> #2, the local-join path corresponding to each such outer relation could
> be built from the cheapest-total paths for the outer and inner
> relations, using CreateLocalJoinPath, so that the result path has the
> outer relation as its required_outer.
> 
>>> I'm a bit sketchy about this kind of thing, though:
>>>
>>> !                 if (required_outer)
>>>                   {
>>> !                     bms_free(required_outer);
>>> !                     return NULL;
>>>                   }
>>>
>>> I don't understand what would stop that from making this fail to
>>> generate parameterized paths in some cases in which it would be legal
>>> to do so, and the comment is very brief.
> 
>> I am not so much worried about this though :).
>> GetExistingLocalJoinPath() also does not handle that case. The new
>> function is not making it worse in this case.
>> 731         /* Skip parameterised paths. */
>> 732         if (path->param_info != NULL)
>> 733             continue;
> 
> One idea to remove such extra checks is to pass the required_outer to
> CreateLocalJoinPath like the attached.  As described above, the caller
> would have that set before calling that function, so we wouldn't need to
> calculate that set in that function.
> 
> 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.
> * Fixed a bug that I missed the RIGHT JOIN case in CreateLocalJoinPath.

This patch does not apply cleanly at cccbdde:

$ patch -p1 < ../other/epqpath-for-foreignjoin-5.patch
patching file contrib/postgres_fdw/expected/postgres_fdw.out
Hunk #6 succeeded at 4134 (offset 81 lines).
Hunk #7 succeeded at 4275 (offset 81 lines).
patching file contrib/postgres_fdw/postgres_fdw.c
Hunk #1 succeeded at 4356 (offset 3 lines).
Hunk #2 succeeded at 4386 (offset 3 lines).
patching file contrib/postgres_fdw/sql/postgres_fdw.sql
patching file doc/src/sgml/fdwhandler.sgml
patching file src/backend/foreign/foreign.c
Hunk #2 FAILED at 696.
1 out of 2 hunks FAILED -- saving rejects to file
src/backend/foreign/foreign.c.rej
patching file src/backend/optimizer/path/joinpath.c
Hunk #1 FAILED at 25.
Hunk #2 succeeded at 109 (offset 27 lines).
Hunk #3 succeeded at 134 (offset 27 lines).
Hunk #4 succeeded at 197 (offset 27 lines).
Hunk #5 succeeded at 208 (offset 27 lines).
Hunk #6 succeeded at 225 (offset 27 lines).
Hunk #7 succeeded at 745 (offset 113 lines).
Hunk #8 FAILED at 894.
Hunk #9 succeeded at 1558 (offset 267 lines).
Hunk #10 succeeded at 1609 (offset 268 lines).
2 out of 10 hunks FAILED -- saving rejects to file
src/backend/optimizer/path/joinpath.c.rej
patching file src/include/foreign/fdwapi.h
Hunk #1 succeeded at 237 (offset 2 lines).
patching file src/include/nodes/relation.h
Hunk #1 succeeded at 914 (offset 10 lines).
Hunk #2 succeeded at 2057 (offset 47 lines).

Marked "Waiting on Author".

-- 
-David
david@pgmasters.net