Some random comments on the patch:
ReleaseConnection is a very generic name for a global function, would be
good to prefix it with "pgsqlfdw" or something. Same with any other
globally visible functions.
Please use the built-in contain_mutable_functions(Node *) instead of
custom is_immutable_func(). Or at least func_volatile(Oid)
Is it really a good idea to allow LOCK TABLE on foreign tables in its
current form? It only locks the local foreign table object, not the
table in the remote server.
Sorry if this was fiercely discussed already, but I don't think the file
FDW belongs in core. I'd rather see it as a contrib module
I would've expected the contrib install script to create the foreign
data wrapper for me. While you can specify options to a foreign data
wrapper, the CREATE FOREIGN DATA WRAPPER seems similar to CREATE
LANGUAGE, ie. something that happens when the foreign data wrapper
library is installed.
How do you specify a foreign table that has a different name in the
remote server? For example, if I wanted to create a foreign table called
"foo", that fetched rows from a remote table called "bar"?
I would really like to see the SQL query that's shipped to the remote
host in EXPLAIN. That's essential information for analyzing a query that
involves a foreign table.
What about transactions? Does the SQL/MED standard have something to say
about that?
In general, I'm surprised that there's no hook at all into the planning
phase. You have this TODO comment postgresql_fdw:
> /*
> * TODO: omit (deparse to "NULL") columns which are not used in the
> * original SQL.
> *
> * We must parse nodes parents of this ForeignScan node to determine unused
> * columns because some columns may be used only in parent Sort/Agg/Limit
> * nodes.
> */
Parsing the parents of the ForeignScan node seems like a backwards way
of solving the problem. The planner should tell the FDW what columns it
needs. And there should be some way for the FDW to tell the planner
which quals it can handle, so that the executor doesn't need to recheck
them.
You could make the planner interface infinitely complicated, but that's
no excuse for doing nothing at all. The interface needs some thought...
-- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com