Re: review: FDW API

On 21.01.2011 17:57, Robert Haas wrote:
> On Fri, Jan 21, 2011 at 10:17 AM, Heikki Linnakangas
> <heikki.linnakangas@enterprisedb.com>  wrote:
>> On 18.01.2011 17:26, Shigeru HANADA wrote:
>>>
>>> 1) 20110118-no_fdw_perm_check.patch - This patch is not included in
>>> last post.  This had been proposed on 2011-01-05 first, but maybe has
>>> not been reviewd yet.  I re-propose this patch for SQL standard
>>> conformance.  This patch removes permission check that requires USAGE
>>> on the foreign-data wrapper at CREATE FOREIGN TABLE.
>>> Please see original post for details.
>>>
>>> http://archives.postgresql.org/message-id/20110105145206.30FD.6989961C@metrosystems.co.jp
>>
>> Committed this part.
>
> How much review have you done of parts (3) and (4)?  The key issue for
> all of the FDW work in progress seems to be what the handler API is
> going to look like, and so once we get that committed it will unblock
> a lot of other things.

I've gone through the code in a bit more detail now. I did a bunch of 
cosmetic changes along the way, patch attached. I also added a few 
paragraphs in the docs. We need more extensive documentation, but this 
at least marks the places where I think the docs need to go.

Comments:

* How can a FDW fetch only the columns required by the scan? The file 
FDW has to read the whole file anyhow, but it could perhaps skip calling 
the input function for unnecessary columns. But more importantly, with 
something like the postgresql_fdw you don't want to fetch any extra 
columns across the wire. I gather the way to do it is to copy 
RelOptInfo->attr_needed to private storage at plan stage, and fill the 
not-needed attributes with NULLs in Iterate. That gets a bit awkward, 
you need to transform attr_needed to something that can be copied for 
starters. Or is that information somehow available at execution phase 
otherwise?

* I think we need something in RelOptInfo to mark foreign tables. At the 
moment, you need to call IsForeignTable() which does a catalog lookup. 
Maybe a new RTEKind, or a boolean flag.

* Can/should we make ReScan optional? Could the executor just do 
EndScan+BeginScan if there's no ReScan function?

* Is there any point in allowing a FDW without a handler? It's totally 
useless, isn't it? We had the CREATE FOREIGN DATA WRAPPER syntax in 
previous versions, and it allowed it, but it has always been totally 
useless so I don't think we need to worry much about 
backwards-compatibility here.

* Is there any use case for changing the handler or validator function 
of an existign FDW with ALTER? To me it just seems like an unnecessary 
complication.

* IMHO the "FDW-info" should always be displayed, without VERBOSE. In my 
experience with another DBMS that had this feature, the SQL being sent 
to the remote server was almost always the key piece of information that 
I was looking for in the query plans.

* this check in expand_inherited_rtentry seems misplaced:

> 		/*
> 		 * SELECT FOR UPDATE/SHARE is not allowed on foreign tables because
> 		 * they are read-only.
> 		 */
> 		if (newrelation->rd_rel->relkind == RELKIND_FOREIGN_TABLE &&
> 			lockmode != AccessShareLock)
> 			ereport(ERROR,
> 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> 					 errmsg("SELECT FOR UPDATE/SHARE is not allowed with foreign tables")));

I don't understand why we'd need to do that for inherited tables in 
particular. And it's not working for regular non-inherited foreign tables:

postgres=# SELECT * FROM filetbl2 FOR UPDATE;
ERROR:  could not open file "base/11933/16397": No such file or directory

* Need to document how the FDW interacts with transaction 
commit/rollback. In particular, I believe EndScan is never called if the 
transaction is aborted. That needs to be noted explicitly, and need to 
suggest how to clean up any external resources in that case. For 
example, postgresql_fdw will need to close any open cursors or result sets.

In general, I think the design is sound. What we need is more 
documentation. It'd also be nice to see the postgresql_fdw brought back 
to shape so that it works against this latest version of the api patch.

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com