|From:||Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>|
|To:||Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>|
|Cc:||Jeff Janes <jeff(dot)janes(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: postgres_fdw bug in 9.6|
|Views:||Raw Message | Whole Thread | Download mbox|
On 2017/01/20 14:24, Ashutosh Bapat wrote:
> On Thu, Jan 19, 2017 at 10:38 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Wed, Jan 18, 2017 at 10:26 PM, Ashutosh Bapat
>> <ashutosh(dot)bapat(at)enterprisedb(dot)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 . 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.
* 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.
|Next Message||Etsuro Fujita||2017-01-23 10:11:46||Odd plan shown in src/backend/optimizer/README|
|Previous Message||Andrew Borodin||2017-01-23 09:55:55||Re: Review: GIN non-intrusive vacuum of posting tree|