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)