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
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
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+255-13
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
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
