2011/6/30 Alvaro Herrera <[email protected]>:
> 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 ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
