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(dot)linnakangas(at)enterprisedb(dot)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