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

2011/1/18 Jan Urbański <wulczer(at)wulczer(dot)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

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.