Re: pl/python refactoring

2011/1/19 Peter Eisentraut <peter_e(at)gmx(dot)net>:
> On mån, 2011-01-17 at 21:49 +0200, Peter Eisentraut wrote:
>> On sön, 2011-01-02 at 12:41 +0100, Jan Urbański wrote:
>> > Here they are. There are 16 patches in total. They amount to what's
>> > currently in my refactor branch (and almost to what I've sent as the
>> > complete refactor patch, while doing the splitting I discovered a few
>> > useless hunks that I've discarded).
>>
>> Committed 0001, rest later.
>
> Today committed: 3, 5, 10, 11, 12, 13

I have reviewed this original patches for myself as the fundamental of
subsequent work, and have comments.

- This is not in the patch, but around line 184 "vis versa" in comment
seems like typo.
- -1 is used as the initial value of PLyTypeInfo.is_rowtype. Why not 0?
- PLy_(input|output)_tuple_funcs() in PLy_trigger_handler() is added.
The comment says it should check for the possibility that the
relation's tupdesc changed since last call. Is it true that tupdesc
may change even in one statement? And it also says the two functions
are responsible for not doing repetitive work, but ISTM they are not
doing something to stop redundant work. The function doesn't seem like
lightweight enough to be called on each row.
- There are elog() and PLy_elog() overall, but it looks like to me
that the usage is inconsistent. Moreover, elog(ERROR, "PLyTypeInfo
struct is initialized for a Datum"); in
PLy_(input|output)_tuple_funcs() should be Assert() not elog()?
- A line break should be added before PLy_add_exception() after "static void"
- This is also not in the patch, but the comment
/* these should only be called once at the first call
* of plpython_call_handler. initialize the python interpreter
* and global data.
*/
is bogus. PLy_init_interp() is called in _PG_init().

That's all for now. Some of them must be my misunderstanding or
trivial, but I post them for the record.

Regards,

--
Hitoshi Harada