Re: Yet another fast GiST build

On 07/04/2021 22:41, Andrey Borodin wrote:
> I see there is a problem with "SET client_min_messages = DEBUG1;" on
> buildfarm. I think these checks were necessary to make sure test
> paths are triggered. When we know that code paths are tested, maybe
> we can omit checks?
Yeah. We don't have very reliable coverage of different GiST build
methods, as noted earlier in this thread. But that's not this patch's fault.

I've been investigating the varbit issue, and realized that all the
comparison functions in this patch for varlen datatypes are broken. The
representation that the sortsupport function sees is the one that the
'compress' function returns. The fixed-length versions got this right,
but the varlen versions assume that the input is a Datum of the original
datatype. It happens to not crash, because the representation returned
by gbt_var_compress() is also a varlena, and all of the comparison
functions tolerate the garbage inputs, but it's bogus. The correct
pattern would be something like this (without the debugging NOTICE, of
course):

> static int
> gbt_text_sort_build_cmp(Datum a, Datum b, SortSupport ssup)
> {
> GBT_VARKEY_R ra = gbt_var_key_readable((GBT_VARKEY *) PG_DETOAST_DATUM(a));
> GBT_VARKEY_R rb = gbt_var_key_readable((GBT_VARKEY *) PG_DETOAST_DATUM(b));
>
> int x = DatumGetInt32(DirectFunctionCall2Coll(bttextcmp,
> ssup->ssup_collation,
> PointerGetDatum(a),
> PointerGetDatum(b)));
> elog(NOTICE, "cmp: %s vs %s: %d",
> TextDatumGetCString(ra.lower),
> TextDatumGetCString(rb.lower),
> x);
> return x;
> }
- Heikki