Bug in SQL/MED?

Started by Albe Laurenzover 14 years ago11 messages

laurenz.albe@wien.gv.at

over 14 years ago

1 attachment(s)

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

Attachments:

fdw.patchapplication/octet-stream; name=fdw.patchDownload

diff --git a/src/backend/commands/foreigncmds.c b/src/backend/commands/foreigncmds.c
new file mode 100644
index 21d52e0..62afbe4
*** a/src/backend/commands/foreigncmds.c
--- b/src/backend/commands/foreigncmds.c
***************
*** 40,46 ****
  /*
   * Convert a DefElem list to the text array format that is used in
   * pg_foreign_data_wrapper, pg_foreign_server, and pg_user_mapping.
!  * Returns the array in the form of a Datum, or PointerGetDatum(NULL)
   * if the list is empty.
   *
   * Note: The array is usually stored to database without further
--- 40,46 ----
  /*
   * Convert a DefElem list to the text array format that is used in
   * pg_foreign_data_wrapper, pg_foreign_server, and pg_user_mapping.
!  * Returns the array in the form of a Datum, an empty array
   * if the list is empty.
   *
   * Note: The array is usually stored to database without further
*************** optionListToArray(List *options)
*** 74,80 ****
  	if (astate)
  		return makeArrayResult(astate, CurrentMemoryContext);
  
! 	return PointerGetDatum(NULL);
  }
  
  
--- 74,80 ----
  	if (astate)
  		return makeArrayResult(astate, CurrentMemoryContext);
  
! 	return construct_empty_array(TEXTOID);
  }
  
  
*************** transformGenericOptions(Oid catalogId,
*** 165,171 ****
  
  	result = optionListToArray(resultOptions);
  
! 	if (OidIsValid(fdwvalidator) && DatumGetPointer(result) != NULL)
  		OidFunctionCall2(fdwvalidator, result, ObjectIdGetDatum(catalogId));
  
  	return result;
--- 165,171 ----
  
  	result = optionListToArray(resultOptions);
  
! 	if (OidIsValid(fdwvalidator))
  		OidFunctionCall2(fdwvalidator, result, ObjectIdGetDatum(catalogId));
  
  	return result;

laurenz.albe@wien.gv.at

over 14 years ago

In reply to: Albe Laurenz (#1)

Re: Bug in SQL/MED?

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

hanada@metrosystems.co.jp

over 14 years ago

In reply to: Albe Laurenz (#1)

2 attachment(s)

Re: Bug in SQL/MED?

(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

Attachments:

fix_file_fdw.patchtext/plain; name=fix_file_fdw.patchDownload

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

sample.txttext/plain; name=sample.txtDownload

alvherre@commandprompt.com

over 14 years ago

In reply to: 花田茂 (#3)

Re: Bug in SQL/MED?

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

shigeru.hanada@gmail.com

over 14 years ago

In reply to: Alvaro Herrera (#4)

1 attachment(s)

Re: Bug in SQL/MED?

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

Attachments:

fix_file_fdw_v2.patchtext/plain; name=fix_file_fdw_v2.patchDownload

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

robertmhaas@gmail.com

over 14 years ago

In reply to: Shigeru Hanada (#5)

Re: Bug in SQL/MED?

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

laurenz.albe@wien.gv.at

over 14 years ago

In reply to: Robert Haas (#6)

Re: Bug in SQL/MED?

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

tgl@sss.pgh.pa.us

over 14 years ago

In reply to: Robert Haas (#6)

Re: Bug in SQL/MED?

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

tgl@sss.pgh.pa.us

over 14 years ago

In reply to: Tom Lane (#8)

Re: Bug in SQL/MED?

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

tgl@sss.pgh.pa.us

over 14 years ago

In reply to: Shigeru Hanada (#5)

Re: Bug in SQL/MED?

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

tgl@sss.pgh.pa.us

over 14 years ago

In reply to: Tom Lane (#9)

Re: Bug in SQL/MED?

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