From: | Jan Urbański <wulczer(at)wulczer(dot)org> |
---|---|
To: | Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com> |
Cc: | Peter Eisentraut <peter_e(at)gmx(dot)net>, Postgres - Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: pl/python refactoring |
Date: | 2011-01-19 10:09:56 |
Message-ID: | 4D36B874.1050805@wulczer.org |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Simon Riggs | 2011-01-19 10:22:00 | Re: Replication logging |
Previous Message | Simon Riggs | 2011-01-19 10:06:09 | Re: Confusing comment in TransactionIdIsInProgress |