Re: pl/python refactoring

On 19/01/11 02:06, Hitoshi Harada wrote:
> 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.

Right, should certainly be "vice versa".

> - -1 is used as the initial value of PLyTypeInfo.is_rowtype. Why not 0?

See the comments in struct PLyTypeInfo:

is_rowtype can be: -1 = not known yet (initial state); 0 = scalar
datatype; 1 = rowtype; 2 = rowtype, but I/O functions not set up yet

> - 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.

Hm, you may be right. I haven't touched that part of the code, but now
it seems to me that for triggers we do the I/O funcs lookup for every
row. I could try to check that and fix it, but not sure if I'll have the
time, and it's been that way before. Also, the CF is already closed in
theory...

> - 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()?

Well in theory PLy_elog should be used if there's a current Python
exception set that you'd like to forward to Postgres, making it a
elog(ERROR). It's true that the usage is sometimes a bit inconsistent,
some of these inconsistencies are cleaned up in the next patches, but
probably not all of them. As for the Assert/elog, I would prefer en
elog, because if there are bugs in that code, using the wrong I/O
functions could lead to unexpected results i production (where an Assert
would not be present).

> - A line break should be added before PLy_add_exception() after "static void"

Oops, you're right.

> - 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().

Yep, that comment looks misguided.

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

Thanks, your feedback it's very valuable!

Cheers,
Jan