Re: pl/python refactoring

On Wed, Jan 19, 2011 at 11:09:56AM +0100, Jan Urbański wrote:
> 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...

If you're fixing things in PL/PythonU, such a change would certainly
be in scope as an update to your patch, i.e. don't let the fact that
the CF has started stop you from fixing it.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate