Re: Set 1s WaitLatch timeout if standby limit has expired in ResolveRecoveryConflictWithBufferPin

Hi Anthony, Shihao,

Thanks for the patch. I reviewed v3 (CF entry 6445) and tested it locally.

What I tested
- v3 applies cleanly with "git apply" on master at 9f191dce61e (no
rebase needed). Build with --enable-cassert --enable-debug has no
warnings, before or after the patch.
- Reproducer: primary plus streaming standby (pg_basebackup -R),
max_standby_streaming_delay = 2s, log_recovery_conflict_waits = on. I
followed Dmytro's steps, with one change: session A is "COPY (SELECT
t.id, repeat('y', 1000000) FROM bufferpin_test t) TO STDOUT | sleep",
so it sits in ClientWrite holding the pin. After the VACUUM on the
primary I waited 5s so the limit had expired and the startup process
was in BufferCleanup, then opened session B (BEGIN; DECLARE c CURSOR
...; FETCH 5 FROM c), and then killed the "sleep" so session A goes
away.
- Without the patch (master 9f191dce61e): session B is never
cancelled. 30s after session A was gone the startup process was still
in BufferCleanup, B was still idle in transaction, and replay was
behind by 34640 bytes. The log has only "recovery still waiting after
1000.189 ms: recovery conflict on buffer pin" and no "finished
waiting" line.
- With v3: in both runs session B got "FATAL: terminating connection
due to conflict with recovery" while session A was still stuck in
ClientWrite, i.e. before I released A and within my 2s check after B
took its pin. The startup process resumed once A was released
("recovery finished waiting after 7059.663 ms: recovery conflict on
buffer pin" in the first run, 7043.339 ms in the second), and the
replay lag went back to 0.
- make -C src/test/recovery check
PROVE_TESTS=t/031_recovery_conflict.pl passes with the patch (18
tests).

Comments
1. The fix looks correct to me. ProcWaitForSignal() is just
WaitLatch(MyLatch, WL_LATCH_SET | WL_EXIT_ON_PM_DEATH, 0, ...)
followed by ResetLatch() and CHECK_FOR_INTERRUPTS(), so the new branch
is the same call with WL_TIMEOUT and 1000 ms. After the timeout,
LockBufferForCleanup() rechecks the pin count and calls
ResolveRecoveryConflictWithBufferPin() again, where the limit_expired
branch resends the signal. Other cases are unchanged: when the limit
has not expired the STANDBY_TIMEOUT and STANDBY_DEADLOCK_TIMEOUT path
is untouched, and with max_standby_streaming_delay = -1 (ltime == 0)
the wait stays unbounded, which is intended.
2. The #define is placed between the large header comment and the
function, so the comment block no longer sits directly above
ResolveRecoveryConflictWithBufferPin(). It would read better at the
top of the file, next to the other static state in standby.c.
3. While the limit stays expired, the startup process now sends the
conflict signal to all backends once per second for as long as a pin
is held. This is cheap, and a backend that cannot process the signal
(like session A in ClientWrite) just gets it again, but it may be
worth saying in the commit message or the comment that this is
intended. It also means the deadlock-check part of the existing XXX
comment is not affected, since the expired path never armed
STANDBY_DEADLOCK_TIMEOUT.
4. Tests: the patch has none. A TAP test seems possible with the
pattern above (two sessions with BackgroundPsql, the second one
pinning after the limit has expired and the first one still holding),
but the first backend has to be blocked somewhere it cannot process
the signal, which is why I used ClientWrite. If you think it is too
timing sensitive for the tree, a short comment saying how it was
verified would still help. I can share my script if useful.
5. Commit message: it could add "Reported-by"/"Reviewed-by" trailers
(Dmytro, Andrey, Shihao) and a "Backpatch-through" line. Since v3 does
not touch proc.c or proc.h it should be easy to backpatch, as Shihao
says, and Andrey and Dmytro saw it on 17 and 18.
6. Process: CF entries 6444 and 6445 both point to this same thread,
and both are in "Needs review" for PG20-3. It might be good to close
one of them so reviews are not split.

Best regards,
João Rito

Em ter., 10 de nov. de 2026 às 20:46, shihao zhong
<zhong950419(at)gmail(dot)com> escreveu:
>
> Hi Andrey,
>
> Sorry I missed your email. I closed 6445 because it is
> duplicate with https://commitfest.postgresql.org/patch/6444/
>
> But I think this is an important issue and we need to fix it.
>
> Thanks,
> Shihao