Re: Refactoring postmaster's code to cleanup after child exit

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(dot)linnakangas(at)iki(dot)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)