REVIEW: PL/Python validator function
Started by Hitoshi Haradaalmost 15 years ago6 messages
This is a review for the patch sent as
https://commitfest.postgresql.org/action/patch_view?id=456
== Submission ==
The patch applied cleanly atop of plpython refactor patches. The
format is git diff (though refactor patches is format-patch). I did
patch -p1.
It includes adequate amount of test. I found regression test failure
in plpython_error.
***************
*** 8,14 ****
'.syntaxerror'
LANGUAGE plpythonu;
ERROR: could not compile PL/Python function "python_syntax_error"
! DETAIL: SyntaxError: invalid syntax (<string>, line 2)
/* With check_function_bodies = false the function should get defined
* and the error reported when called
*/
--- 8,14 ----
'.syntaxerror'
LANGUAGE plpythonu;
ERROR: could not compile PL/Python function "python_syntax_error"
! DETAIL: SyntaxError: invalid syntax (line 2)
/* With check_function_bodies = false the function should get defined
* and the error reported when called
*/
***************
*** 19,29 ****
LANGUAGE plpythonu;
SELECT python_syntax_error();
ERROR: could not compile PL/Python function "python_syntax_error"
! DETAIL: SyntaxError: invalid syntax (<string>, line 2)
/* Run the function twice to check if the hashtable entry gets cleaned up */
SELECT python_syntax_error();
ERROR: could not compile PL/Python function "python_syntax_error"
! DETAIL: SyntaxError: invalid syntax (<string>, line 2)
RESET check_function_bodies;
/* Flat out syntax error
*/
--- 19,29 ----
LANGUAGE plpythonu;
SELECT python_syntax_error();
ERROR: could not compile PL/Python function "python_syntax_error"
! DETAIL: SyntaxError: invalid syntax (line 2)
/* Run the function twice to check if the hashtable entry gets cleaned up */
SELECT python_syntax_error();
ERROR: could not compile PL/Python function "python_syntax_error"
! DETAIL: SyntaxError: invalid syntax (line 2)
RESET check_function_bodies;
/* Flat out syntax error
*/My environment is CentOS release 5.4 (Final) with python 2.4.3
installed default.
It includes no additional doc, which seems sane, for no relevant parts
found in the existing doc.
== Usability and Feature ==
The patch works fine as advertised. CREATE FUNCTION checks the
function body syntax while before the patch it doesn't. The way of it
seems following the one of plperl.
I think catversion update should be included in the patch, since it
contains pg_pltemplate catalog changes.
== Performance ==
The performance is out of scope. The validator function is called by
the system once at the creation of functions.
== Code ==
It looks fine overall. The only thing that I came up with is trigger
check logic in PLy_procedure_is_trigger. Although it seems following
plperl's corresponding function, the check of whether the prorettype
is pseudo type looks redundant since it checks prorettype is
TRIGGEROID or OPAQUEOID later. But it is not critical.
I mark "Waiting on Author" for the regression test issue. Other points
are trivial.
Regards,
--
Hitoshi Harada
Re: REVIEW: PL/Python validator function
On 17/01/11 01:02, Hitoshi Harada wrote:
This is a review for the patch sent as
https://commitfest.postgresql.org/action/patch_view?id=456
Thanks!
It includes adequate amount of test. I found regression test failure
in plpython_error.
My environment is CentOS release 5.4 (Final) with python 2.4.3
installed default.
Darn, I would've sworn I tested all of them on older pythons. I've
reproduced it with 2.4 and earlier versions. Will fix, thanks for
spotting it.
I think catversion update should be included in the patch, since it
contains pg_pltemplate catalog changes.
I think that's usually left for the committer, otherwise it will almost
always be a source of conflicts.
It looks fine overall. The only thing that I came up with is trigger
check logic in PLy_procedure_is_trigger. Although it seems following
plperl's corresponding function, the check of whether the prorettype
is pseudo type looks redundant since it checks prorettype is
TRIGGEROID or OPAQUEOID later. But it is not critical.
Yes, you're right, a check for prorettype only should be sufficient. Wil
fix.
I mark "Waiting on Author" for the regression test issue. Other points
are trivial.
Thank you,
Jan
Re: REVIEW: PL/Python validator function
On 17/01/11 09:26, Jan Urbański wrote:
On 17/01/11 01:02, Hitoshi Harada wrote:
This is a review for the patch sent as
https://commitfest.postgresql.org/action/patch_view?id=456
It includes adequate amount of test. I found regression test failure
in plpython_error.My environment is CentOS release 5.4 (Final) with python 2.4.3
installed default.
Seems that somewhere between Python 2.4 and Python 2.6 the whole module
that was providing SyntaxError got rewritten and the way a syntax error
from Py_CompileString is reported changed :( I tried some tricks but in
the end I don't think it's worth it: I just added an alternative
regression output file for older Pythons.
It looks fine overall. The only thing that I came up with is trigger
check logic in PLy_procedure_is_trigger. Although it seems following
plperl's corresponding function, the check of whether the prorettype
is pseudo type looks redundant since it checks prorettype is
TRIGGEROID or OPAQUEOID later. But it is not critical.Yes, you're right, a check for prorettype only should be sufficient. Wil
fix.
I removed the test for TYPTYPE_PSEUDO in the is_trigger function.
Updated patch attached.
Cheers,
Jan
Attachments:
plpython-validator.difftext/x-patch; name=plpython-validator.diffDownload
diff --git a/src/include/catalog/pg_pltemplate.h b/src/include/catalog/pg_pltemplate.h
index d987ddc..c0578f9 100644
*** a/src/include/catalog/pg_pltemplate.h
--- b/src/include/catalog/pg_pltemplate.h
*************** DATA(insert ( "pltcl" t t "pltcl_call_h
*** 72,79 ****
DATA(insert ( "pltclu" f f "pltclu_call_handler" _null_ _null_ "$libdir/pltcl" _null_ ));
DATA(insert ( "plperl" t t "plperl_call_handler" "plperl_inline_handler" "plperl_validator" "$libdir/plperl" _null_ ));
DATA(insert ( "plperlu" f f "plperl_call_handler" "plperl_inline_handler" "plperl_validator" "$libdir/plperl" _null_ ));
! DATA(insert ( "plpythonu" f f "plpython_call_handler" "plpython_inline_handler" _null_ "$libdir/plpython" _null_ ));
! DATA(insert ( "plpython2u" f f "plpython_call_handler" "plpython_inline_handler" _null_ "$libdir/plpython2" _null_ ));
! DATA(insert ( "plpython3u" f f "plpython3_call_handler" "plpython3_inline_handler" _null_ "$libdir/plpython3" _null_ ));
#endif /* PG_PLTEMPLATE_H */
--- 72,79 ----
DATA(insert ( "pltclu" f f "pltclu_call_handler" _null_ _null_ "$libdir/pltcl" _null_ ));
DATA(insert ( "plperl" t t "plperl_call_handler" "plperl_inline_handler" "plperl_validator" "$libdir/plperl" _null_ ));
DATA(insert ( "plperlu" f f "plperl_call_handler" "plperl_inline_handler" "plperl_validator" "$libdir/plperl" _null_ ));
! DATA(insert ( "plpythonu" f f "plpython_call_handler" "plpython_inline_handler" "plpython_validator" "$libdir/plpython" _null_ ));
! DATA(insert ( "plpython2u" f f "plpython_call_handler" "plpython_inline_handler" "plpython_validator" "$libdir/plpython2" _null_ ));
! DATA(insert ( "plpython3u" f f "plpython3_call_handler" "plpython3_inline_handler" "plpython3_validator" "$libdir/plpython3" _null_ ));
#endif /* PG_PLTEMPLATE_H */
diff --git a/src/pl/plpython/expected/README b/src/pl/plpython/expected/README
index f011528..27c995d 100644
*** a/src/pl/plpython/expected/README
--- b/src/pl/plpython/expected/README
***************
*** 1,5 ****
--- 1,7 ----
Guide to alternative expected files:
+ plpython_error_0.out Python 2.4 and older
+
plpython_unicode.out any version, when server encoding != SQL_ASCII and client encoding = UTF8; else ...
plpython_unicode_0.out any version, when server encoding != SQL_ASCII and client encoding != UTF8; else ...
plpython_unicode_2.out Python 2.2
diff --git a/src/pl/plpython/expected/plpython_error.out b/src/pl/plpython/expected/plpython_error.out
index 70890a8..a9e1f15 100644
*** a/src/pl/plpython/expected/plpython_error.out
--- b/src/pl/plpython/expected/plpython_error.out
***************
*** 1,6 ****
--- 1,30 ----
-- test error handling, i forgot to restore Warn_restart in
-- the trigger handler once. the errors and subsequent core dump were
-- interesting.
+ /* Flat out Python syntax error
+ */
+ CREATE FUNCTION python_syntax_error() RETURNS text
+ AS
+ '.syntaxerror'
+ LANGUAGE plpythonu;
+ ERROR: could not compile PL/Python function "python_syntax_error"
+ DETAIL: SyntaxError: invalid syntax (<string>, line 2)
+ /* With check_function_bodies = false the function should get defined
+ * and the error reported when called
+ */
+ SET check_function_bodies = false;
+ CREATE FUNCTION python_syntax_error() RETURNS text
+ AS
+ '.syntaxerror'
+ LANGUAGE plpythonu;
+ SELECT python_syntax_error();
+ ERROR: could not compile PL/Python function "python_syntax_error"
+ DETAIL: SyntaxError: invalid syntax (<string>, line 2)
+ /* Run the function twice to check if the hashtable entry gets cleaned up */
+ SELECT python_syntax_error();
+ ERROR: could not compile PL/Python function "python_syntax_error"
+ DETAIL: SyntaxError: invalid syntax (<string>, line 2)
+ RESET check_function_bodies;
/* Flat out syntax error
*/
CREATE FUNCTION sql_syntax_error() RETURNS text
diff --git a/src/pl/plpython/expected/plpython_error_0.out b/src/pl/plpython/expected/plpython_error_0.out
index ...1fe3932 .
*** a/src/pl/plpython/expected/plpython_error_0.out
--- b/src/pl/plpython/expected/plpython_error_0.out
***************
*** 0 ****
--- 1,145 ----
+ -- test error handling, i forgot to restore Warn_restart in
+ -- the trigger handler once. the errors and subsequent core dump were
+ -- interesting.
+ /* Flat out Python syntax error
+ */
+ CREATE FUNCTION python_syntax_error() RETURNS text
+ AS
+ '.syntaxerror'
+ LANGUAGE plpythonu;
+ ERROR: could not compile PL/Python function "python_syntax_error"
+ DETAIL: SyntaxError: invalid syntax (line 2)
+ /* With check_function_bodies = false the function should get defined
+ * and the error reported when called
+ */
+ SET check_function_bodies = false;
+ CREATE FUNCTION python_syntax_error() RETURNS text
+ AS
+ '.syntaxerror'
+ LANGUAGE plpythonu;
+ SELECT python_syntax_error();
+ ERROR: could not compile PL/Python function "python_syntax_error"
+ DETAIL: SyntaxError: invalid syntax (line 2)
+ /* Run the function twice to check if the hashtable entry gets cleaned up */
+ SELECT python_syntax_error();
+ ERROR: could not compile PL/Python function "python_syntax_error"
+ DETAIL: SyntaxError: invalid syntax (line 2)
+ RESET check_function_bodies;
+ /* Flat out syntax error
+ */
+ CREATE FUNCTION sql_syntax_error() RETURNS text
+ AS
+ 'plpy.execute("syntax error")'
+ LANGUAGE plpythonu;
+ SELECT sql_syntax_error();
+ WARNING: plpy.SPIError: unrecognized error in PLy_spi_execute_query
+ CONTEXT: PL/Python function "sql_syntax_error"
+ ERROR: plpy.SPIError: syntax error at or near "syntax"
+ CONTEXT: PL/Python function "sql_syntax_error"
+ /* check the handling of uncaught python exceptions
+ */
+ CREATE FUNCTION exception_index_invalid(text) RETURNS text
+ AS
+ 'return args[1]'
+ LANGUAGE plpythonu;
+ SELECT exception_index_invalid('test');
+ ERROR: IndexError: list index out of range
+ CONTEXT: PL/Python function "exception_index_invalid"
+ /* check handling of nested exceptions
+ */
+ CREATE FUNCTION exception_index_invalid_nested() RETURNS text
+ AS
+ 'rv = plpy.execute("SELECT test5(''foo'')")
+ return rv[0]'
+ LANGUAGE plpythonu;
+ SELECT exception_index_invalid_nested();
+ WARNING: plpy.SPIError: unrecognized error in PLy_spi_execute_query
+ CONTEXT: PL/Python function "exception_index_invalid_nested"
+ ERROR: plpy.SPIError: function test5(unknown) does not exist
+ CONTEXT: PL/Python function "exception_index_invalid_nested"
+ /* a typo
+ */
+ CREATE FUNCTION invalid_type_uncaught(a text) RETURNS text
+ AS
+ 'if "plan" not in SD:
+ q = "SELECT fname FROM users WHERE lname = $1"
+ SD["plan"] = plpy.prepare(q, [ "test" ])
+ rv = plpy.execute(SD["plan"], [ a ])
+ if len(rv):
+ return rv[0]["fname"]
+ return None
+ '
+ LANGUAGE plpythonu;
+ SELECT invalid_type_uncaught('rick');
+ WARNING: plpy.SPIError: unrecognized error in PLy_spi_prepare
+ CONTEXT: PL/Python function "invalid_type_uncaught"
+ ERROR: plpy.SPIError: type "test" does not exist
+ CONTEXT: PL/Python function "invalid_type_uncaught"
+ /* for what it's worth catch the exception generated by
+ * the typo, and return None
+ */
+ CREATE FUNCTION invalid_type_caught(a text) RETURNS text
+ AS
+ 'if "plan" not in SD:
+ q = "SELECT fname FROM users WHERE lname = $1"
+ try:
+ SD["plan"] = plpy.prepare(q, [ "test" ])
+ except plpy.SPIError, ex:
+ plpy.notice(str(ex))
+ return None
+ rv = plpy.execute(SD["plan"], [ a ])
+ if len(rv):
+ return rv[0]["fname"]
+ return None
+ '
+ LANGUAGE plpythonu;
+ SELECT invalid_type_caught('rick');
+ WARNING: plpy.SPIError: unrecognized error in PLy_spi_prepare
+ CONTEXT: PL/Python function "invalid_type_caught"
+ NOTICE: type "test" does not exist
+ CONTEXT: PL/Python function "invalid_type_caught"
+ invalid_type_caught
+ ---------------------
+
+ (1 row)
+
+ /* for what it's worth catch the exception generated by
+ * the typo, and reraise it as a plain error
+ */
+ CREATE FUNCTION invalid_type_reraised(a text) RETURNS text
+ AS
+ 'if "plan" not in SD:
+ q = "SELECT fname FROM users WHERE lname = $1"
+ try:
+ SD["plan"] = plpy.prepare(q, [ "test" ])
+ except plpy.SPIError, ex:
+ plpy.error(str(ex))
+ rv = plpy.execute(SD["plan"], [ a ])
+ if len(rv):
+ return rv[0]["fname"]
+ return None
+ '
+ LANGUAGE plpythonu;
+ SELECT invalid_type_reraised('rick');
+ WARNING: plpy.SPIError: unrecognized error in PLy_spi_prepare
+ CONTEXT: PL/Python function "invalid_type_reraised"
+ ERROR: plpy.Error: type "test" does not exist
+ CONTEXT: PL/Python function "invalid_type_reraised"
+ /* no typo no messing about
+ */
+ CREATE FUNCTION valid_type(a text) RETURNS text
+ AS
+ 'if "plan" not in SD:
+ SD["plan"] = plpy.prepare("SELECT fname FROM users WHERE lname = $1", [ "text" ])
+ rv = plpy.execute(SD["plan"], [ a ])
+ if len(rv):
+ return rv[0]["fname"]
+ return None
+ '
+ LANGUAGE plpythonu;
+ SELECT valid_type('rick');
+ valid_type
+ ------------
+
+ (1 row)
+
diff --git a/src/pl/plpython/expected/plpython_record.out b/src/pl/plpython/expected/plpython_record.out
index c8c4f9d..770f764 100644
*** a/src/pl/plpython/expected/plpython_record.out
--- b/src/pl/plpython/expected/plpython_record.out
*************** CREATE FUNCTION test_in_out_params_multi
*** 47,52 ****
--- 47,53 ----
second out text, third out text) AS $$
return first + '_record_in_to_out';
$$ LANGUAGE plpythonu;
+ ERROR: PL/Python functions cannot return type record
CREATE FUNCTION test_inout_params(first inout text) AS $$
return first + '_inout';
$$ LANGUAGE plpythonu;
*************** SELECT * FROM test_in_out_params('test_i
*** 299,305 ****
-- this doesn't work yet :-(
SELECT * FROM test_in_out_params_multi('test_in');
! ERROR: PL/Python functions cannot return type record
SELECT * FROM test_inout_params('test_in');
first
---------------
--- 300,309 ----
-- this doesn't work yet :-(
SELECT * FROM test_in_out_params_multi('test_in');
! ERROR: function test_in_out_params_multi(unknown) does not exist
! LINE 1: SELECT * FROM test_in_out_params_multi('test_in');
! ^
! HINT: No function matches the given name and argument types. You might need to add explicit type casts.
SELECT * FROM test_inout_params('test_in');
first
---------------
diff --git a/src/pl/plpython/expected/plpython_types.out b/src/pl/plpython/expected/plpython_types.out
index 982005b..e74a400 100644
*** a/src/pl/plpython/expected/plpython_types.out
--- b/src/pl/plpython/expected/plpython_types.out
*************** PL/Python function "test_type_conversion
*** 571,579 ****
CREATE FUNCTION test_type_conversion_array_record() RETURNS type_record[] AS $$
return [None]
$$ LANGUAGE plpythonu;
- SELECT * FROM test_type_conversion_array_record();
ERROR: PL/Python functions cannot return type type_record[]
DETAIL: PL/Python does not support conversion to arrays of row types.
CREATE FUNCTION test_type_conversion_array_string() RETURNS text[] AS $$
return 'abc'
$$ LANGUAGE plpythonu;
--- 571,583 ----
CREATE FUNCTION test_type_conversion_array_record() RETURNS type_record[] AS $$
return [None]
$$ LANGUAGE plpythonu;
ERROR: PL/Python functions cannot return type type_record[]
DETAIL: PL/Python does not support conversion to arrays of row types.
+ SELECT * FROM test_type_conversion_array_record();
+ ERROR: function test_type_conversion_array_record() does not exist
+ LINE 1: SELECT * FROM test_type_conversion_array_record();
+ ^
+ HINT: No function matches the given name and argument types. You might need to add explicit type casts.
CREATE FUNCTION test_type_conversion_array_string() RETURNS text[] AS $$
return 'abc'
$$ LANGUAGE plpythonu;
diff --git a/src/pl/plpython/expected/plpython_types_3.out b/src/pl/plpython/expected/plpython_types_3.out
index eeb23b7..577c1ff 100644
*** a/src/pl/plpython/expected/plpython_types_3.out
--- b/src/pl/plpython/expected/plpython_types_3.out
*************** PL/Python function "test_type_conversion
*** 571,579 ****
CREATE FUNCTION test_type_conversion_array_record() RETURNS type_record[] AS $$
return [None]
$$ LANGUAGE plpython3u;
- SELECT * FROM test_type_conversion_array_record();
ERROR: PL/Python functions cannot return type type_record[]
DETAIL: PL/Python does not support conversion to arrays of row types.
CREATE FUNCTION test_type_conversion_array_string() RETURNS text[] AS $$
return 'abc'
$$ LANGUAGE plpython3u;
--- 571,583 ----
CREATE FUNCTION test_type_conversion_array_record() RETURNS type_record[] AS $$
return [None]
$$ LANGUAGE plpython3u;
ERROR: PL/Python functions cannot return type type_record[]
DETAIL: PL/Python does not support conversion to arrays of row types.
+ SELECT * FROM test_type_conversion_array_record();
+ ERROR: function test_type_conversion_array_record() does not exist
+ LINE 1: SELECT * FROM test_type_conversion_array_record();
+ ^
+ HINT: No function matches the given name and argument types. You might need to add explicit type casts.
CREATE FUNCTION test_type_conversion_array_string() RETURNS text[] AS $$
return 'abc'
$$ LANGUAGE plpython3u;
diff --git a/src/pl/plpython/plpython.c b/src/pl/plpython/plpython.c
index 2307627..2f7a0a7 100644
*** a/src/pl/plpython/plpython.c
--- b/src/pl/plpython/plpython.c
*************** typedef struct PLyResultObject
*** 248,262 ****
--- 248,265 ----
#if PY_MAJOR_VERSION >= 3
/* Use separate names to avoid clash in pg_pltemplate */
+ #define plpython_validator plpython3_validator
#define plpython_call_handler plpython3_call_handler
#define plpython_inline_handler plpython3_inline_handler
#endif
/* exported functions */
+ Datum plpython_validator(PG_FUNCTION_ARGS);
Datum plpython_call_handler(PG_FUNCTION_ARGS);
Datum plpython_inline_handler(PG_FUNCTION_ARGS);
void _PG_init(void);
+ PG_FUNCTION_INFO_V1(plpython_validator);
PG_FUNCTION_INFO_V1(plpython_call_handler);
PG_FUNCTION_INFO_V1(plpython_inline_handler);
*************** plpython_return_error_callback(void *arg
*** 431,436 ****
--- 434,475 ----
errcontext("while creating return value");
}
+ static bool
+ PLy_procedure_is_trigger(Form_pg_proc procStruct)
+ {
+ return (procStruct->prorettype == TRIGGEROID ||
+ (procStruct->prorettype == OPAQUEOID &&
+ procStruct->pronargs == 0));
+ }
+
+ Datum
+ plpython_validator(PG_FUNCTION_ARGS)
+ {
+ Oid funcoid = PG_GETARG_OID(0);
+ HeapTuple tuple;
+ Form_pg_proc procStruct;
+ bool is_trigger;
+
+ if (!check_function_bodies)
+ {
+ PG_RETURN_VOID();
+ }
+
+ /* Get the new function's pg_proc entry */
+ tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcoid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for function %u", funcoid);
+ procStruct = (Form_pg_proc) GETSTRUCT(tuple);
+
+ is_trigger = PLy_procedure_is_trigger(procStruct);
+
+ ReleaseSysCache(tuple);
+
+ PLy_procedure_get(funcoid, is_trigger);
+
+ PG_RETURN_VOID();
+ }
+
Datum
plpython_call_handler(PG_FUNCTION_ARGS)
{
diff --git a/src/pl/plpython/sql/plpython_error.sql b/src/pl/plpython/sql/plpython_error.sql
index 5ca6849..6509257 100644
*** a/src/pl/plpython/sql/plpython_error.sql
--- b/src/pl/plpython/sql/plpython_error.sql
***************
*** 2,7 ****
--- 2,30 ----
-- the trigger handler once. the errors and subsequent core dump were
-- interesting.
+ /* Flat out Python syntax error
+ */
+ CREATE FUNCTION python_syntax_error() RETURNS text
+ AS
+ '.syntaxerror'
+ LANGUAGE plpythonu;
+
+ /* With check_function_bodies = false the function should get defined
+ * and the error reported when called
+ */
+ SET check_function_bodies = false;
+
+ CREATE FUNCTION python_syntax_error() RETURNS text
+ AS
+ '.syntaxerror'
+ LANGUAGE plpythonu;
+
+ SELECT python_syntax_error();
+ /* Run the function twice to check if the hashtable entry gets cleaned up */
+ SELECT python_syntax_error();
+
+ RESET check_function_bodies;
+
/* Flat out syntax error
*/
CREATE FUNCTION sql_syntax_error() RETURNS text
Re: REVIEW: PL/Python validator function
2011/1/18 Jan Urbański <wulczer@wulczer.org>:
On 17/01/11 09:26, Jan Urbański wrote:
On 17/01/11 01:02, Hitoshi Harada wrote:
This is a review for the patch sent as
https://commitfest.postgresql.org/action/patch_view?id=456
It includes adequate amount of test. I found regression test failure
in plpython_error.My environment is CentOS release 5.4 (Final) with python 2.4.3
installed default.Seems that somewhere between Python 2.4 and Python 2.6 the whole module
that was providing SyntaxError got rewritten and the way a syntax error
from Py_CompileString is reported changed :( I tried some tricks but in
the end I don't think it's worth it: I just added an alternative
regression output file for older Pythons.It looks fine overall. The only thing that I came up with is trigger
check logic in PLy_procedure_is_trigger. Although it seems following
plperl's corresponding function, the check of whether the prorettype
is pseudo type looks redundant since it checks prorettype is
TRIGGEROID or OPAQUEOID later. But it is not critical.Yes, you're right, a check for prorettype only should be sufficient. Wil
fix.I removed the test for TYPTYPE_PSEUDO in the is_trigger function.
Updated patch attached.
Thanks. I tested the new version and looks ok. I'll mark it "Ready for
Commiter".
Regards,
--
Hitoshi Harada
Re: REVIEW: PL/Python validator function
On 19/01/11 02:16, Hitoshi Harada wrote:
Thanks. I tested the new version and looks ok. I'll mark it "Ready for
Commiter".
Updated version against master.
Jan
Attachments:
plpython-validator.patchtext/x-patch; name=plpython-validator.patchDownload
diff --git a/src/include/catalog/pg_pltemplate.h b/src/include/catalog/pg_pltemplate.h
index d987ddc..c0578f9 100644
*** a/src/include/catalog/pg_pltemplate.h
--- b/src/include/catalog/pg_pltemplate.h
*************** DATA(insert ( "pltcl" t t "pltcl_call_h
*** 72,79 ****
DATA(insert ( "pltclu" f f "pltclu_call_handler" _null_ _null_ "$libdir/pltcl" _null_ ));
DATA(insert ( "plperl" t t "plperl_call_handler" "plperl_inline_handler" "plperl_validator" "$libdir/plperl" _null_ ));
DATA(insert ( "plperlu" f f "plperl_call_handler" "plperl_inline_handler" "plperl_validator" "$libdir/plperl" _null_ ));
! DATA(insert ( "plpythonu" f f "plpython_call_handler" "plpython_inline_handler" _null_ "$libdir/plpython" _null_ ));
! DATA(insert ( "plpython2u" f f "plpython_call_handler" "plpython_inline_handler" _null_ "$libdir/plpython2" _null_ ));
! DATA(insert ( "plpython3u" f f "plpython3_call_handler" "plpython3_inline_handler" _null_ "$libdir/plpython3" _null_ ));
#endif /* PG_PLTEMPLATE_H */
--- 72,79 ----
DATA(insert ( "pltclu" f f "pltclu_call_handler" _null_ _null_ "$libdir/pltcl" _null_ ));
DATA(insert ( "plperl" t t "plperl_call_handler" "plperl_inline_handler" "plperl_validator" "$libdir/plperl" _null_ ));
DATA(insert ( "plperlu" f f "plperl_call_handler" "plperl_inline_handler" "plperl_validator" "$libdir/plperl" _null_ ));
! DATA(insert ( "plpythonu" f f "plpython_call_handler" "plpython_inline_handler" "plpython_validator" "$libdir/plpython" _null_ ));
! DATA(insert ( "plpython2u" f f "plpython_call_handler" "plpython_inline_handler" "plpython_validator" "$libdir/plpython2" _null_ ));
! DATA(insert ( "plpython3u" f f "plpython3_call_handler" "plpython3_inline_handler" "plpython3_validator" "$libdir/plpython3" _null_ ));
#endif /* PG_PLTEMPLATE_H */
diff --git a/src/pl/plpython/expected/README b/src/pl/plpython/expected/README
index f011528..27c995d 100644
*** a/src/pl/plpython/expected/README
--- b/src/pl/plpython/expected/README
***************
*** 1,5 ****
--- 1,7 ----
Guide to alternative expected files:
+ plpython_error_0.out Python 2.4 and older
+
plpython_unicode.out any version, when server encoding != SQL_ASCII and client encoding = UTF8; else ...
plpython_unicode_0.out any version, when server encoding != SQL_ASCII and client encoding != UTF8; else ...
plpython_unicode_2.out Python 2.2
diff --git a/src/pl/plpython/expected/plpython_error.out b/src/pl/plpython/expected/plpython_error.out
index ea4a54c..2b6141c 100644
*** a/src/pl/plpython/expected/plpython_error.out
--- b/src/pl/plpython/expected/plpython_error.out
***************
*** 1,6 ****
--- 1,30 ----
-- test error handling, i forgot to restore Warn_restart in
-- the trigger handler once. the errors and subsequent core dump were
-- interesting.
+ /* Flat out Python syntax error
+ */
+ CREATE FUNCTION python_syntax_error() RETURNS text
+ AS
+ '.syntaxerror'
+ LANGUAGE plpythonu;
+ ERROR: could not compile PL/Python function "python_syntax_error"
+ DETAIL: SyntaxError: invalid syntax (<string>, line 2)
+ /* With check_function_bodies = false the function should get defined
+ * and the error reported when called
+ */
+ SET check_function_bodies = false;
+ CREATE FUNCTION python_syntax_error() RETURNS text
+ AS
+ '.syntaxerror'
+ LANGUAGE plpythonu;
+ SELECT python_syntax_error();
+ ERROR: could not compile PL/Python function "python_syntax_error"
+ DETAIL: SyntaxError: invalid syntax (<string>, line 2)
+ /* Run the function twice to check if the hashtable entry gets cleaned up */
+ SELECT python_syntax_error();
+ ERROR: could not compile PL/Python function "python_syntax_error"
+ DETAIL: SyntaxError: invalid syntax (<string>, line 2)
+ RESET check_function_bodies;
/* Flat out syntax error
*/
CREATE FUNCTION sql_syntax_error() RETURNS text
diff --git a/src/pl/plpython/expected/plpython_error_0.out b/src/pl/plpython/expected/plpython_error_0.out
index ...1fe3932 .
*** a/src/pl/plpython/expected/plpython_error_0.out
--- b/src/pl/plpython/expected/plpython_error_0.out
***************
*** 0 ****
--- 1,145 ----
+ -- test error handling, i forgot to restore Warn_restart in
+ -- the trigger handler once. the errors and subsequent core dump were
+ -- interesting.
+ /* Flat out Python syntax error
+ */
+ CREATE FUNCTION python_syntax_error() RETURNS text
+ AS
+ '.syntaxerror'
+ LANGUAGE plpythonu;
+ ERROR: could not compile PL/Python function "python_syntax_error"
+ DETAIL: SyntaxError: invalid syntax (line 2)
+ /* With check_function_bodies = false the function should get defined
+ * and the error reported when called
+ */
+ SET check_function_bodies = false;
+ CREATE FUNCTION python_syntax_error() RETURNS text
+ AS
+ '.syntaxerror'
+ LANGUAGE plpythonu;
+ SELECT python_syntax_error();
+ ERROR: could not compile PL/Python function "python_syntax_error"
+ DETAIL: SyntaxError: invalid syntax (line 2)
+ /* Run the function twice to check if the hashtable entry gets cleaned up */
+ SELECT python_syntax_error();
+ ERROR: could not compile PL/Python function "python_syntax_error"
+ DETAIL: SyntaxError: invalid syntax (line 2)
+ RESET check_function_bodies;
+ /* Flat out syntax error
+ */
+ CREATE FUNCTION sql_syntax_error() RETURNS text
+ AS
+ 'plpy.execute("syntax error")'
+ LANGUAGE plpythonu;
+ SELECT sql_syntax_error();
+ WARNING: plpy.SPIError: unrecognized error in PLy_spi_execute_query
+ CONTEXT: PL/Python function "sql_syntax_error"
+ ERROR: plpy.SPIError: syntax error at or near "syntax"
+ CONTEXT: PL/Python function "sql_syntax_error"
+ /* check the handling of uncaught python exceptions
+ */
+ CREATE FUNCTION exception_index_invalid(text) RETURNS text
+ AS
+ 'return args[1]'
+ LANGUAGE plpythonu;
+ SELECT exception_index_invalid('test');
+ ERROR: IndexError: list index out of range
+ CONTEXT: PL/Python function "exception_index_invalid"
+ /* check handling of nested exceptions
+ */
+ CREATE FUNCTION exception_index_invalid_nested() RETURNS text
+ AS
+ 'rv = plpy.execute("SELECT test5(''foo'')")
+ return rv[0]'
+ LANGUAGE plpythonu;
+ SELECT exception_index_invalid_nested();
+ WARNING: plpy.SPIError: unrecognized error in PLy_spi_execute_query
+ CONTEXT: PL/Python function "exception_index_invalid_nested"
+ ERROR: plpy.SPIError: function test5(unknown) does not exist
+ CONTEXT: PL/Python function "exception_index_invalid_nested"
+ /* a typo
+ */
+ CREATE FUNCTION invalid_type_uncaught(a text) RETURNS text
+ AS
+ 'if "plan" not in SD:
+ q = "SELECT fname FROM users WHERE lname = $1"
+ SD["plan"] = plpy.prepare(q, [ "test" ])
+ rv = plpy.execute(SD["plan"], [ a ])
+ if len(rv):
+ return rv[0]["fname"]
+ return None
+ '
+ LANGUAGE plpythonu;
+ SELECT invalid_type_uncaught('rick');
+ WARNING: plpy.SPIError: unrecognized error in PLy_spi_prepare
+ CONTEXT: PL/Python function "invalid_type_uncaught"
+ ERROR: plpy.SPIError: type "test" does not exist
+ CONTEXT: PL/Python function "invalid_type_uncaught"
+ /* for what it's worth catch the exception generated by
+ * the typo, and return None
+ */
+ CREATE FUNCTION invalid_type_caught(a text) RETURNS text
+ AS
+ 'if "plan" not in SD:
+ q = "SELECT fname FROM users WHERE lname = $1"
+ try:
+ SD["plan"] = plpy.prepare(q, [ "test" ])
+ except plpy.SPIError, ex:
+ plpy.notice(str(ex))
+ return None
+ rv = plpy.execute(SD["plan"], [ a ])
+ if len(rv):
+ return rv[0]["fname"]
+ return None
+ '
+ LANGUAGE plpythonu;
+ SELECT invalid_type_caught('rick');
+ WARNING: plpy.SPIError: unrecognized error in PLy_spi_prepare
+ CONTEXT: PL/Python function "invalid_type_caught"
+ NOTICE: type "test" does not exist
+ CONTEXT: PL/Python function "invalid_type_caught"
+ invalid_type_caught
+ ---------------------
+
+ (1 row)
+
+ /* for what it's worth catch the exception generated by
+ * the typo, and reraise it as a plain error
+ */
+ CREATE FUNCTION invalid_type_reraised(a text) RETURNS text
+ AS
+ 'if "plan" not in SD:
+ q = "SELECT fname FROM users WHERE lname = $1"
+ try:
+ SD["plan"] = plpy.prepare(q, [ "test" ])
+ except plpy.SPIError, ex:
+ plpy.error(str(ex))
+ rv = plpy.execute(SD["plan"], [ a ])
+ if len(rv):
+ return rv[0]["fname"]
+ return None
+ '
+ LANGUAGE plpythonu;
+ SELECT invalid_type_reraised('rick');
+ WARNING: plpy.SPIError: unrecognized error in PLy_spi_prepare
+ CONTEXT: PL/Python function "invalid_type_reraised"
+ ERROR: plpy.Error: type "test" does not exist
+ CONTEXT: PL/Python function "invalid_type_reraised"
+ /* no typo no messing about
+ */
+ CREATE FUNCTION valid_type(a text) RETURNS text
+ AS
+ 'if "plan" not in SD:
+ SD["plan"] = plpy.prepare("SELECT fname FROM users WHERE lname = $1", [ "text" ])
+ rv = plpy.execute(SD["plan"], [ a ])
+ if len(rv):
+ return rv[0]["fname"]
+ return None
+ '
+ LANGUAGE plpythonu;
+ SELECT valid_type('rick');
+ valid_type
+ ------------
+
+ (1 row)
+
diff --git a/src/pl/plpython/expected/plpython_record.out b/src/pl/plpython/expected/plpython_record.out
index c8c4f9d..770f764 100644
*** a/src/pl/plpython/expected/plpython_record.out
--- b/src/pl/plpython/expected/plpython_record.out
*************** CREATE FUNCTION test_in_out_params_multi
*** 47,52 ****
--- 47,53 ----
second out text, third out text) AS $$
return first + '_record_in_to_out';
$$ LANGUAGE plpythonu;
+ ERROR: PL/Python functions cannot return type record
CREATE FUNCTION test_inout_params(first inout text) AS $$
return first + '_inout';
$$ LANGUAGE plpythonu;
*************** SELECT * FROM test_in_out_params('test_i
*** 299,305 ****
-- this doesn't work yet :-(
SELECT * FROM test_in_out_params_multi('test_in');
! ERROR: PL/Python functions cannot return type record
SELECT * FROM test_inout_params('test_in');
first
---------------
--- 300,309 ----
-- this doesn't work yet :-(
SELECT * FROM test_in_out_params_multi('test_in');
! ERROR: function test_in_out_params_multi(unknown) does not exist
! LINE 1: SELECT * FROM test_in_out_params_multi('test_in');
! ^
! HINT: No function matches the given name and argument types. You might need to add explicit type casts.
SELECT * FROM test_inout_params('test_in');
first
---------------
diff --git a/src/pl/plpython/expected/plpython_types.out b/src/pl/plpython/expected/plpython_types.out
index 982005b..e74a400 100644
*** a/src/pl/plpython/expected/plpython_types.out
--- b/src/pl/plpython/expected/plpython_types.out
*************** PL/Python function "test_type_conversion
*** 571,579 ****
CREATE FUNCTION test_type_conversion_array_record() RETURNS type_record[] AS $$
return [None]
$$ LANGUAGE plpythonu;
- SELECT * FROM test_type_conversion_array_record();
ERROR: PL/Python functions cannot return type type_record[]
DETAIL: PL/Python does not support conversion to arrays of row types.
CREATE FUNCTION test_type_conversion_array_string() RETURNS text[] AS $$
return 'abc'
$$ LANGUAGE plpythonu;
--- 571,583 ----
CREATE FUNCTION test_type_conversion_array_record() RETURNS type_record[] AS $$
return [None]
$$ LANGUAGE plpythonu;
ERROR: PL/Python functions cannot return type type_record[]
DETAIL: PL/Python does not support conversion to arrays of row types.
+ SELECT * FROM test_type_conversion_array_record();
+ ERROR: function test_type_conversion_array_record() does not exist
+ LINE 1: SELECT * FROM test_type_conversion_array_record();
+ ^
+ HINT: No function matches the given name and argument types. You might need to add explicit type casts.
CREATE FUNCTION test_type_conversion_array_string() RETURNS text[] AS $$
return 'abc'
$$ LANGUAGE plpythonu;
diff --git a/src/pl/plpython/expected/plpython_types_3.out b/src/pl/plpython/expected/plpython_types_3.out
index eeb23b7..577c1ff 100644
*** a/src/pl/plpython/expected/plpython_types_3.out
--- b/src/pl/plpython/expected/plpython_types_3.out
*************** PL/Python function "test_type_conversion
*** 571,579 ****
CREATE FUNCTION test_type_conversion_array_record() RETURNS type_record[] AS $$
return [None]
$$ LANGUAGE plpython3u;
- SELECT * FROM test_type_conversion_array_record();
ERROR: PL/Python functions cannot return type type_record[]
DETAIL: PL/Python does not support conversion to arrays of row types.
CREATE FUNCTION test_type_conversion_array_string() RETURNS text[] AS $$
return 'abc'
$$ LANGUAGE plpython3u;
--- 571,583 ----
CREATE FUNCTION test_type_conversion_array_record() RETURNS type_record[] AS $$
return [None]
$$ LANGUAGE plpython3u;
ERROR: PL/Python functions cannot return type type_record[]
DETAIL: PL/Python does not support conversion to arrays of row types.
+ SELECT * FROM test_type_conversion_array_record();
+ ERROR: function test_type_conversion_array_record() does not exist
+ LINE 1: SELECT * FROM test_type_conversion_array_record();
+ ^
+ HINT: No function matches the given name and argument types. You might need to add explicit type casts.
CREATE FUNCTION test_type_conversion_array_string() RETURNS text[] AS $$
return 'abc'
$$ LANGUAGE plpython3u;
diff --git a/src/pl/plpython/plpython.c b/src/pl/plpython/plpython.c
index aafe556..fbfeb2c 100644
*** a/src/pl/plpython/plpython.c
--- b/src/pl/plpython/plpython.c
*************** typedef struct PLyResultObject
*** 251,265 ****
--- 251,268 ----
#if PY_MAJOR_VERSION >= 3
/* Use separate names to avoid clash in pg_pltemplate */
+ #define plpython_validator plpython3_validator
#define plpython_call_handler plpython3_call_handler
#define plpython_inline_handler plpython3_inline_handler
#endif
/* exported functions */
+ Datum plpython_validator(PG_FUNCTION_ARGS);
Datum plpython_call_handler(PG_FUNCTION_ARGS);
Datum plpython_inline_handler(PG_FUNCTION_ARGS);
void _PG_init(void);
+ PG_FUNCTION_INFO_V1(plpython_validator);
PG_FUNCTION_INFO_V1(plpython_call_handler);
PG_FUNCTION_INFO_V1(plpython_inline_handler);
*************** plpython_return_error_callback(void *arg
*** 437,442 ****
--- 440,481 ----
errcontext("while creating return value");
}
+ static bool
+ PLy_procedure_is_trigger(Form_pg_proc procStruct)
+ {
+ return (procStruct->prorettype == TRIGGEROID ||
+ (procStruct->prorettype == OPAQUEOID &&
+ procStruct->pronargs == 0));
+ }
+
+ Datum
+ plpython_validator(PG_FUNCTION_ARGS)
+ {
+ Oid funcoid = PG_GETARG_OID(0);
+ HeapTuple tuple;
+ Form_pg_proc procStruct;
+ bool is_trigger;
+
+ if (!check_function_bodies)
+ {
+ PG_RETURN_VOID();
+ }
+
+ /* Get the new function's pg_proc entry */
+ tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcoid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for function %u", funcoid);
+ procStruct = (Form_pg_proc) GETSTRUCT(tuple);
+
+ is_trigger = PLy_procedure_is_trigger(procStruct);
+
+ ReleaseSysCache(tuple);
+
+ PLy_procedure_get(funcoid, is_trigger);
+
+ PG_RETURN_VOID();
+ }
+
Datum
plpython_call_handler(PG_FUNCTION_ARGS)
{
diff --git a/src/pl/plpython/sql/plpython_error.sql b/src/pl/plpython/sql/plpython_error.sql
index 5ca6849..6509257 100644
*** a/src/pl/plpython/sql/plpython_error.sql
--- b/src/pl/plpython/sql/plpython_error.sql
***************
*** 2,7 ****
--- 2,30 ----
-- the trigger handler once. the errors and subsequent core dump were
-- interesting.
+ /* Flat out Python syntax error
+ */
+ CREATE FUNCTION python_syntax_error() RETURNS text
+ AS
+ '.syntaxerror'
+ LANGUAGE plpythonu;
+
+ /* With check_function_bodies = false the function should get defined
+ * and the error reported when called
+ */
+ SET check_function_bodies = false;
+
+ CREATE FUNCTION python_syntax_error() RETURNS text
+ AS
+ '.syntaxerror'
+ LANGUAGE plpythonu;
+
+ SELECT python_syntax_error();
+ /* Run the function twice to check if the hashtable entry gets cleaned up */
+ SELECT python_syntax_error();
+
+ RESET check_function_bodies;
+
/* Flat out syntax error
*/
CREATE FUNCTION sql_syntax_error() RETURNS text
