On Tue, Mar 7, 2017 at 9:33 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
> On 2017/03/06 21:22, Ashutosh Bapat wrote:
>>
>> On Mon, Mar 6, 2017 at 1:29 PM, David Rowley
>> <david.rowley@2ndquadrant.com> wrote:
>>>
>>> On 6 March 2017 at 18:51, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
>>> wrote:
>>>>
>>>> On 2017/03/06 11:05, David Rowley wrote:
>>>
>>> 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.
>
>
> Thanks for the comments! Actually, those options including
> use_remote_estimate are set later in foreign_join_ok, so
> estimate_path_cost_size would work well, for example.
>
>>> 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.
>
>
> I agree with you on that point.
>
>>> It seems like a much better idea to keep the server option processing
>>> in one location, which is what I did.
>
>
> Seems like a better idea.
>
>> 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.
>
>
> I looked over the patch quickly. I think it's a better idea that to gather
> various option processing in foreign_join_ok (or foreign_grouping_ok) in one
> place. However, I'm wondering we really need to introduce
> apply_table_options and apply_server_options. ISTM that those option
> processing in postgresGetForeignRelSize is gathered in one place well
> already.
I kept that as is since I liked the way it's modularized now.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
