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

On Fri, Feb 9, 2024 at 2:14 AM Bernd Helmle <mailings(at)oopsware(dot)de> wrote:
>
> Am Mittwoch, dem 10.01.2024 um 22:18 +0800 schrieb jian he:
> >
> > I split the original author's patch into 2.
> > 1. Add GiST sortsupport function for all the btree-gist module data
> > types except anyrange data type (which actually does not in this
> > module)
> > 2. Add GiST sortsupport function for anyrange data type.
> >
>
> > what I am confused:
> > In fmgr.h
> >
> > /*
> > * Support for cleaning up detoasted copies of inputs. This must
> > only
> > * be used for pass-by-ref datatypes, and normally would only be used
> > * for toastable types. If the given pointer is different from the
> > * original argument, assume it's a palloc'd detoasted copy, and
> > pfree it.
> > * NOTE: most functions on toastable types do not have to worry about
> > this,
> > * but we currently require that support functions for indexes not
> > leak
> > * memory.
> > */
> > #define PG_FREE_IF_COPY(ptr,n) \
> > do { \
> > if ((Pointer) (ptr) != PG_GETARG_POINTER(n)) \
> > pfree(ptr); \
> > } while (0)
> >
> > but the doc
> > (https://www.postgresql.org/docs/current/gist-extensibility.html)
> > says:
> > 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.
> > ENDOF_QUOTE
> >
> > so I am not sure in range_gist_cmp, we need the following:
> > `
> > if ((Datum) range_a != a)
> > pfree(range_a);
> > if ((Datum) range_b != b)
> > pfree(range_b);
> > `
>
> Turns out this is not true for sortsupport: the comparison function is
> called for each tuple during sorting, which will leak the detoasted
> (and probably copied datum) in the sort memory context. See the same
> for e.g. numeric and text, which i needed to change to pass the key
> values correctly to the text_cmp() or numeric_cmp() function in these
> cases.
>

+ <para>
+ Per default <filename>btree_gist</filename> builts
<acronym>GiST</acronym> indexe with
+ <function>sortsupport</function> in <firstterm>sorted</firstterm>
mode. This usually results in a
+ much better index quality and smaller index sizes by 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 believe `built` |`builts` should be `build`.
Also
maybe we can simply copy some texts from
https://www.postgresql.org/docs/current/gist-implementation.html.
how about the following:
<para>
The sorted method is only available if each of the opclasses used by the
index provides a <function>sortsupport</function> function, as described
in <xref linkend="gist-extensibility"/>. If they do, this method is
usually the best, so it is used by default.
It is still possible to change to a buffered build strategy by using
the <literal>buffering</literal> parameter
to the CREATE INDEX command.
</para>

you've changed contrib/btree_gist/meson.build, seems we also need to
change contrib/btree_gist/Makefile

gist_point_sortsupport have `if (ssup->abbreviate)`, does
range_gist_sortsupport also this part?
I think the `if(ssup->abbreviate)` part is optional?
Can we add some comments on it?