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