Re: [HACKERS] REVIEW: PL/Python validator function
On ons, 2011-01-19 at 10:16 +0900, Hitoshi Harada wrote: Thanks. I tested the new version and looks ok. I'll mark it Ready for Commiter. Committed. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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 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:
Re: [HACKERS] REVIEW: PL/Python validator function
2011/1/18 Jan Urbański wulc...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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 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
[HACKERS] REVIEW: PL/Python validator function
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
