Thread: Bug in SQL/MED?
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? Yours, Laurenz Albe
Attachment
I 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. [...] > Example: [...] The example is misleading. Here a better one: CREATE SERVER myoradb FOREIGN DATA WRAPPER oracle_fdw OPTIONS (foo 'bar'); ERROR: invalid option "foo" HINT: Valid options in this context are: dbserver, user, password but: CREATE SERVER myoradb FOREIGN DATA WRAPPER oracle_fdw; gives no error. Yours, Laurenz Albe
(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
Attachment
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. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
2011/6/30 Alvaro Herrera <alvherre@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
Attachment
2011/7/1 Shigeru Hanada <shigeru.hanada@gmail.com>: > 2011/6/30 Alvaro Herrera <alvherre@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 Do we want to apply this to 9.1 before beta3? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas wrote: >>>> 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. > Do we want to apply this to 9.1 before beta3? If possible, yes please. Yours, Laurenz Albe
Robert Haas <robertmhaas@gmail.com> writes: > 2011/7/1 Shigeru Hanada <shigeru.hanada@gmail.com>: >> 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 > Do we want to apply this to 9.1 before beta3? The original patch in this thread seems pretty bogus to me, because it changes only one place of many in foreigncmds.c where PointerGetDatum(NULL) is used as the representation of an empty options list. If we're going to go over to the rule that pg_foreign_data_wrapper.fdwoptions can't be NULL, which is what this is effectively proposing, we need much more extensive changes than this. Also, since foreigncmds.c is sharing code with reloptions.c, wherein the PointerGetDatum(NULL) convention is also used, I'm concerned that we're going to have more bugs added than removed by changing the rule for just pg_foreign_data_wrapper.fdwoptions. I think it might be better to keep the convention that an empty options list is represented by null, and to say that if a validator wants to be called on such a list, it had better declare itself non-strict. At least we ought to think about that before redefining the catalog semantics at this late hour. regards, tom lane
I wrote:
> I think it might be better to keep the convention that an empty options
> list is represented by null, and to say that if a validator wants to be
> called on such a list, it had better declare itself non-strict. At
> least we ought to think about that before redefining the catalog
> semantics at this late hour.
Another possibility that just occurred to me is to call the validator
like this:
if (OidIsValid(fdwvalidator)) { Datum valarg = result;
/* pass a null options list as an empty array */ if (DatumGetPointer(valarg) == NULL) valarg =
construct_empty_array(TEXTOID); OidFunctionCall2(fdwvalidator, valarg, ObjectIdGetDatum(catalogId)); }
This would avoid messing with the semantics of empty options lists
throughout foreigncmds.c, and also avoid requiring validators to deal
with null arguments.
regards, tom lane
Shigeru Hanada <shigeru.hanada@gmail.com> writes:
> 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
Applied with small adjustments.
regards, tom lane
I wrote:
> Another possibility that just occurred to me is to call the validator
> like this:
>
> if (OidIsValid(fdwvalidator))
> {
> Datum valarg = result;
>
> /* pass a null options list as an empty array */
> if (DatumGetPointer(valarg) == NULL)
> valarg = construct_empty_array(TEXTOID);
> OidFunctionCall2(fdwvalidator, valarg, ObjectIdGetDatum(catalogId));
> }
> This would avoid messing with the semantics of empty options lists
> throughout foreigncmds.c, and also avoid requiring validators to deal
> with null arguments.
Not hearing any objections, I've fixed it that way.
regards, tom lane