REVIEW: PL/Python validator function - Mailing list pgsql-hackers

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
getscleaned 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