Subject: Re: postgres_fdw bug in 9.6
To: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
References: 
 <CAMkU=1y2G8VOVBHv3iXU2TMAj7-RyBFFW1uhkr5sm9LQ2=X35g@mail.gmail.com>
 <CAFjFpRf2hmbuUa5q1Yo3A9jA3L9ezSCA=vN+ZPGWRp9+Za2R5A@mail.gmail.com>
 <98cc218e-a5e1-37ad-ad57-4e52b5c933a2@lab.ntt.co.jp>
 <CAFjFpRek1+1QXBiWNqeWw1cWyXBNH4rSWFuVZ-dHn_cT-ZH+QQ@mail.gmail.com>
 <f721652b-a9af-9a3b-6ba4-8720953eba3c@lab.ntt.co.jp>
 <fa9c8cdb-ee60-36bd-6cda-4172899e79b7@lab.ntt.co.jp>
 <CAFjFpRccHHQbfzbWZfo=SuC8sjcqukMRjNpx7tw7th2v-Ghmjw@mail.gmail.com>
 <ce7d8b72-c286-52d1-4a8a-57dba26c2494@lab.ntt.co.jp>
 <CA+Tgmob-VxZyzUndew2bbzi7_C_yAWk_5qCitQhwjRuwMnSEqw@mail.gmail.com>
 <e18b9bf5-1557-cb9c-001e-0861a1d7dfce@lab.ntt.co.jp>
 <CA+TgmoZny9Bte4_rYi2S+vGh1TV69NreAHzD6v96tUxryyDn2w@mail.gmail.com>
 <CAFjFpRd+g1fw5EVR7e=BcJP8adY7WjDBq=SZgo6GH4qyaPRq1A@mail.gmail.com>
 <CA+TgmoaZDP0ysRqwmP=W4cQr57zN6deNcp0gj_xZw8Czwqf13Q@mail.gmail.com>
 <CAFjFpRfSbaaWyJxaGs5Au1gy3puQ13mTGTGF7Az2FAhi+1QQEQ@mail.gmail.com>
 <0700eb97-d9db-33da-4ba2-e28d2a1631d9@lab.ntt.co.jp>
Cc: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>,
 Robert Haas <robertmhaas@gmail.com>, Jeff Janes <jeff.janes@gmail.com>,
 Tom Lane <tgl@sss.pgh.pa.us>, pgsql-hackers <pgsql-hackers@postgresql.org>
From: David Steele <david@pgmasters.net>
Message-ID: <47f35675-e504-033a-5bbe-185b78b47c3b@pgmasters.net>
Date: Thu, 16 Mar 2017 11:37:03 -0400
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:45.0)
 Gecko/20100101 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <0700eb97-d9db-33da-4ba2-e28d2a1631d9@lab.ntt.co.jp>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers