On Jul 12, 2008, at 12:17, Tom Lane wrote:
BTW, I looked at the SQL file (citext.sql.in) a bit. Some comments:
Thanks a million for these, Tom. I greatly appreciate it.
* An explicit comment explaining that you're piggybacking on the I/O
functions (and some others) for "text" wouldn't be out of place.
I've added SQL comments. Were you talking about a COMMENT?
* Lose the GRANT EXECUTEs on the I/O functions; they're redundant.
(If you needed them, you'd need them on a lot more than these two.)
I'd be inclined to lose the COMMENTs on the functions too; again
these are about the least useful ones to comment on out of the
whole module.
I wondered about that; those were copied from CITEXT 1. I've removed
all GRANTs and COMMENTs.
* You should provide binary I/O (send/receive) functions, if you want
this to be an industrial-strength module. It's easy since you can
piggyback on text's.
I'm confused. Is that not what the citextin and citextout functions are?
* The type declaration needs to say storage = extended, else the type
won't be toastable.
Ah, good, thanks.
* The cast from bpchar to citext cannot be WITHOUT FUNCTION;
it needs to invoke rtrim1. Compare the bpchar to text cast.
Where do I find that? I have trouble finding the SQL that creates the
core types. :-(
* <= is surely not its own commutator.
Changed to >=.
You might try running the
opr_sanity regression test on this module to see if it finds any
other silliness. (Procedure: insert the citext definition script
into the serial_schedule list just ahead of opr_sanity, run tests,
make sure you understand the reason for any diffs in the opr_sanity
result. There will be at least one from the uses of text-related
functions for citext.)
Thanks. Added to my list.
* I think you can and should drop all of the "size" functions and
a lot of the "miscellaneous" functions: anyplace where the implicit
coercion to text would serve, you don't need a piggyback function,
and introducing one just creates risks of
can't-resolve-ambiguous-function failures. The overloaded
miscellaneous
functions are only justifiable to the extent that it's important to
preserve "citext-ness" of the result of a function, which seems at
best dubious for many of these. It is likewise pretty pointless
AFAICS
to introduce regex functions taking citext pattern arguments.
I added most of those as I wrote tests and they failed to find the
functions. Once I added the functions, they worked. But I'll do an
audit to make sure that I didn't inadvertantly leave in any unneeded
ones (I'm happy to have less code :-)).
* Don't use the OPERATOR() notation when you don't need to.
It's just clutter.
Sorry, don't know what you're referring to here. CREATE OPERATOR
appears to require parens…
Thanks for the great feedback! I'll work on getting things all
straightened out and a new patch in tonight.
Best,
David
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
