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

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.

Best regards,
Etsuro Fujita

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