Message-ID: <b764bf2ababe2066128b5e8977e6db0990fa1fe9.camel@oopsware.de>
Subject: Re: [PATCH] Add sortsupport for range types and btree_gist
From: Bernd Helmle <mailings@oopsware.de>
To: Heikki Linnakangas <hlinnaka@iki.fi>, Andrey Borodin
 <x4mmm@yandex-team.ru>
Cc: Michael Paquier <michael@paquier.xyz>, jian he
 <jian.universality@gmail.com>,  PostgreSQL Hackers
 <pgsql-hackers@lists.postgresql.org>
Date: Thu, 03 Apr 2025 17:00:43 +0200
In-Reply-To: <2d3078fa-4700-431f-99a5-91ae8ee3bf86@iki.fi>
References: <a380cfd805c37c2aa70b1083d45820e1a7cbe759.camel@oopsware.de>
	 <7C9400DF-66FE-4308-BAD8-92FE3D0E55C6@yandex-team.ru>
	 <0d9fbd59de15e2fc987a850c3d1f8a79a6ede825.camel@oopsware.de>
	 <19c62e62c79b727d2c9a82eaeecfea5017001099.camel@oopsware.de>
	 <4BADF8DD-A4D3-43AA-AA08-037F6FFA40BC@yandex-team.ru>
	 <61128550659807032dd29c8c8a71674c818cbbb6.camel@oopsware.de>
	 <7095B5FA-FADB-4524-B858-ECD14998710B@yandex-team.ru>
	 <0176b173adfcff250543fe0aee2ef6137bd71953.camel@oopsware.de>
	 <bcdf583f38e7991a374c00ad77bb3adde24d8143.camel@oopsware.de>
	 <F6394698-D6BD-469A-BEC4-2D23D348A324@yandex-team.ru>
	 <Z0lGG9B1gSJXEwzh@paquier.xyz>
	 <78499920f1e9ed5895423e735272ca2fdb90baf6.camel@oopsware.de>
	 <770DB2DA-0B56-464A-87CE-5620CF12E114@yandex-team.ru>
	 <6bd891c9e3aa12baea5c938267bdb5a1e50a299e.camel@oopsware.de>
	 <64d324ce2a6d535d3f0f3baeeea7b25beff82ce4.camel@oopsware.de>
	 <a27a7d1e-7c24-4355-8e17-9ba5c61837ee@iki.fi>
	 <2d3078fa-4700-431f-99a5-91ae8ee3bf86@iki.fi>
Autocrypt: addr=mailings@oopsware.de; prefer-encrypt=mutual;
 keydata=mQGiBEIM4dYRBACEnxQZ/CfR1xbCJzGOIjvh3fZpLALMm+tt237QlZm9nlLaEwCn317sT
 1khPAJwhyBHSHbNFbH+LOL6ezYrg4V2Q9KHyYJDStJtC8c+D7vHVB5Jfslynx89DnCH6jb3H0j+U0
 iNYmRmT18PWTZhD5RuXT+Toyz/0hH4YnJzha4VUwCggikXRuhIQ0KIzrps7/PsvasTPjUD/1aoEHR
 wVyk2C2hXh6GWJYq+1l+sB2d0TOnFJf2xy3Xo03Hi+BEJJdYGqJKqzd0KfsFyJYhO2GIGScSEj1hs
 TqAiY8OZeRqRGHe69OQEguUSVXp/jPaqU8fel8dbDDMnXkgULq8oO4T9e4GJSFqIvrlx0sJqSMFdi
 ihV+xCzDS40BACBrTq9tyXmyxJWzQ1/0Hx9S4sU+yeR8ztZH9xgWi1wksfHpEgZBdAkQynlI80/bC
 tgxfVu+LKgxeqGfMSnT9t8agpr1IGcmSbDntbn/K+3EmCze0jq8EQrZHJWiwCqraM8mnmILSEl5zn
 aoVoBIF062b60zZicuNeYaVgEKSHfirQgQmVybmQgSGVsbWxlIDxiZXJuZEBvb3Bzd2FyZS5kZT6I
 XgQTEQIAHgUCQhyP2gIbAwYLCQgHAwIDFQIDAxYCAQIeAQIXgAAKCRBRdc2OhdlKzMm4AJ9mW1np2
 RXi66WzEvcIanaJkDT+xgCfZmXG/Sziz5FchI3OjXfXDZfwnNm0J0Jlcm5kIEhlbG1sZSA8YmVybm
 QuaGVsbWxlQG9vcHN3YXJlLmRlPoheBBMRAgAeBQJCDOHWAhsDBgsJCAcDAgMVAgMDFgIBAh4BAhe
 AAAoJEFF1zY6F2UrMWZgAn3MVwDBzRS5dZu1yjx7WU3E3JSheAJ41CUYMIkX/oYfWssUVYnOuedgn
 8LQlQmVybmQgSGVsbWxlIDxiaEBzY2huYWVwcGNoZW5qYWdkLmRlPoheBBMRAgAeBQJCHI+pAhsDB
 gsJCAcDAgMVAgMDFgIBAh4BAheAAAoJEFF1zY6F2UrMYMUAn0rk8fEEY2tHGSUD7PNbk+Fwkd7MAJ
 4wMfOerrvsKTrB9xmFnZ2ZUy67w7QjQmVybmQgSGVsbWxlIDxtYWlsaW5nc0Bvb3Bzd2FyZS5kZT6
 IXgQTEQIAHgUCQhyPxgIbAwYLCQgHAwIDFQIDAxYCAQIeAQIXgAAKCRBRdc2OhdlKzOhrAJ4w+A3H
 cjTJb54lUDpyyQu6Y5dlVQCeJCFKygOVbpn4HSp4Z+R6+s4Rul60J0Jlcm5kIEhlbG1sZSA8YmVyb
 mQuaGVsbWxlQGN5YmVydGVjLmF0Poh4BBMRAgA4FiEEcLVWcrwQIdBqHtvMUXXNjoXZSswFAmDe1D
 kCGwMFCwkIBwIGFQoJCAsCBBYCAwECHgECF4AACgkQUXXNjoXZSszs9QCfbbpKT0/Mh4PhfS6rRbm
 NNQC1zS8An2x1PLHPS6649DRkiYOMWTXzAlSE
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.54.3 (3.54.3-1.fc41) 
MIME-Version: 1.0
Archived-At: <https://www.postgresql.org/message-id/b764bf2ababe2066128b5e8977e6db0990fa1fe9.camel%40oopsware.de>
Precedence: bulk

Am Mittwoch, dem 02.04.2025 um 22:41 +0300 schrieb Heikki Linnakangas:
> * The stuff to save the FmgrInfo for gbt_enum_sortsupport() was
> broken.=20
> 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=20
> 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=20
> ADD VALUE. I fixed that, and modified the existing enum test to cover
> that case.
>=20

Ugh, i obviously didn't pay enough attention here.=20

> * The gist_bpchar_ops opfamily was using the built-in=20
> bpchar_sortsupport() function directly. That's not right, you need to
> have a shim that extracts and comparse just the 'lower' value, just
> like=20
> for all other datatypes. I think that was just a simple oversight,
> but=20
> it happened to pass the tests because bpchar_sortsupport() would not=20
> outright crash, even though we were passing it garbage. It's
> interesting=20
> that because the gist sortsupport function is used just to order the=20
> input during build, everything still works if it sorts the input to a
> completely bogus ordering, it just gets slower. At one point while=20
> fixing that, I also accidentally used "btcharcmp" instead of=20
> "bpcharcmp", and all the tests passed with that too.
>=20

Yes, i missed that. Originally the code did this all over the place
with many other types, too. I seem to have overseen this somehow.

> Those are now fixed. I also harmonized the comments to use the same=20
> phrasing for all the datatypes, marked all the sortsupport functions
> as=20
> PARALLEL SAFE, and reformatted slightly.

Note that the original coding was by Christoph Heiss and i picked up
his work and rewrote/fixed especially the part with the direct use of
sortsupport functions and many other issues spotted by testing and by
Andrey.

Many thanks for picking this up. There are users out there which are
going to be very happy with this if no further issues arise. :)