From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | "David E(dot) Wheeler" <david(at)kineticode(dot)com> |
Cc: | pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: PATCH: CITEXT 2.0 v3 |
Date: | 2008-07-12 19:17:59 |
Message-ID: | 23294.1215890279@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
BTW, I looked at the SQL file (citext.sql.in) a bit. Some comments:
* An explicit comment explaining that you're piggybacking on the I/O
functions (and some others) for "text" wouldn't be out of place.
* 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.
* 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.
* The type declaration needs to say storage = extended, else the type
won't be toastable.
* The cast from bpchar to citext cannot be WITHOUT FUNCTION;
it needs to invoke rtrim1. Compare the bpchar to text cast.
* <= is surely not its own commutator. 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.)
* 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.
* Don't use the OPERATOR() notation when you don't need to.
It's just clutter.
regards, tom lane
From | Date | Subject | |
---|---|---|---|
Next Message | Jaime Casanova | 2008-07-12 19:32:03 | Re: Extending grant insert on tables to sequences |
Previous Message | Tom Lane | 2008-07-12 17:32:13 | Re: VACUUM Improvements - WIP Patch |