> 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
No comments:
Post a Comment