injection_points: canceled or terminated waiters leak their wait slots

On Tue, Jul 21, 2026 at 09:28:22AM +0100, Zsolt Parragi wrote:

While stress-testing REPACK CONCURRENTLY on 19beta2 I saw a logical
decoding activation race, and I extended 051_effective_wal_level.pl
with a test that waits on an injection point and wakes it up later.
051 cancels two injection point waiters earlier in the script, and
injection_wait() never cleans up after a canceled waiter, the wakeup
never arrived and the test deadlocked.

I think we are missing an ENSURE_ERROR_CLEANUP block there. See
attached patch with a testcase reproducing the issue.

Hmm. I think that I'd rather use a PG_TRY/PG_FINALLY and avoid the
refactoring with the extra routine required, keeping the cleanup
action local to injection_wait(). That's also because the cleanup
action is the same for both the "normal" exit path and the interrupt
path.

I also attached a separate version for pg19, as master has a
refactored version of injection_wait. All previous branches have the
19 version, it should be easy to backport to other branches.

Thanks for that. I'm always OK to deal with a backpatch as required.
Posting versions saves some time, of course, just don't feel obliged
if you feel that this is extra work on your side.

On an unpatched code, the test would hang due to the fact that we are
doing a wait but we should not because the slot was not cleaned up.
It means that a failure mode equals to a timeout. Why not, we have
other tests of this class. Another thought: the addition of a SQL
function that provides the list of waiters that we reuse here. I
don't really see why this is worth the cost compared to your test, but
opinions of others are welcome, of course.
--
Michael

I think that I'd rather use a PG_TRY/PG_FINALLY and avoid the
refactoring with the extra routine required, keeping the cleanup
action local to injection_wait()

Wouldn't that miss FATAL? (pg_terminate_backend in the testcase)

Thanks for that. I'm always OK to deal with a backpatch as required.
Posting versions saves some time, of course, just don't feel obliged
if you feel that this is extra work on your side.

I don't always consistently do this (sometimes it's interesting to check that its different in back branches, sometimes I completely forgot about it)

In this case, I started the testing / debugging on pg19, and only checked that the code is different on master after I already had the fix. Verifying that the code is the same on earlier branches wasn't much extra work after that.

Another thought: the addition of a SQL
function that provides the list of waiters that we reuse here.

I can implement that if you think it is better, my initial logic was to keep the fix smaller and strictly a bugfix.

On Tue, Jul 21, 2026 at 11:07:01PM -0700, Zsolt Parragi wrote:

Wouldn't that miss FATAL? (pg_terminate_backend in the testcase)

Arf. I've forgotten that elog.h documents that. Thanks.

I can implement that if you think it is better, my initial logic was
to keep the fix smaller and strictly a bugfix.

Minimal sounds good here. I'll make something happen after
double-checking what you have sent.
--
Michael

On Wed, Jul 22, 2026 at 03:28:56PM +0900, Michael Paquier wrote:

Minimal sounds good here. I'll make something happen after
double-checking what you have sent.

And now done down to v17, as of a49b6a610946.
--
Michael

On 23 Jul 2026, at 08:40, Michael Paquier <michael@paquier.xyz> wrote:

done

Hi,

While running CI for an unrelated patch, I saw wait_cleanup fail in the
Windows Visual Studio job[0]https://github.com/x4m/postgres_g/actions/runs/32582921356/job/97055012805.

The server log contains the expected FATAL, but isolationtester only saw:

PQconsumeInput failed: server closed the connection unexpectedly

It then exited without running the rest of the permutation or teardown, so
heap_lock_update failed afterwards because the injection_points extension
still existed.

This seems to be another instance of the known Windows behavior where the
last server message can be lost when a connection is closed [1]/messages/by-id/CA+hUKGLR10ZqRCvdoRrkQusq75wF5=vEetRSs2_u1s+FAUosFQ@mail.gmail.com. The test
added in a49b6a61094 intentionally terminates an isolationtester connection,
so it is exposed to that behavior.

The attached patch makes isolationtester treat PQconsumeInput() failure with
CONNECTION_BAD as completion of the step. It reports any complete server
error already buffered by libpq, followed by the saved connection error, and
the rest of the test and teardown can run. Other PQconsumeInput() failures
remain fatal. An alternative expected file covers the case where Windows
loses the server's FATAL and only the libpq-generated connection error
remains.

The alternative output is synthetic. I tested it by temporarily suppressing
the final ErrorResponse while leaving backend termination unchanged. The
output then matched wait_cleanup_1.out. With normal error delivery it matched
wait_cleanup.out. If anyone knows a way to reproduce the actual Windows
message loss on demand, that would be useful. Otherwise, the next occurrence
in CI with this patch applied will give us an output to compare with the
alternative file.

The injection_points isolation tests pass through Windows CI.

If I have misdiagnosed the cause of this CI failure, apologies for the noise.

Best regards, Andrey Borodin.

[0]: https://github.com/x4m/postgres_g/actions/runs/32582921356/job/97055012805
[1]: /messages/by-id/CA+hUKGLR10ZqRCvdoRrkQusq75wF5=vEetRSs2_u1s+FAUosFQ@mail.gmail.com

On Sun, Aug 23, 2026, Andrey Borodin <x4mmm@yandex-team.ru> wrote:

The attached patch makes isolationtester treat PQconsumeInput() failure with
CONNECTION_BAD as completion of the step.

Thanks Andrey. CF 6992 hit the same symptom in slot_creation_error on
Windows. Attached are three follow-ups on top of your v1.

There is another case with a notice blocker: try_complete_step() can return
while the step is still blocked, losing the saved connection error. The next
attempt then fails with "invalid socket". The fix keeps the error in
IsoConnInfo until the step completes. The other patches add the alternate
slot_creation_error output and extend wait_cleanup to cover this case.

With the extended test and the final ErrorResponse suppressed, v1 fails
before release3/detach3; the fixed version completes both. The suppression
is test-only and leaves the server's FATAL in the log. It does not force
Windows transport loss.

The Windows run below passed core regression/isolation, test_decoding,
injection_points isolation and subscription TAP (one ICU-dependent skip),
with your v1 plus these patches. The workflow includes setup, the missing
ErrorResponse test and old-driver controls:

https://github.com/NikolayS/postgres/actions/runs/35770093308
https://github.com/NikolayS/postgres/blob/8039ee47313f7ac860d26bd6fd9892c0ac296fee/.github/workflows/cf6992-windows-validation.yml

AI found the blocker case and prepared these follow-up patches.
I have not manually reviewed these patches.

Nik

Thanks for the patches!

I was able to reproduce the issue both on windows, and on linux by "patching" the server to reproduce the described behavior.

The overall changes look good to me, it just needs some squashing/organizing to be a proper patchset.

On Mon, Sep 28, 2026 at 10:12:37PM +0000, Zsolt Parragi wrote:

I was able to reproduce the issue both on windows, and on linux by
"patching" the server to reproduce the described behavior.

How exactly?

The overall changes look good to me, it just needs some
squashing/organizing to be a proper patchset.

TBH, I am not completely sure what you are proposing here. v1-0001
from Andrey and AI-generated-not-reviewed 0003 from Nikolay step on
each other with the latter patch requiring the former patch, in terms
of the way the last buffered error messages can be consumed, which
should be either something local to try_complete_step() or tracked by
IsoConnInfo. I'm OK with the basic idea of having more predictible
output here, with some alternate outputs to make the CI happier on
Windows, let's just organize a bit the whole..

-       SELECT pg_terminate_backend(pid) FROM pg_stat_activity
+       SELECT pg_terminate_backend(pid, 180000) FROM pg_stat_activity

Avoiding hardcoded timeouts would be nice. They are not liked on slow
machines.
--
Michael

How exactly?

If we assume the linked thread about windows skipping the last message
is correct, we can simulate that by adding an option not to send the
fatal message to the client:

diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index b9d2c96b97a..e043717b943 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -1924,7 +1924,8 @@ EmitErrorReport(void)
                send_message_to_server_log(edata);

        /* Send to client, if enabled */
-       if (edata->output_to_client)
+       if (edata->output_to_client &&
+               !(edata->elevel == FATAL && getenv("PG_TEST_DROP_FATAL")))
                send_message_to_frontend(edata);

MemoryContextSwitchTo(oldcontext);

On windows, it seems it can randomly happen, but it's very unlikely

TBH, I am not completely sure what you are proposing here. v1-0001
from Andrey and AI-generated-not-reviewed 0003 from Nikolay step on
each other

0001+0002 is enough to fix the existing reports. I think
temp-schema-cleanup could also use an alternative output, as it uses
the same construction and fails with the above modification.

0003+0004 seems to fix a blocking issue Nikolay mentioned, but I don't
think that's currently required by any of the existing test cases, it
seems more like an AI-discovered scenario.

One thing I realized after sending that message is that if we accept
the limitation in this comment:

+ /*
+ * Save the error before PQgetResult() adds another complaint about
+ * attempting to read from the dead socket.
+ */
+ connection_error = pg_strdup(PQerrorMessage(conn));

Which means an additional "invalid socket" message in the output,
0001+0003 together becomes 3 simple condition changes. See the
attached patch, the only difference is that there's one more extra
line in the alternative test outputs, but now the code change is
simpler, so this might be actually better. (I also validated Nikolay's
0004 test case with this, but I didn't include it in the patch)

This still need actual windows testing, I didn't do that part yet.

On windows, it seems it can randomly happen, but it's very unlikely

I ended up debugging for some more on windows, and then looking into
the history of this issue.

The following modification works for testing Andrey's patch or also my
v2, and doesn't use a hard coded timeout, it reproduces the original
reported issue 100% in my tests on windows. It could be still timing
dependent on CI under load, but that just means that sometimes the
test would pass with the original output, not _1.

step terminate3 {
SELECT pg_terminate_backend(pid) FROM pg_stat_activity
WHERE wait_event = 'injection-points-wait';
+ SELECT pg_sleep(0.1);
}

It doesn't test the remainder issue fixed by Nikolay's 0003 or also by
v2-0001, but it is a simple change. Alternatively, if we do this in a
wait loop we can also reproduce the additional failure reported/fixed
by Nikolay without a hardcoded timeout.

I attached v3, which contains this additional change (the looped version).

I also looked into the history of this, and I think we might solve the
problem instead, at least on master:

6051857fc on 2021-12-02 added closesocket() in socket_close() under
#ifdef WIN32.
ed52c3707 on 2021-12-07 also shutdown(sock, SD_SEND)

these would fix the issue properly, making the alternative outputs and
the isolation tester fix unnecessary, however

75674c7ec on 2022-01-25 reverted in back branches because of issues
with walreciever
29992a6a5 on 2022-03-22 reverted also in master for the same reason

but also

a8458f508a7 on 2024-07-13 fixed the issue causing that instability, so
now it should be safe to revert the revert, at least I can't reproduce
the problem mentioned in the reverts with this fix in place, and I can
without it.
In theory a8458f508a7 was backported everywhere, but that doesn't mean
this change would be completely risk-free.

On Tue, Sep 29, 2026 at 12:24:46PM +0100, Zsolt Parragi wrote:

a8458f508a7 on 2024-07-13 fixed the issue causing that instability, so
now it should be safe to revert the revert, at least I can't reproduce
the problem mentioned in the reverts with this fix in place, and I can
without it.
In theory a8458f508a7 was backported everywhere, but that doesn't mean
this change would be completely risk-free.

Thanks for looking at some history and putting some pieces together.
I was not aware of this stuff.

In this context, the revert of the revert can be translated as `git
revert 29992a6a509b`, right?  If we do that, being able to get rid of
the alternate outputs would be super nice, and we would not even need
to have alternate outputs like that:
--- temp-schema-cleanup.out	2026-07-27 08:05:11.192854428 +0900
+++ temp-schema-cleanup_1.out	2026-09-30 08:03:16.982528616 +0900
@@ -89,10 +89,10 @@ DETAIL:  It depends on temporary type ju
 step s1_exit: 
     SELECT pg_terminate_backend(pg_backend_pid());

-FATAL:  terminating connection due to administrator command
 server closed the connection unexpectedly
 	This probably means the server terminated abnormally
 	before or while processing the request.
+invalid socket

step s2_advisory: <... completed>
pg_advisory_lock

This points to the fact that losing the messages is wrong, because the
tests do not check what we want them to do in some environments as the
data we want is not received in the backends. And we would not even
need the isolationtester.c tweaks, assuming that things work out and
that we can rely on the fact that the backend has sent all its
messages, would we?

+    DO $$
+    BEGIN
+      WHILE EXISTS (SELECT FROM pg_stat_activity
+                    WHERE application_name = 'isolation/wait_cleanup/s1')
+      LOOP
+        PERFORM pg_sleep(0.01);
+        PERFORM pg_stat_clear_snapshot();
+      END LOOP;
+    END$$;

Even that feels like the wrong thing to do, spreading a tweak that
ought to be simpler for folks implement tests.

In short, it means that the proposal is papering over the actual
Windows problem: pqcomm.c should really close that socket, so my issue
here is that everybody has lost track of the inter-dependency between
all these issues; pieces that you are just putting together. I would
not mind experimenting with a revert of 29992a6a509b on HEAD, at
least, and give it some time to brew before deciding what to do with
the stable branches.

Another option that would be on the table for me would be to make
these tests conditional, not running on WIN32 where we know they're
unstable. We cannot do that within schedule files so
temp-schema-cleanup would be an issue if not moved out somewhere else
(just test_misc with a conditional ISOLATION list?), and I don't
remember a way to control that cleanly at SQL level.. We could do a
solution based on a test list filtering in test_decoding and
injection_points, at least, taking take of two instabilities out of
three.

The perfect scenario for me would be to prove that undoing
29992a6a509b is now really-absolutely-stable safe, as it's still a
server bug to me to not send back this information back to the client
on WIN32.

What do you think?
--
Michael

In this context, the revert of the revert can be translated as `git
revert 29992a6a509b`, right? If we do that, being able to get rid of
the alternate outputs would be super nice, and we would not even need
to have alternate outputs like that:

Yes. If we revert 29992a6a509b, I can't reproduce any of the issues anymore. We can still make a test modification if we want to explicitly test for this scenario, to make sure it doesn't reappear, but we won't need the alternative outputs or the isolationtester modifications.

+    DO $$
+    BEGIN
+      WHILE EXISTS (SELECT FROM pg_stat_activity
+                    WHERE application_name = 'isolation/wait_cleanup/s1')
+      LOOP
+        PERFORM pg_sleep(0.01);
+        PERFORM pg_stat_clear_snapshot();
+      END LOOP;
+    END$$;

Even that feels like the wrong thing to do, spreading a tweak that
ought to be simpler for folks implement tests.

We don't have to spread this around, I added this to one scenario to explicitly test the missing last message issue. This, or the simpler single pg_sleep call makes it deterministic. This is the part either in this form or just as the one line pg_sleep addition that might be worth keeping even in the 29992a6a509b direction, so we notice if the issue comes back / still happens sometimes.

What do you think?

I agree with the let's try the revert on master approach. That won't help with random failures on the stable branches, but at least we are aware why it is happening now, and later we can either apply the revert on them, or disable these tests on them, or apply v3.

The perfect scenario for me would be to prove that undoing
29992a6a509b is now really-absolutely-stable safe, as it's still a
server bug to me to not send back this information back to the client
on WIN32.

The question is, what would be good enough proof? I can reproduce the walreceiver issue with around 1% failure rate on my laptop, and it didn't reproduce even once with only reverting 29992a6a509b in more than 5000 runs.
I'll try to do the same thing on back branches, and I'll also set it up on github actions to test it there.

On Wed, Sep 30, 2026 at 12:31:53PM +0100, Zsolt Parragi wrote:

The question is, what would be good enough proof?

I got to the same conclusion independently, from the client side:
clients on Windows get "connection reset" instead of the server's
FATAL, and there's nothing a client can do about it, because by the
time it reads, the data is already gone. Here are my results, in case
another machine helps.

Setup: Windows 11 Pro 25H2 (build 26200), MSVC 19.50, meson, OpenSSL
3.6.2, master at 9510a826e4a, all over localhost. "Patched" means
master plus the revert of 29992a6a509b.

1. wait_cleanup with your pg_sleep(0.1) after pg_terminate_backend(),
and the rest of the injection_points isolation suite, 20 runs each:

unpatched: 20/20 lose the FATAL ("PQconsumeInput failed: server
closed the connection unexpectedly"), as in Andrey's CI
patched: 20/20 get the FATAL, and the suite passes every time

2. Startup FATAL. The attached script sends a startup packet for a
role that doesn't exist and sleeps before reading; 50 attempts per
delay:

delay before read unpatched patched
0 ms 50/50 50/50
50 ms 4/50 50/50
300 ms 0/50 50/50

3. The tests that hung in 2022 (commit_ts/002_standby,
commit_ts/003_standby_2, recovery/001_stream_rep), patched, 20 runs
each: all 60 passed, no hangs.

4. SSL, patched: ssl/001-004, 20 runs each, all passed. Alexander
reported in [1]/messages/by-id/32d112ee-0b6f-d4ab-441b-e2bba66a1d83@gmail.com that the revoked-client-cert case in 001_ssltests.pl
sometimes got "Software caused connection abort" with the earlier
patch set, so I also looped just that case 2000 times on both builds.
It reported "certificate revoked" every time on each.

5. A full meson test run, patched, with PG_TEST_EXTRA=ssl but without
injection points (those are covered by 1). Everything passed except
pg_test_timing/001_basic and psql/001_basic, which fail here because
of the Polish locale's decimal comma, not because of the patch.

Not tested: a connection that isn't over localhost, and the back
branches.

Two more pieces of history that may help: Thomas already proposed
re-committing 6051857fc on master in March 2025 [2]/messages/by-id/CA+hUKGKJSOAdAukP4QTkR3-jFws39+8C197XC-a97dgYr=cdBA@mail.gmail.com, and nobody
objected, but it didn't happen. And there's a remaining walreceiver
hang on WSAECONNRESET that a8458f508 doesn't cover [3]/messages/by-id/93515d62-edce-9041-ec6e-7122f6e92bea@gmail.com, but it happens
without the revert too, so I don't think it's related.

So +1 for trying the revert on master.

[1]: /messages/by-id/32d112ee-0b6f-d4ab-441b-e2bba66a1d83@gmail.com
[2]: /messages/by-id/CA+hUKGKJSOAdAukP4QTkR3-jFws39+8C197XC-a97dgYr=cdBA@mail.gmail.com
[3]: /messages/by-id/93515d62-edce-9041-ec6e-7122f6e92bea@gmail.com

--
Kacper Kuras

On 30 Sep 2026, at 04:46, Michael Paquier <michael@paquier.xyz> wrote:

29992a6a509b

Wow, what a deep story for just a couple of lines!

+1 for trying to revert in master.

I think there's more benefit than possible harm. There are ~4 similar
failures a month on master. If we resurrect walreceiver problems, we will
notice soon. There were like ~20 failures a month. And a8458f508a7 commit
message looks promising (I do not understand commit itself).

Best regards, Andrey Borodin.

On Wed, Sep 30, 2026 at 05:31:53AM -0700, Zsolt Parragi wrote:

I agree with the let's try the revert on master approach. That won't
help with random failures on the stable branches, but at least we are
aware why it is happening now, and later we can either apply the
revert on them, or disable these tests on them, or apply v3.

It would mean that the spurious failures would still be annoying on
stable branches while we evaluate the HEAD case. See below.

The question is, what would be good enough proof? I can reproduce the
walreceiver issue with around 1% failure rate on my laptop, and it
didn't reproduce even once with only reverting 29992a6a509b in more
than 5000 runs.
I'll try to do the same thing on back branches, and I'll also set it
up on github actions to test it there.

How about the following plan, instead? I would suggest moving forward
with the following steps:
- First disable these tests on WIN32, backpatch this change to
entirely silence the buildfarm. I am not really sure that we gain
much coverage by keeping them while the non-WIN32 paths would still
stress the cases in a stable manner. In order to do that, some
Makefile and meson.build can be manipulated to make the tests
conditional depending on the platform, something that we already do
that. One small-ish issue here is temp-schema-cleanup in
src/test/isolation/. Let's just move that to test_misc, I guess
(cannot think of a better location), create an ISOLATION target list
in a conditional manner.
- Revert the revert on HEAD, allow the tests to work on WIN32.
- Monitor the result for a few days or weeks. Investigate if more
actions are required and what to do in stable branches, and if we
actually want to do something in stable branches. My gut feeling is
that we'd do that only on HEAD, this has been reverted a lot already.

The meson and Makefile tricks already exist in the tree:
* For meson "if host_system == 'windows'".
* For Makefile, that should be "ifeq ($(PORTNAME), win32)".

This plan is risk-free: even if step 1 is reverted, we still have the
benefit of not running these unstable tests on WIN32, keeping the
buildfarm stable in the long run anyway.

So, thoughts?
--
Michael

On Wed, Sep 30, 2026 at 11:00:26PM +0500, Andrey Borodin wrote:

I think there's more benefit than possible harm. There are ~4 similar
failures a month on master. If we resurrect walreceiver problems, we will
notice soon. There were like ~20 failures a month. And a8458f508a7 commit
message looks promising (I do not understand commit itself).

Well, as mentioned in this commit, peeking at a socket before sleeping
is qualified as a kludge, probably not the best long-term approach. I
doubt that a lot of people are motivated to redesign the event vs
socket handling just to make for a better socket implementation of
Postgres on WIN32, and I suspect that relying on Thomas's mage skills
should be good enough. FWIW, I trust his skills, and also I have no
plans to dive into this leel of details for the WIN32 issues; there's
more than enough going on.

Now I don't mind running the revert/revert experiment and see how it
goes, at the condition that we make all the branches more stable as a
starting point. Bypassing these tests on WIN32 is part of the plan
I'd be OK with.
--
Michael

I attached v4.

I only disabled the two tests that already had failures on CI. After
looking into it again I realized that temp-schema-cleanup could only
fail with my hacked together linux repro, I can't reproduce it on
windows. In wait_cleanup and slot_creation_error one session kills
another session. In temp-schema-cleanup s1 kills itself. When the
fatal happens the isolation tester is reading s1's socket, and
receives the message properly, I can't make that fail without
modifying the testcase.

On Fri, Oct 02, 2026 at 10:44:20PM +0100, Zsolt Parragi wrote:

I only disabled the two tests that already had failures on CI. After
looking into it again I realized that temp-schema-cleanup could only
fail with my hacked together linux repro, I can't reproduce it on
windows. In wait_cleanup and slot_creation_error one session kills
another session.

Thanks for the patch. That should be able to do the job. Good idea
to mention the thread, for future references.

In temp-schema-cleanup s1 kills itself. When the
fatal happens the isolation tester is reading s1's socket, and
receives the message properly, I can't make that fail without
modifying the testcase.

Okay, that's good to know. That leads to an even simpler patch.
--
Michael

On Fri, Oct 02, 2026 at 10:44:20PM +0100, Zsolt Parragi wrote:

I only disabled the two tests that already had failures on CI. After
looking into it again I realized that temp-schema-cleanup could only
fail with my hacked together linux repro, I can't reproduce it on
windows. In wait_cleanup and slot_creation_error one session kills
another session. In temp-schema-cleanup s1 kills itself. When the
fatal happens the isolation tester is reading s1's socket, and
receives the message properly, I can't make that fail without
modifying the testcase.

Okay, let's only disable these two as a first step, then. For the
Makefiles, I have reused the same thing as pgcrypto, so as the
conditional test is stored in a variable before saving the ISOLATION
list. For meson, the lists have been moved outside the "tests". All
that felt slightly cleaner.

Now back to the main issue..
--
Michael