From: David Steele <david@pgmasters.net>
To: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
Cc: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Jeff Janes <jeff.janes@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: postgres_fdw bug in 9.6
Date: Thu, 16 Mar 2017 11:37:03 -0400
Message-ID: <47f35675-e504-033a-5bbe-185b78b47c3b@pgmasters.net> (raw)
In-Reply-To: <0700eb97-d9db-33da-4ba2-e28d2a1631d9@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>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: david@pgmasters.net, fujita.etsuro@lab.ntt.co.jp, ashutosh.bapat@enterprisedb.com, robertmhaas@gmail.com, jeff.janes@gmail.com, tgl@sss.pgh.pa.us
Subject: Re: postgres_fdw bug in 9.6
In-Reply-To: <47f35675-e504-033a-5bbe-185b78b47c3b@pgmasters.net>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox