Re: review: FDW API

On Fri, 21 Jan 2011 18:28:19 +0200
Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> On 18.01.2011 17:26, Shigeru HANADA wrote:
> > 3) 20110118-fdw_handler.patch - This patch adds support for HANDLER
> > option to FOREIGN DATA WRAPPER object.
>
> Some quick comments on that:

Thanks for the comments.
I'll post revised version of patches soon.

> * The elogs in parse_func_options() should be ereports.
>
> * pg_dump should check the version number and only try to select
> fdwhandler column if >= 9.1. See the other functions there for example
> of that.

Fixed.

> * dumpForeignDataWrapper() in pg_dump checks if fdwhandler field is "-".
> I don't think we use that as magic value there, do we? Same with validator.

That magic value, "-", is used as "no-function-was-set" in
dumpForeignDataWrapper() and dumpForeignServer(), and I followed them.
Agreed that magic value should be removed, but it would be a refactoring
issue about pg_dump.

> * Please check that the HANDLER and VALIDATOR options that pg_dump
> creates properly quoted.

I checked quoting for HANDLER and VALIDATOR with file_fdw functions,
and it seems works fine. The pg_dump generats:

------------
CREATE FOREIGN DATA WRAPPER dummy_fdw VALIDATOR public."File_Fdw_Validator"
HANDLER public."FILE_FDW_HANDLER";
------------

from these DDLs:

------------
CREATE OR REPLACE FUNCTION "File_Fdw_Validator" (text[], oid)
RETURNS bool
AS '$libdir/file_fdw','file_fdw_validator'
LANGUAGE C STRICT;

CREATE OR REPLACE FUNCTION "FILE_FDW_HANDLER" ()
RETURNS fdw_handler
AS '$libdir/file_fdw','file_fdw_handler'
LANGUAGE C STRICT;

CREATE FOREIGN DATA WRAPPER dummy_fdw
VALIDATOR "File_Fdw_Validator" HANDLER "FILE_FDW_HANDLER";
------------

Regard,
--
Shigeru Hanada