On Jul 12, 2008, at 14:50, David E. Wheeler wrote:
* 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. :-(
Duh, you just told me. Added, thanks.
* <= 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 :-)).
Of the size functions, I was able to remove only this one and keep all
of my pgTAP tests passing:
CREATE FUNCTION textlen(citext)
RETURNS int4 AS 'textlen'
LANGUAGE 'internal' IMMUTABLE STRICT;
When I deleted any of the others, I got errors like this:
psql:sql/citext.sql:865: ERROR: function length(citext) is not unique
LINE 1: SELECT is( length( name ), length(name::text), 'length("' ||...
^
HINT: Could not choose a best candidate function. You might need to
add explicit type casts.
I think you can see now why I wrote the tests: I wanted to ensure that
CITEXT really does work (almost) just like TEXT.
I was able to eliminate *all* of the miscellaneous functions, but the
upper() and lower() functions now return TEXT instead of CITEXT, which
I don't think is exactly what we want, is it? For now, I'e left
upper() and lower() in. It just seems like more expected functionality.
* 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…
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
