Re: review: FDW API

On 08.02.2011 13:07, Shigeru HANADA wrote:
>
> On Mon, 07 Feb 2011 09:37:37 +0100
> Heikki Linnakangas<heikki.linnakangas@enterprisedb.com>  wrote:
>> In get_relation_info(), you do the catalog lookup anyway and have the
>> Relation object at hand. Add a flag to RelOptInfo indicating if it's a
>> foreign table or not, and set that in get_relation_info().
>
> Thanks a lot.
>
> Attached is a revised version of foreign_scan patch.  This still
> requires fdw_handler patch which was attached to the orginal post.
>
> Avoid_catalog_lookup.patch is attached for review purpose.
> This patch includes changes for this fix.

Thanks.

I spent some more time reviewing this, and working on the PostgreSQL FDW 
in tandem. Here's an updated API patch, with a bunch of cosmetic 
changes, and a bug fix for FOR SHARE/UPDATE. FOR SHARE/UPDATE on a 
foreign table should behave the same as on other relations that can't be 
locked, like a function scan. If you explicitly specify such a relation 
with "FOR UPDATE OF foo", you should get an error, and if you just have 
an unspecified "FOR UPDATE", it should be silently ignored for foreign 
tables.

Doing that requires that we can distinguish foreign tables from other 
relations in transformLockingClause(), but we don't have a RelOptInfo at 
that stage yet. I just used get_rel_relkind() there (and elsewhere 
instead of the IsForeignTable() function), but I've got a nagging 
feeling that sooner or later we'll have to bite the bullet and add that 
field to RangeTblEntry, or introduce a whole new rtekind for foreign tables.

As for the PostgreSQL FDW, I'm trying to make it do two basic tricks, to 
validate the API:
1. Only fetch those columns that are actually needed by the query. This 
involves examining the baserel->reltargetlist, also paying attention to 
whole-row Vars.
2. Push down simple quals, like "column = 'foo'". To do that, I'm trying 
to use the deparsing code from ruleutils.c.

That's pretty much what Shigeru's original postgresql_fdw patch also 
did, but I've changed the implementation quite heavily to make it work 
with the new API. That said, it's still a mess, but I think it validates 
that the API is usable. We could offer a lot more help for FDW authors 
to make those things easier, but I think this is acceptable for now.

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