Re: [v9.3] writable foreign tables

Awesome.

I would love to implement this API in JDBC_FDW.

Atri

Sent from my iPad

On 16-Nov-2012, at 20:20, "Albe Laurenz" <laurenz(dot)albe(at)wien(dot)gv(dot)at> wrote:

> 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