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