Re: Foreign Join pushdowns not working properly for outer joins

On Mon, Mar 6, 2017 at 1:29 PM, David Rowley
<david(dot)rowley(at)2ndquadrant(dot)com> wrote:
> On 6 March 2017 at 18:51, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> On 2017/03/06 11:05, David Rowley wrote:
>>> The attached patch, based on 9.6, fixes the problem by properly
>>> processing the foreign server options in
>>> postgresGetForeignJoinPaths().
>>
>> I think the fix would work well, but another way I think is much simpler and
>> more consistent with the existing code is to (1) move code for getting the
>> server info from the outer's fpinfo before calling is_foreign_expr() in
>> foreign_join_ok() and (2) add code for getting the shippable extensions info
>> from the outer's fpinfo before calling that function, like the attached.
>
> Thanks for looking over my patch.
>
> I looked over yours and was surprised to see:
>
> + /* is_foreign_expr might need server and shippable-extensions info. */
> + fpinfo->server = fpinfo_o->server;
> + fpinfo->shippable_extensions = fpinfo_o->shippable_extensions;
>
> That appears to do nothing for the other server options. For example
> use_remote_estimate, which is used in estimate_path_cost_size(). As of
> now, that's also broken. My patch fixes that problem too, yours
> appears not to.
>
> Even if you expanded the above code to process all server options, if
> someone comes along later and adds a new option, my code will pick it
> up, yours could very easily be forgotten about and be the cause of yet
> more bugs.
>
> It seems like a much better idea to keep the server option processing
> in one location, which is what I did.

I agree with this. However
1. apply_server_options() is parsing the options strings again and
again, which seems wastage of CPU cycles. It should probably pick up
the options from one of the joining relations. Also, the patch calls
GetForeignServer(), which is not needed; in foreign_join_ok(), it will
copy it from the outer relation anyway.
2. Some server options like use_remote_estimate and fetch_size are
overridden by corresponding table level options. For a join relation
the values of these options are derived by some combination of
table-level options.

I think we should write a separate function
apply_fdw_options_for_joinrel() and pass the fpinfos of joinrel, outer
and inner relation. The function will copy the values of server level
options and derive values for table level options. We would add a note
there to keep this function in sync with apply_*_options(). I don't
think there's any better way to keep the options in sync for base
relations and join relations.

Here's the patch attached.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company