Re: [PATCH] Add sortsupport for range types and btree_gist

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. 
> 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=src/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