Re: Refactoring postmaster's code to cleanup after child exit - Mailing list pgsql-hackers

On 04/09/2024 17:35, Andres Freund wrote:
> On 2024-08-12 12:55:00 +0300, Heikki Linnakangas wrote:
>> +Running the tests
>> +=================
>> +
>> +NOTE: You must have given the --enable-tap-tests argument to configure.
>> +
>> +Run
>> +    make check
>> +or
>> +    make installcheck
>> +You can use "make installcheck" if you previously did "make install".
>> +In that case, the code in the installation tree is tested.  With
>> +"make check", a temporary installation tree is built from the current
>> +sources and then tested.
>> +
>> +Either way, this test initializes, starts, and stops a test Postgres
>> +cluster.
>> +
>> +See src/test/perl/README for more info about running these tests.
> 
> Is it really useful to have such instructions all over the tree?

That's debatable but I didn't want to go down that rabbit hole with this 
patch.

It's repetitive for sure. But there are small variations in which 
PG_TEST_EXTRA options you need, whether "make installcheck" runs against 
a running server or still creates a temporary cluster, etc.

I tried to deduplicate those instructions by moving the above 
boilerplate to src/test/README, and only noting the variations in the 
subdirectory READMEs. I didn't like the result. It's very helpful to 
have full copy-pasteable commands with all the right "PG_TEST_EXTRA" 
options for each test.

These instructions also don't mention how to run the tests with Meson. 
The first time I wanted to run individual tests with Meson, it took me a 
while to figure it out.

I'll think a little more about how to improve these READMEs, but let's 
take that to a separate thread.

>>  From 93b9e9b6e072f63af9009e0d66ab6d0d62ea8c15 Mon Sep 17 00:00:00 2001
>> From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
>> Date: Mon, 12 Aug 2024 10:55:11 +0300
>> Subject: [PATCH v4 2/8] Add test for dead-end backends
>>
>> The code path for launching a dead-end backend because we're out of
>> slots was not covered by any tests, so add one. (Some tests did hit
>> the case of launching a dead-end backend because the server is still
>> starting up, though, so the gap in our test coverage wasn't as big as
>> it sounds.)
>> ---
>>   src/test/perl/PostgreSQL/Test/Cluster.pm      | 39 +++++++++++++++++++
>>   .../postmaster/t/001_connection_limits.pl     | 17 +++++++-
>>   2 files changed, 55 insertions(+), 1 deletion(-)
> 
> Why does this need to use "raw" connections?  Can't you just create a bunch of
> connections with BackgroundPsql?

No, these need to be connections that haven't sent the startup packet 
the yet.

With Andrew's PqFFI work [1], we could do better. The latest version on 
that thread doesn't expose the async functions like PQconnectStart() 
PQconnectPoll() though, but they can be added.

[1] 
https://www.postgresql.org/message-id/97d1d1b9-d147-f69d-1991-d8794efed41c%40dunslane.net


Unless you have comments on these first two patches which just add 
tests, I'll commit them shortly. Still processing the rest of your 
comments...

-- 
Heikki Linnakangas
Neon (https://neon.tech)