Shigeru Hanada <shigeru.hanada@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