On Tue, Mar 7, 2017 at 9:33 AM, Etsuro Fujita <fujita.ets...@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.row...@2ndquadrant.com> wrote: >>> >>> On 6 March 2017 at 18:51, Etsuro Fujita <fujita.ets...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
