Обсуждение: make -C src/test/isolation failure in index-killtuples due to btree_gist

Hi all,
(CCing committer)

The following command fails, because btree_gist is not installed in
the context of the isolation tests:
make -C src/test/isolation/

This test has been added recently by 377b7ab14524.  Some efforts have
been done lately to remove any dependency to contrib/ in
src/test/regress/, like b1720fe63f34, and it does not strike me as a
good idea to begin doing that in the main isolation test suite, so
perhaps the best thing to do here is just move this test to
contrib/btree_gist/?  Rewriting this test without the dependency to
the contrib module seems tricky, and there's the matter of the test
correctness.

Regards,
--
Michael

Hi,

On 2025-08-18 08:57:13 +0900, Michael Paquier wrote:
> The following command fails, because btree_gist is not installed in
> the context of the isolation tests:
> make -C src/test/isolation/
> 
> This test has been added recently by 377b7ab14524.  Some efforts have
> been done lately to remove any dependency to contrib/ in
> src/test/regress/, like b1720fe63f34

I don't see what the point of that effort is. All it's doing is to make it
ever harder to write tests. IMO we should go diametrically the opposite way
and assume that we have fully built postgres when running tests.


> and it does not strike me as a good idea to begin doing that in the main
> isolation test suite, so perhaps the best thing to do here is just move this
> test to contrib/btree_gist/?

No, it makes absolutely no sense to test e.g. hash killtuples support in
btree_gist.


> and there's the matter of the test correctness.

What are you trying to say here?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2025-08-18 08:57:13 +0900, Michael Paquier wrote:
>> The following command fails, because btree_gist is not installed in
>> the context of the isolation tests:
>> make -C src/test/isolation/
>> ...
>> and it does not strike me as a good idea to begin doing that in the main
>> isolation test suite, so perhaps the best thing to do here is just move this
>> test to contrib/btree_gist/?

> No, it makes absolutely no sense to test e.g. hash killtuples support in
> btree_gist.

I think the complaint is that nothing has been done to ensure that
these modules have been installed.  You created a new dependency that
developers have to work around, rather than teaching the build system
to handle it.  As a comparison point, all of the tests in
src/test/recovery, src/test/authentication, etc take care to install
required modules when you say "make check" in those directories.
You broke that for src/test/isolation, and you should fix it.
It shouldn't be much harder than setting EXTRA_INSTALL in the
Makefile case; I dunno about meson.

            regards, tom lane

On Mon, Aug 18, 2025 at 11:38:02AM -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
>> No, it makes absolutely no sense to test e.g. hash killtuples support in
>> btree_gist.
>
> I think the complaint is that nothing has been done to ensure that
> these modules have been installed.  You created a new dependency that
> developers have to work around, rather than teaching the build system
> to handle it.  As a comparison point, all of the tests in
> src/test/recovery, src/test/authentication, etc take care to install
> required modules when you say "make check" in those directories.
> You broke that for src/test/isolation, and you should fix it.
> It shouldn't be much harder than setting EXTRA_INSTALL in the
> Makefile case; I dunno about meson.

All these test suites can rely on EXTRA_INSTALL because they rely on
the check/installcheck rules from Makefile.global, where checkprep is
pulled in.  src/test/isolation defines its own check rules, so we
would need a trick similar to what was done before b1720fe63f34.
--
Michael

On Mon, Aug 18, 2025 at 09:58:28AM -0400, Andres Freund wrote:
> On 2025-08-18 08:57:13 +0900, Michael Paquier wrote:
>> and it does not strike me as a good idea to begin doing that in the main
>> isolation test suite, so perhaps the best thing to do here is just move this
>> test to contrib/btree_gist/?
>
> No, it makes absolutely no sense to test e.g. hash killtuples support in
> btree_gist.

Okay, then let's make sure that the build rules install it
automatically if one runs only the isolation tests :)

We do that everywhere else for other test suites with EXTRA_INSTALL.
Being able to use make check is much more developer-friendly than
requiring developers to use installcheck + a custom step to make sure
that the module is installed in the instance running the tests.  make
check has worked for years when run independently in
src/test/isolation/, and I do quite a bit of concurrency testing
myself over time when working on a specific step of a larger
implementation.  I am pretty sure to not be the only one doing that.

> What are you trying to say here?

I've quickly looked at the possibility of *not* relying on btree_gist
in this test, and also quickly concluded that you have made this
choice to keep the test faster and easier to understand.  That's fine.
--
Michael

Hi,

On 2025-08-18 11:38:02 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2025-08-18 08:57:13 +0900, Michael Paquier wrote:
> >> The following command fails, because btree_gist is not installed in
> >> the context of the isolation tests:
> >> make -C src/test/isolation/
> >> ...
> >> and it does not strike me as a good idea to begin doing that in the main
> >> isolation test suite, so perhaps the best thing to do here is just move this
> >> test to contrib/btree_gist/?
> 
> > No, it makes absolutely no sense to test e.g. hash killtuples support in
> > btree_gist.
> 
> I think the complaint is that nothing has been done to ensure that
> these modules have been installed.

Yes, clearly that needs to be fixed. Not sure why I missed that. My point in
my above - perhaps to flippant - response was just that moving it to
btree_gist doesn't make sense.


> You created a new dependency that developers have to work around, rather
> than teaching the build system to handle it.  As a comparison point, all of
> the tests in src/test/recovery, src/test/authentication, etc take care to
> install required modules when you say "make check" in those directories.
> You broke that for src/test/isolation, and you should fix it.  It shouldn't
> be much harder than setting EXTRA_INSTALL in the Makefile case; I dunno
> about meson.

That would be the easiest fix - but I'm starting to wonder if it shouldn't
just be its own test module, as annoying as the boilerplate for that is.

While the test improved code coverage for the various indexes noticeably, I
did subsequently realize that the new test doesn't end up testing the recovery
path :(. Better than nothing, but having any coverage of those paths might be
worth the boilerplate and the runtime overhead of a test module :/


Greetings,

Andres Freund