Re: [PATCH] Add sortsupport for range types and btree_gist

On 02/04/2025 20:18, Heikki Linnakangas wrote:
> So I added it for the btree opfamily too, and moved the function to
> rangetypes.c since it's not just for gist anymore. I Ccmmitted that
> part, and will start looking more closely at the remaining btree_gist
> parts now.

Here's an updated version of the remaining parts. Found a couple of bugs:

* The stuff to save the FmgrInfo for gbt_enum_sortsupport() was broken.
It saved the address of FmgrInfo struct that was allocated in a local
variable, on the stack, in SortSupport->ssup-extra, and expected it to
be valid on subsequent call to gbt_enum_ssup_cmp(). It went unnoticed
because enum_cmp() only accesses the FmgrInfo struct if it encounters
odd enum values, i.e. enum values that have been added with ALTER TYPE
ADD VALUE. I fixed that, and modified the existing enum test to cover
that case.

* The gist_bpchar_ops opfamily was using the built-in
bpchar_sortsupport() function directly. That's not right, you need to
have a shim that extracts and comparse just the 'lower' value, just like
for all other datatypes. I think that was just a simple oversight, but
it happened to pass the tests because bpchar_sortsupport() would not
outright crash, even though we were passing it garbage. It's interesting
that because the gist sortsupport function is used just to order the
input during build, everything still works if it sorts the input to a
completely bogus ordering, it just gets slower. At one point while
fixing that, I also accidentally used "btcharcmp" instead of
"bpcharcmp", and all the tests passed with that too.

Those are now fixed. I also harmonized the comments to use the same
phrasing for all the datatypes, marked all the sortsupport functions as
PARALLEL SAFE, and reformatted slightly.

> + <para>
> + By default <filename>btree_gist</filename> builds <acronym>GiST</acronym> index with
> + <function>sortsupport</function> in <firstterm>sorted</firstterm> mode. This usually results in
> + much faster index built speed. It is still possible to revert to buffered built strategy
> + by using the <literal>buffering</literal> parameter when creating the index.
> + </para>

I'm inclined to leave out this paragraph. That's not specific to the
btree_gist module, you can make any GiST index slower by disabling the
sorted mode; but why would you do that? Let's mention in the release
notes that btree_gist index builds got faster, and leave it at that.

--
Heikki Linnakangas
Neon (https://neon.tech)