Re: PATCH: CITEXT 2.0

On Jul 2, 2008, at 22:14, Tom Lane wrote:

> Note that this sort of stuff will mostly be fixed by pg_indent,
> whether or not David does anything about it. But certainly
> conforming to the project style to begin with will cause less
> pain to reviewers' eyeballs.

Yeah, I'll change it. I'm JAPH, so kind of made up the formatting as I
went, though I did try to copy the style in varlena.c.

>> +// PostgreSQL 8.2 Magic.
>> +#ifdef PG_MODULE_MAGIC
>> +PG_MODULE_MAGIC;
>> +#endif
>
> Here however is an outright bug: we do not allow // comments,
> because we
> still support ANSI-spec compilers that don't recognize them.

Forgot about that. I'll change it for the next version.

> btree cmp functions are allowed to return int32 negative, zero, or
> positive. There is *not* any requirement that negative or positive
> results be exactly -1 or +1. However, if you are comparing values
> that are int32 or wider then you can't just return their difference
> because it might overflow.

Thanks for the explanation. I'll make sure that they're both int32.

> The "leak" is irrelevant for larger/smaller. The only place where
> it's
> actually useful to do PG_FREE_IF_COPY is in a btree or hash index
> support function. In other cases you can assume that you're being
> called in a memory context that's too short-lived for it to matter.

So would that be for any function used by

CREATE OPERATOR CLASS citext_ops
DEFAULT FOR TYPE CITEXT USING btree AS
OPERATOR 1 < (citext, citext),
OPERATOR 2 <= (citext, citext),
OPERATOR 3 = (citext, citext),
OPERATOR 4 >= (citext, citext),
OPERATOR 5 > (citext, citext),
FUNCTION 1 citext_cmp(citext, citext);

? (And then the btree operator and function to be added, of course.)

>
>
>>> 5) There are several commented out lines in CREATE OPERATOR
>>> statement mostly related to NEGATOR. Is there some reason for that?
>
>> I copied it from the original citext.sql. Not sure what effect it
>> has.
>
> http://developer.postgresql.org/pgdocs/postgres/xoper-
> optimization.html

Thanks. Sounds like it'd be valuable to have them in there. I'll add
tests, as well.

Best,

David