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

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