Обсуждение: 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

On Fri, Aug 22, 2025 at 07:46:43PM -0400, Andres Freund wrote:
> 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 :/

Adding just an EXTRA_INSTALL to isolation's Makefile would not work,
no?  This Makefile has its own rules, which implies that fixing this
issue would be to duplicate what EXTRA_INSTALL does if we don't use
the test module solution.  How about a new test/modules/gist/ then?
--
Michael

Hi,

On 2025-10-28 16:55:49 +0900, Michael Paquier wrote:
> On Fri, Aug 22, 2025 at 07:46:43PM -0400, Andres Freund wrote:
> > 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 :/
> 
> Adding just an EXTRA_INSTALL to isolation's Makefile would not work,
> no?

How precisely to get the contrib module installed seems like a boring
detail. It's not hard to do that, if we decide that that's the way to go, even
without EXTRA_INSTALL.


> How about a new test/modules/gist/ then?

Well, it tests multiple index types, not just gist. I guess
test/modules/indexes/ or so should work? Not sure about plural or not.

Greetings,

Andres Freund

On Thu, Oct 30, 2025 at 11:24:55AM -0400, Andres Freund wrote:
> Well, it tests multiple index types, not just gist. I guess
> test/modules/indexes/ or so should work? Not sure about plural or not.

test/modules/index/ would work here.  If you feel strongly about being
able to do an EXTRA_INSTALL in src/test/isolation/, I have no
objections to that as that would also solve the original problem if
that's the consensus.  My personal choice would be a test module for
that, as it feels cleaner.
--
Michael

Hi,

On Fri, 31 Oct 2025 at 03:42, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Oct 30, 2025 at 11:24:55AM -0400, Andres Freund wrote:
> > Well, it tests multiple index types, not just gist. I guess
> > test/modules/indexes/ or so should work? Not sure about plural or not.
>
> test/modules/index/ would work here.  If you feel strongly about being
> able to do an EXTRA_INSTALL in src/test/isolation/, I have no
> objections to that as that would also solve the original problem if
> that's the consensus.  My personal choice would be a test module for
> that, as it feels cleaner.

I have two patches to implement this: one moving the killtuples test
under test/modules/index/, and another adding coverage for the
recovery path.

0001 moves killtuples test under the test/modules/index/ without any
implementation change.

0002 converts the killtuples isolation test to a TAP test to exercise
the recovery path. This patch sets up a standby and additionally
re-inserts into the table while testing the GiST index to ensure
coverage of the gistRedoDeleteRecord() function.

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

On Mon, Nov 17, 2025 at 06:45:30PM +0300, Nazir Bilal Yavuz wrote:
> I have two patches to implement this: one moving the killtuples test
> under test/modules/index/, and another adding coverage for the
> recovery path.
>
> 0001 moves killtuples test under the test/modules/index/ without any
> implementation change.
>
> 0002 converts the killtuples isolation test to a TAP test to exercise
> the recovery path. This patch sets up a standby and additionally
> re-inserts into the table while testing the GiST index to ensure
> coverage of the gistRedoDeleteRecord() function.

Hmm.  We should try to conclude over the benefit of TAP over
isolation, and merge both patches together if the consensus is towards
a TAP test.

The isolation test feels much cleaner to me in terms of clarity and
debugging output compared to the suggested TAP test as there are many
SQL sequences the test depends on.  Other opinions?
--
Michael

On Tue, Nov 18, 2025 at 08:31:41AM +0900, Michael Paquier wrote:
> Hmm.  We should try to conclude over the benefit of TAP over
> isolation, and merge both patches together if the consensus is towards
> a TAP test.
>
> The isolation test feels much cleaner to me in terms of clarity and
> debugging output compared to the suggested TAP test as there are many
> SQL sequences the test depends on.  Other opinions?

By the way, an extra argument in favor of an isolation test here: the
proposed TAP tests only wants to make sure that replay is able to
finish on a standby, we don't query it.  We are already doing the same
in 027_stream_regress.pl for the main regression test suite, and we
could expand this TAP test or add a new one that grabs the isolation
test suite and runs it in a TAP test with a standby plugged.  Hence we
would automatically test the replay path for all isolation tests added
to the tree, including this one related to kill tuples.
--
Michael

Hi,

On 2025-11-18 08:40:46 +0900, Michael Paquier wrote:
> On Tue, Nov 18, 2025 at 08:31:41AM +0900, Michael Paquier wrote:
> > Hmm.  We should try to conclude over the benefit of TAP over
> > isolation, and merge both patches together if the consensus is towards
> > a TAP test.
> > 
> > The isolation test feels much cleaner to me in terms of clarity and
> > debugging output compared to the suggested TAP test as there are many
> > SQL sequences the test depends on.  Other opinions?
> 
> By the way, an extra argument in favor of an isolation test here: the
> proposed TAP tests only wants to make sure that replay is able to
> finish on a standby, we don't query it.

I think we should eventually extend the test to run amcheck etc on both
primary and standby.


> We are already doing the same in 027_stream_regress.pl for the main
> regression test suite, and we could expand this TAP test or add a new one
> that grabs the isolation test suite and runs it in a TAP test with a standby
> plugged.

I don't think we should add new things to 027_stream_regress, it's already one
of the two slowest tests and, at least for me, often the test that takes the
longest to complete in a testrun.

Greetings,

Andres Freund

On Tue, Nov 18, 2025 at 09:00:39AM -0500, Andres Freund wrote:
> On 2025-11-18 08:40:46 +0900, Michael Paquier wrote:
>> By the way, an extra argument in favor of an isolation test here: the
>> proposed TAP tests only wants to make sure that replay is able to
>> finish on a standby, we don't query it.
>
> I think we should eventually extend the test to run amcheck etc on both
> primary and standby.

It's also something that I was wondering we could do after a main
regression test run for some subset of objects.  Not sure that it has
to be part of one test.

>> We are already doing the same in 027_stream_regress.pl for the main
>> regression test suite, and we could expand this TAP test or add a new one
>> that grabs the isolation test suite and runs it in a TAP test with a standby
>> plugged.
>
> I don't think we should add new things to 027_stream_regress, it's already one
> of the two slowest tests and, at least for me, often the test that takes the
> longest to complete in a testrun.

Of course, agreed.  That's the part about adding a completely new TAP
test that's able to run one or more isolation test suites, and this
isolation test would be part of it when running things across a
standby and a primary.  You're the author of the original test, so
your opinion takes priority over mine, for sure.
--
Michael

On Wed, Nov 19, 2025 at 11:30:30AM +0900, Michael Paquier wrote:
> It's also something that I was wondering we could do after a main
> regression test run for some subset of objects.  Not sure that it has
> to be part of one test.

FWIW, I still getting annoyed by this one (am I the only one?), so I
have applied the patch moving the isolation test.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> FWIW, I still getting annoyed by this one (am I the only one?), so I
> have applied the patch moving the isolation test.

No, you weren't the only one --- it was in my way just yesterday
while trying to update expected results for 572c40ba9.  I'd not paid
attention to this thread, but the committed solution seems fine.

            regards, tom lane