Re: 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