Re: Rewriting PL/Python's typeio code

I started to work on teaching PL/Python about domains over composite,
and soon found that it was a can of worms. Aside from the difficulty
of shoehorning that in with a minimal patch, there were pre-existing
problems. I found that it didn't do arrays of domains right either
(ok, that's an oversight in my recent commit c12d570fa), and there
are assorted bugs that have been there much longer. For instance, if
you return a composite type containing a domain, it fails to enforce
domain constraints on the type's field. Also, if a transform function
is in use, it missed enforcing domain constraints on the result.
And in many places it missed checking domain constraints on null values,
because the plpy_typeio code simply wasn't called for Py_None.

Plus the code was really messy and duplicative, e.g. domain_check was
called in three different places ... which wasn't enough. It also did
a lot of repetitive catalog lookups.

So, I ended up rewriting/refactoring pretty heavily. The new idea
is to solve these problems by making heavier use of recursion between
plpy_typeio's conversion functions, and in particular to treat domains
as if they were containers. So now there's exactly one place to call
domain_check, in a conversion function that has first recursed to do
conversion of the base type. Nulls are treated more honestly, and
the SQL-to-Python functions are more careful about not leaking memory.
Also, I solved some of the repetitive catalog lookup problems by
making the code rely as much as possible on the typcache (which I think
didn't exist when this code originated). I added a couple of small
features to typcache to help with that.

This is a fairly large amount of code churn, and it could stand testing
by someone who's more Python-savvy than I am. So I'll stick it into
the upcoming commitfest as a separate item.

regards, tom lane

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: not tested
Spec compliant: not tested
Documentation: tested, passed

Hello,
I have checked your patch. Everything looks fine for me, but I have some doubts:
1. In file plpy_exec.c there is a typo on line 347:
"... We use the result and resultin[should be here "g"?] variables...
2. In file plpy_cursorobject.c there is a non-volatile variable "elem" used in try-except construction. Is that OK?

--
Anthony Bykov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

The new status of this patch is: Waiting on Author

Anthony Bykov <a(dot)bykov(at)postgrespro(dot)ru> writes:
> I have checked your patch. Everything looks fine for me, but I have some doubts:

Thanks for reviewing!

> 1. In file plpy_exec.c there is a typo on line 347:
> "... We use the result and resultin[should be here "g"?] variables...

No, "resultin" is the name of the variable. Maybe that wasn't a good
choice of name, though --- do you have a better idea?

> 2. In file plpy_cursorobject.c there is a non-volatile variable "elem" used in try-except construction. Is that OK?

Hm, my compiler didn't complain about that. Did yours? The variable
is not changed inside the PG_TRY, so according to my ideas of how
this works, it should be OK. Also, it was like that before, and no
one has reported a problem.

regards, tom lane

I wrote:
> Anthony Bykov <a(dot)bykov(at)postgrespro(dot)ru> writes:
>> 1. In file plpy_exec.c there is a typo on line 347:
>> "... We use the result and resultin[should be here "g"?] variables...

> No, "resultin" is the name of the variable. Maybe that wasn't a good
> choice of name, though --- do you have a better idea?

After some thought I went with "result_in", which hopefully looks less
like a typo. Thanks again for reviewing.

regards, tom lane