Re: bytea_ops

"Joe Conway" <joseph(dot)conway(at)home(dot)com> writes:
> I found that I had a need to create and use an index on bytea columns for a
> project I'm currently working on.

Looks pretty good. I have only three gripes, two trivial. In order of
decreasing trivialness:

+bytealt(PG_FUNCTION_ARGS)
+{
+ bytea *arg1 = PG_GETARG_BYTEA_P(0);
+ VarChar *arg2 = PG_GETARG_BYTEA_P(1);

Looks like you missed one VarChar -> bytea.

+ if ((cmp < 0) || ((cmp == 0) && (len1 < len2)))
+ PG_RETURN_BOOL(TRUE);
+ else
+ PG_RETURN_BOOL(FALSE);

I think it's better style to code stuff like this as

PG_RETURN_BOOL((cmp < 0) || ((cmp == 0) && (len1 < len2)));

BoolGetDatum already does the work of ensuring that the returned Datum
is exactly TRUE or FALSE, so why do it over again?

The biggie is that you missed adding support for bytea to scalarltsel,
which puts a severe crimp on the optimizer's ability to make any
intelligent decisions about using your index. See
src/backend/utils/adt/selfuncs.c, about line 580. You need to add a
case to the convert_to_scalar() function in that file. I'd say that
bytea should be a separate type category --- don't try to cram it into
convert_to_scalar's string category, because the string-handling code
assumes null-termination is safe. But the implementation should be
modeled on the handling of strings: you want to strip any common prefix,
then use the next few bytes as base-256 digits to form numeric values.
The comments for convert_string_to_scalar should give you an idea.

regards, tom lane