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