2011/6/30 Alvaro Herrera <alvhe...@commandprompt.com>: > Excerpts from 花田 茂's message of jue jun 30 06:00:23 -0400 2011: > >> 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. > > I think it'd be good to keep the error check in fileGetOptions() just to > ensure that we don't crash in case a catalog is messed up with, though > I'd degrade it to elog(ERROR) from ereport.
Thanks for the comments. Please find attached a patch. Now file_fdw validates filename in: * file_fdw_validator(), to catch lack of required option at DDL * fileGetOptions(), to avoid crash caused by corrupted catalog I used ereport for the former check, because maybe such error usually happens and is visible to users. This criteria was taken from the document "Reporting Errors Within the Server". http://developer.postgresql.org/pgdocs/postgres/error-message-reporting.html Regards, -- Shigeru Hanada
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c index 466c015..bf80568 100644 *** a/contrib/file_fdw/file_fdw.c --- b/contrib/file_fdw/file_fdw.c *************** file_fdw_validator(PG_FUNCTION_ARGS) *** 215,220 **** --- 215,231 ---- */ ProcessCopyOptions(NULL, true, other_options); + /* + * Filename is required for file_fdw foreign tables. We must validate it + * separately because ProcessCopyOptions() doesn't validate it. + * We don't care whether the value is empty or not, because COPY doesn't + * care that. + */ + if (catalog == ForeignTableRelationId && filename == NULL) + ereport(ERROR, + (errcode(ERRCODE_FDW_UNABLE_TO_CREATE_REPLY), + 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; } --- 297,311 ---- } prev = lc; } + + /* + * The requirement of filename option must have been checked by + * file_fdw_validator at every DDL. We check it here again just in case + * to avoid crash caused by unexpected catalog corruption. + */ if (*filename == NULL) ! elog(ERROR, "filename is required for file_fdw foreign tables"); ! *other_options = options; } 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
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers