Feedback-ID: i0fe9450f:Fastmail
Date: Fri, 29 Nov 2024 13:42:03 +0900
From: Michael Paquier <michael@paquier.xyz>
To: "Andrey M. Borodin" <x4mmm@yandex-team.ru>
Cc: Bernd Helmle <mailings@oopsware.de>,
	jian he <jian.universality@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: [PATCH] Add sortsupport for range types and btree_gist
Message-ID: <Z0lGG9B1gSJXEwzh@paquier.xyz>
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>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="ljOm7NlFYhHeD3nM"
Content-Disposition: inline
In-Reply-To: <F6394698-D6BD-469A-BEC4-2D23D348A324@yandex-team.ru>
Archived-At: <https://www.postgresql.org/message-id/Z0lGG9B1gSJXEwzh%40paquier.xyz>
Precedence: bulk


--ljOm7NlFYhHeD3nM
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, Nov 28, 2024 at 11:32:59PM +0500, Andrey M. Borodin wrote:
> Michael, we have 30 tests with checks that need injection
> points. But these 30 tests also test functionality that needs to be
> tested even in build without injection points.=20
> Do we have to extract checks with injection point into separate
> regression test? So that we can exclude this test in builds without
> injection points.

I've looked at what the patch is doing with injection points, and
that's incorrect.

+CREATE EXTENSION injection_points;
+
+SELECT injection_points_attach('gist-sorted-build', 'notice');

This attaches points locally, meaning that this is going to create
some noise for any tests running in parallel taking the same code path
as where the point is created.  To make tests able to run safely in a
concurrent manner, please use injection_points_set_local().  As a
bonus, your points would be automatically detached when the session
turns off.

Any module that requires the module injection_points to be installed
can do a few things, none of them are done this way in this patch:
- Add EXTRA_INSTALL=3Dsrc/test/modules/injection_points.
- You could make a test able to run installcheck, but you should use
an extra query that checks if the module is available for installation
by querying pg_available_extensions and use two possible output files:
one for the case where the module is *not* installed and one for the
case where the module is installed.  A simpler way would be to block
installcheck, or add SQL tests in src/test/modules/injection_points.
Both options don't seem adapted to me here as they impact the
portability of existing tests.

As a whole, I'm very dubious about the need for injection points at
all here.  The sortsupport property claimed for this patch tells that
this results in smaller index sizes, but the tests don't really check
that: they just make sure that sortsupport routine paths are taken.
What this should test is not the path taken, but how the new code
affects the index data generated.  Perhaps pageinspect could be
something to use to show the difference in contents, not sure though.

The number of tests added to contrib/btree_gist/Makefile is not
acceptable for a patch of this size, leading to a large bloat.  And
that's harder to maintain in the long-term.
--
Michael

--ljOm7NlFYhHeD3nM
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmdJRhsACgkQnvQgOdby
QH1EaQ/+PIn7oCKYh63iYk4oPPobRoFXJgYfl+YqmwPQt9gTiZ4UgILpncHjaug2
eQgZHD3J38PhQBvVREFcjxrXBJ5LHinjg40BlPr1J9mhjsN5a+QDiruVdP4GTQKM
fikA9ImLRGHkPQb1aosV/oGLrUMugjjTI41n6lkT+H2P07v2KOIO6yk1G7RQK4sl
wg0mV9BZIwntLscn7K1UvB2eHkQdu83oQp5bRB++Ayqj96Bfc6yorGTcswSWsRVG
lcf/Yd5i4a56rmEqA4gXeyf33zSUxyvvIGeQoZFfDsO/O6duRIHBBKgzWIAMaSCf
Z+sJGAFERJG3Y9bJ+1R/X7ryyZwaiRoZygj+UCCwBZx6rHK/qDqCj9Yaf/lypYqT
plxoJfAvv1cGe8ZWdgKrc3RobaSysmVDLNUiJUEBjF8PpDc2LpGHclKNSzZ/cFmb
gxubEI04YR2ddku618x5d6AnBaZXqzwYy0BQQixmxRPW/DkhtCwj9TuvtdrXBF8v
CCCUdlPTxFBzAEZY9WQ32XTuFNFOd4O48pKk0L14F2au72Pyvsz76PXbGFRbIpF9
iYCcw+582EEY3o5KWpiZi8/46ZrmQHxZ4QNUg4xcCkRYRiOxfbvzUQ1wPjeK6UGs
NpGZqN+XYH2/6yn3bHBphawVkhTahl6byAJO0PzQuRiOKzV8vJI=
=KXeh
-----END PGP SIGNATURE-----

--ljOm7NlFYhHeD3nM--