Re: [v9.3] writable foreign tables

Kohei KaiGai wrote:
> The attached patch is just a refreshed version for clean applying to
> the latest tree.
>
> As previous version doing, it makes pseudo enhancement on file_fdw
> to print something about the supplied tuple on INSERT, UPDATE and
> DELETE statement.

Basics:
-------

The patch applies cleanly, compiles without warnings and passes
regression tests.

I think that the functionality is highly desirable; judging from
the number of talks at pgConf EU about SQL/MED this is a hot
topic, and further development is welcome.

Testing the functionality:
--------------------------

I ran a few queries with the file_fdw and found this:

$ cat flatfile
1,Laurenz,1968-10-20
2,Renée,1975-09-03
3,Caroline,2009-01-26
4,Ray,2011-03-09
5,Stephan,2011-03-09

CREATE SERVER file FOREIGN DATA WRAPPER file_fdw;

CREATE FOREIGN TABLE flat(
id integer not null,
name varchar(20) not null,
birthday date not null
) SERVER file
OPTIONS (filename 'flatfile', format 'csv');

UPDATE flat SET name='' FROM flat f WHERE f.id = flat.id and f.name like '%e';
ERROR: bitmapset is empty

About the code:
---------------

I am not so happy with GetForeignRelInfo:
- The name seems ill-chosen from the FDW API side.
I guess that you chose the name because the function
is called from get_relation_info, but I think the name
should be more descriptive for the FDW API.
Something like PlanForeignRelRowid.

- I guess that every FDW that only needs "rowid" will
do exactly the same as your fileGetForeignRelInfo.
Why can't that be done in core?
The function could pass an AttrNumber for the rowid
to the FDW, and will receive a boolean return code
depending on whether the FDW plans to use rowid or not.
That would be more convenient for FDW authors.

- I guess the order is dictated by planner steps, but
it would be "nice to have" if GetForeignRelInfo were
not the first function to be called during planning.
That would make it easier for a FDW to support both
9.2 and 9.3 (fewer #ifdefs), because the FDW plan state
will probably have to be created in the first API
function.

I also wonder why you changed the signature of functions in
trigger.c. It changes an exposed API unnecessarily, unless
you plan to have trigger support for foreign tables.
That is currently not supported, and I think that such
changes should be part of the present patch.

Other than that, I like the patch.
I cannot give an educated opinion if the changes to planner
and executor are well done or not.

Other comments:
---------------

I hope I find time to implement the API in oracle_fdw to
be able to test it more thoroughly.

There is an interdependence with the "FDW for PostgreSQL" patch
in the commitfest. If both these patches get committed, the
FDW should be extended to support the new API. If nothing else,
this would greatly improve the ability to test the new API
and find out if it is well designed.

> Here is one other idea. My GPU acceleration module (PG-Strom)
> implements column-oriented data store underlying foreign table.
> It might make sense to cut out this portion for proof-of-concept of
> writable foreign tables.
>
> Any ideas?

The best would be to have a patch on top of the PostgreSQL FDW
to be able to test. It need not be perfect.

Yours,
Laurenz Albe