> 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
---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend
