Message-ID: <0176b173adfcff250543fe0aee2ef6137bd71953.camel@oopsware.de>
Subject: Re: [PATCH] Add sortsupport for range types and btree_gist
From: Bernd Helmle <mailings@oopsware.de>
To: "Andrey M. Borodin" <x4mmm@yandex-team.ru>
Cc: jian he <jian.universality@gmail.com>, PostgreSQL Hackers
	 <pgsql-hackers@lists.postgresql.org>
Date: Tue, 12 Nov 2024 16:43:50 +0100
In-Reply-To: <7095B5FA-FADB-4524-B858-ECD14998710B@yandex-team.ru>
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>
	 <2f2e88bf1d44d06c1baf27bbf3db880f42a5cb87.camel@oopsware.de>
	 <CACJufxHSsRk9egTjZWYXK4mJF8rsoV0XuRuw4uCxymoeeYBAJw@mail.gmail.com>
	 <31f23e46f71b744fb639967169d7df4c748110af.camel@oopsware.de>
	 <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>
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.1 (3.54.1-1.fc41) 
MIME-Version: 1.0
Archived-At: <https://www.postgresql.org/message-id/0176b173adfcff250543fe0aee2ef6137bd71953.camel%40oopsware.de>
Precedence: bulk

Am Montag, dem 11.11.2024 um 23:03 +0500 schrieb Andrey M. Borodin:
> Some nitpicking:
> 0. postgres % git apply ~/Downloads/v7.3-Add-GIST-sortsupport-*
> /Users/x4mmm/Downloads/v7.3-Add-GIST-sortsupport-btree-gist.patch:19:
> space before tab in indent.

I obviously shouldn't do patches after a long work day....


[...]

>=20
> 1. I'm mostly seening patches formatted with `git patch-format`
> rather than diffs as patches...
>=20

Will do.

> 2. Changes in contrib/btree_gist/btree_gist.c seem unnecessary.
>=20

Agreed. Will fix

> 3. Why do you move "typedef struct int32key" to btree_gist.h, but do
> not need to move all other keys?
>=20

Hmm, afair i did this back in an earlier stage of the patch and forgot
about it. It needs to be moved back to btree_int4.c, will fix.

> 4. These ifdedfs are not needed, just do INJECTION_POINT()
> #ifdef USE_INJECTION_POINTS
> INJECTION_POINT("btree-gist-sorted-build");
> #endif
>=20

I never worked with injection points before, i copied that pattern from
the docs somewhere, where those #ifdef's are used. Will fix.

> Also, there are 61 occurance of this code. Why not just 1 in
> gist_indexsortbuild() ?
>=20
>=20

Right, but after thinking about this i'd even go further: naming the
injection points for the sortsupport according to their datatype
they're called on. That would connect the regression test with the
specific datatype/sortsupport function and this would make sure the
correct sortsupport function for the specific test case is called.
Though that means that each test needs to attach/detach the injection
point itself...

What do you think?

Thanks,
	Bernd