Shigeru Hanada <shigeru.han...@gmail.com> writes: > Attached patches also contains changes to catch up to the redesign of > PlanForeignScan.
I started to look at this patch, which soon led me back to the prerequisite patch fdw_helper_v5.patch (that is the last version you posted of that one, right?). I can see the value of GetForeignColumnOptions, but ISTM that GetFdwOptionValue is poorly designed and will accomplish little except to encourage inefficient searching. The original version was even worse, but even in the current version there is no way to avoid a useless scan of table-level options when you are looking for a column-level option. Also, it's inefficient when looking for several option values, as in this extract from pgsql_fdw_v13.patch, + nspname = GetFdwOptionValue(InvalidOid, InvalidOid, relid, + InvalidAttrNumber, "nspname"); + if (nspname == NULL) + nspname = get_namespace_name(get_rel_namespace(relid)); + q_nspname = quote_identifier(nspname); + + relname = GetFdwOptionValue(InvalidOid, InvalidOid, relid, + InvalidAttrNumber, "relname"); + if (relname == NULL) + relname = get_rel_name(relid); + q_relname = quote_identifier(relname); where we are going to uselessly run GetForeignTable twice. If we had a lot of options that could usefully be specified at multiple levels of the foreign-objects hierarchy, it might be appropriate to have a search function defined like this; but the existing samples of FDWs don't seem to support the idea that that's going to be common. It looks to me like the vast majority of options make sense at exactly one level. So I'm thinking we should forget GetFdwOptionValue and just expect the callers to search the option lists for the appropriate object(s). It might be worth providing get_options_value() as an exported function, though surely there's not that much to it. Another issue that get_options_value ignores defGetString() which is what it really ought to be using, instead of assuming strVal() is appropriate. ISTM that it would also encourage people to take shortcuts where they should be using functions like defGetBoolean() etc. Not quite sure what we should do about that; maybe we need to provide several variants of the function that are appropriate for different option datatypes. regards, tom lane -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers