Re: pgsql_fdw, FDW for PostgreSQL server - Mailing list pgsql-hackers
From | Shigeru Hanada |
---|---|
Subject | Re: pgsql_fdw, FDW for PostgreSQL server |
Date | |
Msg-id | 4F5758FF.4080701@gmail.com Whole thread Raw |
In response to | Re: pgsql_fdw, FDW for PostgreSQL server (Tom Lane <tgl@sss.pgh.pa.us>) |
Responses |
Re: pgsql_fdw, FDW for PostgreSQL server
(Tom Lane <tgl@sss.pgh.pa.us>)
Re: pgsql_fdw, FDW for PostgreSQL server (Shigeru Hanada <shigeru.hanada@gmail.com>) |
List | pgsql-hackers |
(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
Attachment
pgsql-hackers by date: