Thread: 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 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
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
Attachment
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
Attachment
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.