> 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.
Ouch! You are of course correct.
>
> + 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)));
OK
>
> 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.
I thought I must have missed something here. I noticed that with ~40000 rows
inserted (and the table vacuumed), explain on a < or > query showed the
optimizer tended to not use the index unless severely restricted, i.e.
select * from foobar where f1 < '\\000\\015';
would do a table scan while
select * from foobar where f1 < '\\000\\011';
would use the index.
Hopefully fixing the above will improve this. Thank you for looking at this
so quickly :-)
I'll take another pass and send in a new patch soon.
-- Joe
