Re: ISBN/ISSN/ISMN/EAN13 module - Mailing list pgsql-patches
From | Jeremy Kronuz |
---|---|
Subject | Re: ISBN/ISSN/ISMN/EAN13 module |
Date | |
Msg-id | BAY107-F327BBAE40C9E946D71FCD1D8350@phx.gbl Whole thread Raw |
In response to | Re: ISBN/ISSN/ISMN/EAN13 module (Tom Lane <tgl@sss.pgh.pa.us>) |
Responses |
Re: ISBN/ISSN/ISMN/EAN13 module
(Tom Lane <tgl@sss.pgh.pa.us>)
|
List | 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
pgsql-patches by date: