Re: ISBN/ISSN/ISMN/EAN13 module

From: "Jeremy Kronuz" <kronuz(at)hotmail(dot)com>
To: pgsql-hackers(at)postgresql(dot)org, pgsql-patches(at)postgresql(dot)org
Cc: tgl(at)sss(dot)pgh(dot)pa(dot)us
Subject: Re: ISBN/ISSN/ISMN/EAN13 module
Date: 2006-09-10 15:21:16
Message-ID: BAY107-F327BBAE40C9E946D71FCD1D8350@phx.gbl
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers pgsql-patches

> > Tom, I've checked the version in the cvs and I had made significant
>changes
> > from that version.
>
>Hm, it sounds like I guessed wrong about which version was newer ... is
>there something flaky about your machine's system clock? The file
>timestamps in the two tarballs definitely pointed the other way.

Nope, not a system clock problem, not that I know of... I don't know what
the problem was... anyway, with the patch I made it should now be fully
functional.

> > I fixed some major bugs that prevented some ISBN numbers
> > from working. I've merged the changes made for the cvs with my own,
>hoping I
> > didn't miss anything that was already fixed in the cvs... I noticed you
> > changed some log messages and that you added GET_STR and GET_TEXT.
>indeed
> > there was that thing that took me some time to figure in both
> > ean13_cast_to_text() and isn_cast_to_text() functions, not changing to a
> > valid TEXT the return value; but I did not know about the other
> > *_cast_from_text() functions having the problem of the missing GET_STR()
>(I
> > was using VARDATA()).
>
>Yeah, your to/from text functions simply did not work --- the VARDATA
>hack for example presumes that the data part of a text value is
>null-terminated, which it isn't. In any case it seems pretty likely
>that the content of TEXT datums will change someday to allow better
>charset/collation support, so you really are better off calling
>textin/textout if possible rather than assuming you know the
>representation of type TEXT. I copied the GET_STR/GET_TEXT macros
>from some other contrib module ... you're free to do it differently
>if you want, but in any case you have to deal with the fact that TEXT
>is *not* a C-string.

Yeah, that's what I painfully learned, specially with the *_cast_to_text()
functions... those were definitely crashing the server. I didn't know the
VARDATA() "hack" could fail at some point (even in the patch, I already
replaced it with GET_STR(), just in case)... and since it took me so long to
figure out why the other two functions where crashing, and not knowing what
exactly was that you made with the GET_TEXT() macro, I just decided to use
my own patched version... I suppose the use of GET_TEXT() is better, as it's
smaller and makes the code easier to follow; perhaps we might want to change
those two functions back the way you had them (using GET_TEXT())

>As for the log message changes, there's room for debate about that, but
>the way you had it struck me as far too chatty. The use-case for weak
>checking is that you have a large pile of data you don't want to
>validate right now, correct? So why would you want a warning for every
>single bad datum during the import? Also, all the fancy formatting in
>that one big warning was directly against our message style guidelines.

That's right, that's the idea behind "weak mode"... I've been in that
position and I found the weak mode very useful, so I thought it would be
nice to have it as a feature that others could benefit from. And yes, I know
the messages were far too chatty, as you rightfully said so. In my latest
version (before the patch) I took away some of the messages, and I also
shortened the *big* warning to a single line but I think not having so many
messages it's truly better; I wasn't sure when the messages were shown (this
is my first module, you know) but it seems those removed messages were
really not needed at all.

I'm very happy to have contributed a bit to the growth and improvement of
PostgreSQL, and I look forward the day I can make bigger/better
contributions. And Tom, were you able to apply the patch without problems
('cause this is one of the first times I post a patch in a mailing list.)

Regards,
Kronuz

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Gregory Stark 2006-09-10 16:23:07 Re: Foreign keys
Previous Message Tom Lane 2006-09-10 15:09:31 Re: Foreign keys

Browse pgsql-patches by date

  From Date Subject
Next Message Tom Lane 2006-09-10 18:08:45 contrib uninstall scripts need some love
Previous Message Tom Lane 2006-09-10 00:41:59 Re: ISBN/ISSN/ISMN/EAN13 module