Re: review: FDW API

On 11.02.2011 22:50, Heikki Linnakangas wrote:
> 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.

Another version, rebased against master branch and with a bunch of small 
cosmetic fixes.

I guess this is as good as this is going to get for 9.1. I'm sure we'll 
have to revisit the API in the future as people write more FDWs and we 
know their needs better, but there's one detail I'd like to have a 
second opinion on before I commit this:

As the patch stands, we have to do get_rel_relkind() in a couple of 
places in parse analysis and the planner to distinguish a foreign table 
from a regular one. As the patch stands, there's nothing in 
RangeTblEntry (which is what we have in transformLockingClause) or 
RelOptInfo (for set_plain_rel_pathlist) to directly distinguish them.

I'm actually surprised we don't need to distinguish them in more places, 
but nevertheless it feels like we should have that info available more 
conveniently, and without requiring a catalog lookup like 
get_rel_relkind() does. At first I thought we should add a field to 
RelOptInfo, but that doesn't help transformLockingClause. Adding the 
field to RangeTblEntry seems quite invasive, and it doesn't feel like 
the right place to cache that kind of information anyway. Thoughts on that?

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