MIME-Version: 1.0
References: <437ccbcf-8f80-2919-411d-a3af88becf6c@cybertec.at>
 <77f4bacbeb9d223f90b3a0bd55ef7d65407bfddd.camel@oopsware.de>
 <CACJufxHvXHNi_9roSo2gLJVCSep1EgD-z=-ey6K_Xg96PHs9+g@mail.gmail.com>
 <CACJufxEVYNCQJEnRr6jpdrNSb9hH6YkavxrLabrRuCWoeTDHjQ@mail.gmail.com>
In-Reply-To: 
 <CACJufxEVYNCQJEnRr6jpdrNSb9hH6YkavxrLabrRuCWoeTDHjQ@mail.gmail.com>
From: vignesh C <vignesh21@gmail.com>
Date: Fri, 26 Jan 2024 18:31:12 +0530
Message-ID: 
 <CALDaNm3J+CQ6KJQYNM66iuK3c3t3KCty17EoPySQeXNDy+unwA@mail.gmail.com>
Subject: Re: [PATCH] Add sortsupport for range types and btree_gist
To: jian he <jian.universality@gmail.com>
Cc: Bernd Helmle <mailings@oopsware.de>, pgsql-hackers@lists.postgresql.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CALDaNm3J%2BCQ6KJQYNM66iuK3c3t3KCty17EoPySQeXNDy%2BunwA%40mail.gmail.com>
Precedence: bulk

On Wed, 10 Jan 2024 at 19:49, jian he <jian.universality@gmail.com> wrote:
>
> On Wed, Jan 10, 2024 at 8:00=E2=80=AFAM jian he <jian.universality@gmail.=
com> wrote:
> >
> > `
> > from the doc, add sortsupport function will only influence index build =
time?
> >
> > +/*
> > + * GiST sortsupport comparator for ranges.
> > + *
> > + * Operates solely on the lower bounds of the ranges, comparing them u=
sing
> > + * range_cmp_bounds().
> > + * Empty ranges are sorted before non-empty ones.
> > + */
> > +static int
> > +range_gist_cmp(Datum a, Datum b, SortSupport ssup)
> > +{
> > + RangeType *range_a =3D DatumGetRangeTypeP(a);
> > + RangeType *range_b =3D DatumGetRangeTypeP(b);
> > + TypeCacheEntry *typcache =3D ssup->ssup_extra;
> > + RangeBound lower1,
> > + lower2;
> > + RangeBound upper1,
> > + upper2;
> > + bool empty1,
> > + empty2;
> > + int result;
> > +
> > + if (typcache =3D=3D NULL) {
> > + Assert(RangeTypeGetOid(range_a) =3D=3D RangeTypeGetOid(range_b));
> > + typcache =3D lookup_type_cache(RangeTypeGetOid(range_a), TYPECACHE_RA=
NGE_INFO);
> > +
> > + /*
> > + * Cache the range info between calls to avoid having to call
> > + * lookup_type_cache() for each comparison.
> > + */
> > + ssup->ssup_extra =3D typcache;
> > + }
> > +
> > + range_deserialize(typcache, range_a, &lower1, &upper1, &empty1);
> > + range_deserialize(typcache, range_b, &lower2, &upper2, &empty2);
> > +
> > + /* For b-tree use, empty ranges sort before all else */
> > + if (empty1 && empty2)
> > + result =3D 0;
> > + else if (empty1)
> > + result =3D -1;
> > + else if (empty2)
> > + result =3D 1;
> > + else
> > + result =3D range_cmp_bounds(typcache, &lower1, &lower2);
> > +
> > + if ((Datum) range_a !=3D a)
> > + pfree(range_a);
> > +
> > + if ((Datum) range_b !=3D b)
> > + pfree(range_b);
> > +
> > + return result;
> > +}
> >
> > 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 !=3D a)
> > + pfree(range_a);
> > +
> > + if ((Datum) range_b !=3D b)
> > + pfree(range_b);
> >
> > 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 =3D range_cmp_bounds(typcache, &lower1, &lower2);
> > to
> > `
> > else
> > {
> > result =3D range_cmp_bounds(typcache, &lower1, &lower2);
> > if (result =3D=3D 0)
> > result =3D 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.
>
> the original author email address (christoph.heiss@cybertec.at)
> Address not found.
> so I don't include it.
>
> 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.

CFBot shows that the patch does not apply anymore as in [1]:
=3D=3D=3D Applying patches on top of PostgreSQL commit ID
7014c9a4bba2d1b67d60687afb5b2091c1d07f73 =3D=3D=3D
=3D=3D=3D applying patch
./v5-0001-Add-GIST-sortsupport-function-for-all-the-btree-g.patch
patching file contrib/btree_gist/Makefile
Hunk #1 FAILED at 33.
1 out of 1 hunk FAILED -- saving rejects to file contrib/btree_gist/Makefil=
e.rej
...
The next patch would create the file
contrib/btree_gist/btree_gist--1.7--1.8.sql,
which already exists!  Applying it anyway.
patching file contrib/btree_gist/btree_gist--1.7--1.8.sql
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file
contrib/btree_gist/btree_gist--1.7--1.8.sql.rej
patching file contrib/btree_gist/btree_gist.control
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file
contrib/btree_gist/btree_gist.control.rej
...
patching file contrib/btree_gist/meson.build
Hunk #1 FAILED at 50.
1 out of 1 hunk FAILED -- saving rejects to file
contrib/btree_gist/meson.build.rej

Please post an updated version for the same.

[1] - http://cfbot.cputube.org/patch_46_3686.log

Regards,
Vignesh