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

On Fri, Feb 9, 2024 at 2:14 AM Bernd Helmle <mailings@oopsware.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?