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