Re: pgsql_fdw, FDW for PostgreSQL server

(2012/03/07 9:01), Tom Lane wrote:
> 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?).

Thanks for the review. Yes, v5 is the last version of fdw_helper patch.

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

In addition, request for fetch_count option value via
GetFdwOptionValue() uselessly runs GetUserMapping() and
GetForeignDataWrapper() too, they are obvious waste of clocks and memory.

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

Yes, I added GetFdwOptionValue() to provide an easy way to obtain the
value of a particular FDW option which might be stored in multiple
levels of foreign-objects hierarchy without looping per object. I used
it in pgsql_fdw to get value of fetch_count option, which can be stored
in server and/or foreign table, but it seems the only one use case now.

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

Agreed. Attached fdw_helper patch doesn't contain GetFdwOptionValue()
any more, and pgsql_fdw patch accesses only necessary catalogs.

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

strVal() used in pgsql_fdw were replaced with defGetString(). It seems
to me that it's enough.

Regards,
--
Shigeru Hanada