Re: PATCH: CITEXT 2.0 v3 - Mailing list pgsql-hackers

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