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

Am Mittwoch, dem 10.01.2024 um 08:00 +0800 schrieb jian he:

> you do  `CREATE INDEX ON pgbench_accounts USING gist(aid);`
> but the original patch didn't change contrib/btree_gist/btree_int4.c
> So I doubt your benchmark is related to the original patch.
> or maybe I missed something.
>

The patch originally does this:

+ALTER OPERATOR FAMILY gist_int4_ops USING gist ADD
+ FUNCTION 11 (int4, int4) btint4sortsupport (internal) ;

This adds sortsupport function to int4 as well. We reuse existing
btint4sortsupport() function, so no need to change btree_int4.c.

> also per doc:
> `
> sortsupport
> Returns a comparator function to sort data in a way that preserves
> locality. It is used by CREATE INDEX and REINDEX commands. The
> quality
> of the created index depends on how well the sort order determined by
> the comparator function preserves locality of the inputs.
> `
> from the doc, add sortsupport function will only influence index
> build time?
>

Thats the point of this patch. Though it influences the index quality
in a way which seems to cause the measured performance regression
upthread.

>
> per https://www.postgresql.org/docs/current/gist-extensibility.html
> QUOTE:
> All the GiST support methods are normally called in short-lived
> memory
> contexts; that is, CurrentMemoryContext will get reset after each
> tuple is processed. It is therefore not very important to worry about
> pfree'ing everything you palloc. However, in some cases it's useful
> for a support method to
> ENDOF_QUOTE
>
> so removing the following part should be OK.
> + if ((Datum) range_a != a)
> + pfree(range_a);
> +
> + if ((Datum) range_b != b)
> + pfree(range_b);
>

Probably, i think we get a different range objects in case of
detoasting in this case.

> comparison solely on the lower bounds looks strange to me.
> if lower bound is the same, then compare upper bound, so the
> range_gist_cmp function is consistent with function range_compare.
> so following change:
>
> + else
> + result = range_cmp_bounds(typcache, &lower1, &lower2);
> to
> `
> else
> {
> result = range_cmp_bounds(typcache, &lower1, &lower2);
> if (result == 0)
> result = range_cmp_bounds(typcache, &upper1, &upper2);
> }
> `
>
> does contrib/btree_gist/btree_gist--1.7--1.8.sql function be declared
> as strict ? (I am not sure)
> other than that, the whole patch looks good.
>
>

That's something surely to consider.