(2011/06/29 21:23), Albe Laurenz wrote:
> If you invoke any of the SQL/MED CREATE or ALTER commands,
> the validator function is only called if an option list was given.
>
> That means that you cannot enforce required options at object creation
> time, because the validator function is not always called.
> I consider that unexpected an undesirable behaviour.
>
> Example:
> CREATE EXTENSION file_fdw;
> CREATE FOREIGN DATA WRAPPER file HANDLER file_fdw_handler VALIDATOR
> file_fdw_validator;
> CREATE SERVER file FOREIGN DATA WRAPPER file;
> CREATE FOREIGN TABLE flat (id integer) SERVER file OPTIONS (format
> 'csv');
> SELECT * FROM flat;
> ERROR: filename is required for file_fdw foreign tables
>
> Now file_fdw does not try to enforce the "filename" option, but it
> would be nice to be able to do so.
>
> The attached patch would change the behaviour so that the validator
> function
> is always called.
>
>
> If that change meets with approval, should file_fdw be changed so that
> it
> requires "filename" at table creation time?
I think this proposal is reasonable. IMHO this fix is enough trivial to
be merged into 9.1 release.
I attached a patch which fixes file_fdw to check required option
(filename) in its validator function. I think that such requirement
should be checked again in PlanForeignScan(), as it had been so far.
Note that this patch requires fdw.patch has been applied.
With this patch, file_fdw rejects commands which eliminate filename
option as result. Please see attached sample.txt for detail.
BTW, I noticed that current document says just "the validator function
is called for CREATE FDW/SERVER/FOREIGN TABLE", and doesn't mention
ALTER command or USER MAPPING. I'll post another patch for this issue
later.
Regards,
--
Shigeru Hanada
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index 466c015..57e522f 100644
*** a/contrib/file_fdw/file_fdw.c
--- b/contrib/file_fdw/file_fdw.c
*************** file_fdw_validator(PG_FUNCTION_ARGS)
*** 215,220 ****
--- 215,230 ----
*/
ProcessCopyOptions(NULL, true, other_options);
+ /*
+ * filename is a required option. Validity of other options including
+ * relative ones have been checked in ProcessCopyOptions().
+ * Note: We don't care its value even though it might be empty, because
+ * COPY comand doesn't.
+ */
+ if (catalog == ForeignTableRelationId && filename == NULL)
+ ereport(ERROR,
+ (errmsg("filename is required for file_fdw
foreign tables")));
+
PG_RETURN_VOID();
}
*************** fileGetOptions(Oid foreigntableid,
*** 286,295 ****
}
prev = lc;
}
- if (*filename == NULL)
- ereport(ERROR,
- (errcode(ERRCODE_FDW_UNABLE_TO_CREATE_REPLY),
- errmsg("filename is required for file_fdw
foreign tables")));
*other_options = options;
}
--- 296,301 ----
*************** filePlanForeignScan(Oid foreigntableid,
*** 308,313 ****
--- 314,323 ----
/* Fetch options --- we only need filename at this point */
fileGetOptions(foreigntableid, &filename, &options);
+ if (filename == NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_FDW_UNABLE_TO_CREATE_REPLY),
+ errmsg("filename is required for file_fdw
foreign tables")));
/* Construct FdwPlan with cost estimates */
fdwplan = makeNode(FdwPlan);
diff --git a/contrib/file_fdw/input/file_fdw.source
b/contrib/file_fdw/input/file_fdw.source
index 9ff7235..8d6dfa3 100644
*** a/contrib/file_fdw/input/file_fdw.source
--- b/contrib/file_fdw/input/file_fdw.source
*************** CREATE FOREIGN TABLE tbl () SERVER file_
*** 59,64 ****
--- 59,65 ----
'); -- ERROR
CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'csv', null '
'); -- ERROR
+ CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'csv');
-- ERROR
CREATE FOREIGN TABLE agg_text (
a int2,
diff --git a/contrib/file_fdw/output/file_fdw.source
b/contrib/file_fdw/output/file_fdw.source
index 2ba36c9..6cc6746 100644
*** a/contrib/file_fdw/output/file_fdw.source
--- b/contrib/file_fdw/output/file_fdw.source
*************** ERROR: COPY delimiter cannot be newline
*** 75,80 ****
--- 75,82 ----
CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'csv', null '
'); -- ERROR
ERROR: COPY null representation cannot use newline or carriage return
+ CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'csv');
-- ERROR
+ ERROR: filename is required for file_fdw foreign tables
CREATE FOREIGN TABLE agg_text (
a int2,
b float4
postgres=# CREATE FOREIGN TABLE foo () SERVER file;
ERROR: filename is required for file_fdw foreign tables
postgres=# CREATE FOREIGN TABLE foo () SERVER file OPTIONS (format 'csv');
ERROR: filename is required for file_fdw foreign tables
postgres=# CREATE FOREIGN TABLE foo () SERVER file OPTIONS (filename '', format
'csv');
CREATE FOREIGN TABLE
postgres=# ALTER FOREIGN TABLE foo OPTIONS (DROP filename);
ERROR: filename is required for file_fdw foreign tables
postgres=# ALTER FOREIGN TABLE foo OPTIONS (DROP format);
ALTER FOREIGN TABLE
postgres=#
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
