Re: postgres_fdw bug in 9.6

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 [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