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