Message-ID: <4d983458445f220043b4a49a1baae9e299e69a79.camel@oopsware.de>
Subject: Re: [PATCH] Add sortsupport for range types and btree_gist
From: Bernd Helmle <mailings@oopsware.de>
To: jian he <jian.universality@gmail.com>, christoph.heiss@cybertec.at
Cc: pgsql-hackers@lists.postgresql.org, Christoph Berg
	 <christoph.berg@cybertec.at>
Date: Wed, 10 Jan 2024 17:35:37 +0100
In-Reply-To: <CACJufxHvXHNi_9roSo2gLJVCSep1EgD-z=-ey6K_Xg96PHs9+g@mail.gmail.com>
References: <437ccbcf-8f80-2919-411d-a3af88becf6c@cybertec.at>
	 <77f4bacbeb9d223f90b3a0bd55ef7d65407bfddd.camel@oopsware.de>
	 <CACJufxHvXHNi_9roSo2gLJVCSep1EgD-z=-ey6K_Xg96PHs9+g@mail.gmail.com>
Autocrypt: addr=mailings@oopsware.de; prefer-encrypt=mutual;
 keydata=mQGiBEIM4dYRBACEnxQZ/CfR1xbCJzGOIjvh3fZpLALMm+tt237QlZm9nlLaEwCn317sT1khPAJwhyBHSHbNFbH+LOL6ezYrg4V2Q9KHyYJDStJtC8c+D7vHVB5Jfslynx89DnCH6jb3H0j+U0iNYmRmT18PWTZhD5RuXT+Toyz/0hH4YnJzha4VUwCggikXRuhIQ0KIzrps7/PsvasTPjUD/1aoEHRwVyk2C2hXh6GWJYq+1l+sB2d0TOnFJf2xy3Xo03Hi+BEJJdYGqJKqzd0KfsFyJYhO2GIGScSEj1hsTqAiY8OZeRqRGHe69OQEguUSVXp/jPaqU8fel8dbDDMnXkgULq8oO4T9e4GJSFqIvrlx0sJqSMFdiihV+xCzDS40BACBrTq9tyXmyxJWzQ1/0Hx9S4sU+yeR8ztZH9xgWi1wksfHpEgZBdAkQynlI80/bCtgxfVu+LKgxeqGfMSnT9t8agpr1IGcmSbDntbn/K+3EmCze0jq8EQrZHJWiwCqraM8mnmILSEl5znaoVoBIF062b60zZicuNeYaVgEKSHfirQgQmVybmQgSGVsbWxlIDxiZXJuZEBvb3Bzd2FyZS5kZT6IXgQTEQIAHgUCQhyP2gIbAwYLCQgHAwIDFQIDAxYCAQIeAQIXgAAKCRBRdc2OhdlKzMm4AJ9mW1np2RXi66WzEvcIanaJkDT+xgCfZmXG/Sziz5FchI3OjXfXDZfwnNm0J0Jlcm5kIEhlbG1sZSA8YmVybmQuaGVsbWxlQG9vcHN3YXJlLmRlPoheBBMRAgAeBQJCDOHWAhsDBgsJCAcDAgMVAgMDFgIBAh4BAheAAAoJEFF1zY6F2UrMWZgAn3MVwDBzRS5dZu1yjx7WU3E3JSheAJ41CUYMIkX/oYfWssUVYnOuedgn8LQlQmVybmQgSGVsbWxlIDxiaEBzY2huYWVwcGNoZW5qYWdkLmRlPoheBBMRAgAeBQJCHI+pA
	hsDBgsJCAcDAgMVAgMDFgIBAh4BAheAAAoJEFF1zY6F2UrMYMUAn0rk8fEEY2tHGSUD7PNbk+Fwkd7MAJ4wMfOerrvsKTrB9xmFnZ2ZUy67w7QjQmVybmQgSGVsbWxlIDxtYWlsaW5nc0Bvb3Bzd2FyZS5kZT6IXgQTEQIAHgUCQhyPxgIbAwYLCQgHAwIDFQIDAxYCAQIeAQIXgAAKCRBRdc2OhdlKzOhrAJ4w+A3HcjTJb54lUDpyyQu6Y5dlVQCeJCFKygOVbpn4HSp4Z+R6+s4Rul60J0Jlcm5kIEhlbG1sZSA8YmVybmQuaGVsbWxlQGN5YmVydGVjLmF0Poh4BBMRAgA4FiEEcLVWcrwQIdBqHtvMUXXNjoXZSswFAmDe1DkCGwMFCwkIBwIGFQoJCAsCBBYCAwECHgECF4AACgkQUXXNjoXZSszs9QCfbbpKT0/Mh4PhfS6rRbmNNQC1zS8An2x1PLHPS6649DRkiYOMWTXzAlSEuQINBEIM4d0QCADaxWn10/2l8HHygHCUlBs+vNRYkMrGDWuj1tyehVRRcFYnNfBaEB44GMsaWdKW0GGP4S1BcEwLM3T+ObIrMt6jsLYXWrMctMcGkW60CTbusllwqFN80prqk0Nh/yZsja8UOt+OrWL4ucIBEx9RsUcdUd7+PeuYThOFO1dMgu4gmbac5eN2LZceHJgN9fha19m/vdLXmLc5IEPX/s6+O5oyEkbBt4GYIdvodnBHG9fyip7ff5GPLKzU2gBJz3RWUuHGEaj3RxCffK1x1S87MonbbEkPl2BtmJg2HeWNHn0eaauTp61V0xUWo1XaBB7Ezovogj+70wQjJ4GEgXoH7MRvAAMFCAC/MFpNfWALuQiKE3GA/VdoBTljq2rOnkDcRzBrn8BBVwpYqa2g7SA/VUW334ztI2PmZZYJ0Bjms/Q+oEv87agSL7sQTr8xUOPEyABVnAoDIuIzrAilflYYbvq7o3kwNcsmkr
	vPZ1FURstKbO+85M/e8tIyCHa6IbTKru6W8Rqxldx2TjoTvcOov3H7bknAdaA8dv6ea/dpI0WTVkuBTMw+jCy2gezwO1wvY+2ehiObOOnlKB2OsMshbe7FHtMU4qB6H1IthiSCzYz5IAsovS8u88HwtUVh/fjLHjTBB85IIMswvJ6SiCXfpe4z8Aj2b5hdo6ggZmI6opiBpaeK164SiEkEGBECAAkFAkIM4d0CGwwACgkQUXXNjoXZSszvIgCfeqX8AYMGEnF+rOp2ukoCWQXfq48AmQEAzB0c5vVMlRnuBh0z9qMKRqbi
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.2 (3.50.2-1.fc39) 
MIME-Version: 1.0
Archived-At: <https://www.postgresql.org/message-id/4d983458445f220043b4a49a1baae9e299e69a79.camel%40oopsware.de>
Precedence: bulk

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


> you do=C2=A0 `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.
>=20

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?
>=20

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

>=20
> 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
>=20
> 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);
>=20

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

> 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:
>=20
> + 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);
> }
> `
>=20
> 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.
>=20
>=20

That's something surely to consider.