[PATCH] Prevent repeated deadlock-check signals in standby buffer pin waits

On Sun, Apr 19, 2026 at 2:47 PM JoongHyuk Shin <sjh910805@gmail.com> wrote:

In ResolveRecoveryConflictWithBufferPin(), when deadlock_timeout fires,
the function sends RECOVERY_CONFLICT_BUFFERPIN_DEADLOCK and returns.
The caller (LockBufferForCleanup) loops back, sets up another deadlock_timeout,
and the signal gets sent again every interval.

The lock-conflict path had the same problem and was fixed in 8900b5a9d59a
by adding a second ProcWaitForSignal() after the deadlock-check signal.
The buffer-pin path was left with an XXX comment asking "should we fix this?".

The attached patch applies the same fix: after sending the deadlock-check
signal, reset got_standby_deadlock_timeout and call ProcWaitForSignal()
so the startup process waits for UnpinBuffer() rather than looping
and re-signaling.

Patch attached.

Thanks for the patch! LGTM.

Since this change improves recovery-conflict behavior rather than fixing a bug,
it doesn't seem to need backpatching and we may need to wait until v20
development opens (probably July) before committing it.

While reading the patch and ResolveRecoveryConflictWithBufferPin(), I also
noticed that got_standby_delay_timeout is not initialized to false before
enabling the timeout. This is unrelated to the patch, and I think it is
harmless in the current code, but would it be better to initialize it there,
as we already do for got_standby_deadlock_timeout?

if (ltime != 0)
{
+ got_standby_delay_timeout = false;
timeouts[cnt].id = STANDBY_TIMEOUT;
timeouts[cnt].type = TMPARAM_AT;
timeouts[cnt].fin_time = ltime;

Regards,

--
Fujii Masao

On Tue, Apr 21, 2026 at 02:42:38PM +0900, Fujii Masao wrote:

Since this change improves recovery-conflict behavior rather than fixing a bug,
it doesn't seem to need backpatching and we may need to wait until v20
development opens (probably July) before committing it.

Yeah, this one is an improvement, not an actual bug, so let's wait for
v20 if worth doing (I did not check it).
--
Michael

Thanks for the review.
v2 attached, with the suggested initialization added for symmetry.

Agreed this is an improvement rather than a bug fix,
so I've updated the CF tag to Performance accordingly.

I also verified the fix locally on a primary-standby setup,
using the buffer-pin conflict scenario from src/test/recovery/t/
031_recovery_conflict.pl
(aborted INSERT + cursor on standby + VACUUM FREEZE on primary).
On master, strace showed 9 SIGUSR1 broadcasts to the conflicting backend
over a 10-second window (one per deadlock_timeout).
With the patch applied, only 1 broadcast over the same window.

Patch attached.

--
JoongHyuk Shin

On Tue, Apr 21, 2026 at 2:55 PM Michael Paquier <michael@paquier.xyz> wrote:

Hello,

On 2026-Apr-21, Michael Paquier wrote:

On Tue, Apr 21, 2026 at 02:42:38PM +0900, Fujii Masao wrote:

Since this change improves recovery-conflict behavior rather than fixing a bug,
it doesn't seem to need backpatching and we may need to wait until v20
development opens (probably July) before committing it.

Yeah, this one is an improvement, not an actual bug, so let's wait for
v20 if worth doing (I did not check it).

Hmm, is this related to
/messages/by-id/44c24dcf-5710-410f-b1b6-d10b315f3d51@postgrespro.ru ?
In there, Vitaly claims to be reporting a bug that goes back to pg15,
which contradicts this assessment.

Regards,

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"¿Qué importan los años? Lo que realmente importa es comprobar que
a fin de cuentas la mejor edad de la vida es estar vivo" (Mafalda)

Hello.

Same function, different races, I think.
Vitaly reports a missed wake-up where deadlock_timeout never fires
(spurious SIGALRM from log_startup_progress_interval plus the lazy
setitimer in 09cf1d52).
This patch addresses the opposite,
deadlock_timeout does fire, but LockBufferForCleanup loops back and re-arms
it,
so the signal repeats once per second.

The two fixes do not overlap
(the added ProcWaitForSignal sits inside the deadlock branch that Vitaly's
scenario never reaches).

--
JH Shin

On Wed, May 20, 2026 at 7:15 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:

On Fri, May 22, 2026 at 05:41:03PM +0900, JoongHyuk Shin wrote:

This patch addresses the opposite,
deadlock_timeout does fire, but LockBufferForCleanup loops back and re-arms
it, so the signal repeats once per second.

Right. I don't really see why this should be backpatched. One
argument would be more consistency of this area of the code across all
the stable branches, but the argument is kind of moot as this does not
fix a problem, just improves a bit what we have.
--
Michael

Hello!

Shouldn't this need a condition similar to ResolveRecoveryConflictWithLock (if (logging_conflict) ...)?

Otherwise this can result in a long wait time, with:

log_recovery_conflict_waits = on
deadlock_timeout = 100ms
max_standby_streaming_delay = 5s

It can wait for 5 seconds instead of the current 0.1 seconds without the change.

The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: tested, failed
Spec compliant: not tested
Documentation: not tested

Hi,

I looked at v2, focusing on the latest question about
log_recovery_conflict_waits behavior.

The patch applies cleanly on current master at
db5ed03217b9c238703df8b4b286115d6e940488. git diff --check reports no
issues. I built both master and v2 locally with:

./configure --prefix="$PWD/pg-install" --without-readline --without-zlib --without-icu
make -s -j8
make -s install

Both builds passed.

I tried to check the case Zsolt described. In my local repro, the final
cancellation still happens at about max_standby_streaming_delay in both
master and v2. The visible difference is that v2 delays the first
log_recovery_conflict_waits "recovery still waiting" message.

I used a small primary/standby setup based on the buffer-pin conflict pattern
from src/test/recovery/t/031_recovery_conflict.pl:

log_recovery_conflict_waits = on
deadlock_timeout = 100ms
max_standby_streaming_delay = 5s

The scenario is:

1. Create a small table.
2. Generate an aborted tuple on the primary and replay it on the standby.
3. Open a cursor on the standby and fetch one row, leaving the transaction
idle with the cursor open.
4. Run VACUUM FREEZE on the primary.

On current master, the standby log shows:

recovery still waiting after 101.060 ms: recovery conflict on buffer pin
terminating connection due to conflict with recovery
recovery finished waiting after 5001.324 ms: recovery conflict on buffer pin

With v2, the standby log shows:

recovery still waiting after 5001.064 ms: recovery conflict on buffer pin
terminating connection due to conflict with recovery
recovery finished waiting after 5001.535 ms: recovery conflict on buffer pin

So in this repro the early "still waiting" log at deadlock_timeout is lost,
and the first waiting log is emitted only when the wait is already being
resolved at about max_standby_streaming_delay.

This seems to come from the new second ProcWaitForSignal() inside
ResolveRecoveryConflictWithBufferPin(). After the deadlock timeout fires, v2
sends RECOVERY_CONFLICT_BUFFERPIN_DEADLOCK and waits again internally. For a
non-deadlock buffer pin holder, LockBufferForCleanup() does not get control
back after deadlock_timeout, so it cannot emit the early recovery conflict
log message.

This looks similar to the reason ResolveRecoveryConflictWithLock() has the
logging_conflict argument: if the conflict still needs to be logged, it returns
after sending the deadlock-check signal, and only avoids the repeated wait on
the later call after the conflict has been logged.

Should ResolveRecoveryConflictWithBufferPin() preserve the same behavior, for
example by skipping the new second wait until LockBufferForCleanup() has had a
chance to emit the first log_recovery_conflict_waits message?

I have not checked the repeated SIGUSR1 count with strace in my local macOS
environment, and I have not reviewed the backpatching question.

Regards,
Ilmar Yunusov

The new status of this patch is: Waiting on Author

Thanks for the reviews.

v3 attached.

* Emit "recovery still waiting" inside the function.
It now fires at deadlock_timeout instead of max_standby_streaming_delay
(Ilmar).

* Pass waitStart and &logged_recovery_conflict from the caller;
the in-function branch reuses the same gate.

* An early-return alternative reopens a race in the
SetStartupBufferPinWaitBufId(-1) gap; the lock path has
no equivalent because its caller is structured differently.

* Covered by src/test/recovery/t/054_bufferpin_conflict_log_timing.pl
(FAIL on v2, PASS on v3).

--
JH Shin

On Fri, May 29, 2026 at 3:31 PM Michael Paquier <michael@paquier.xyz> wrote:

The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested

Hi,

I looked at v3 again, this time on Linux, focusing on the repeated SIGUSR1
behavior and the log_recovery_conflict_waits timing issue I reported for v2.

I used the v3 attachment from JoongHyuk's 2026-06-03 message, on
origin/master at f2081a7800f1696cb0415bacd655cb41b7b9ca63.

The patch applies cleanly with git am, and git diff --check reports no
issues.

I built with:

./configure --prefix="$PWD/pg-install" --without-readline --without-zlib --without-icu --enable-tap-tests
make -s -j3
make -s install

That passed.

The new targeted TAP test passed:

make -C src/test/recovery check PROVE_TESTS=t/054_bufferpin_conflict_log_timing.pl

Result:

t/054_bufferpin_conflict_log_timing.pl .. ok
All tests successful.
Files=1, Tests=3
Result: PASS

I also ran the full recovery TAP suite:

make -C src/test/recovery check

That passed too:

All tests successful.
Files=53, Tests=633
Result: PASS

Six tests were skipped because injection points were not supported by this
build.

For the signal behavior, I ran the same buffer-pin conflict reproducer under
strace on the standby postmaster and its children:

strace -ff -qq -e trace=kill,tgkill,tkill

The count below is for kill/tgkill/tkill(..., SIGUSR1) syscalls during the
conflict window, after subtracting signals already seen before VACUUM FREEZE.

On unpatched master:

sigusr1_delta=51
recovery still waiting after 100.442 ms: recovery conflict on buffer pin
terminating connection due to conflict with recovery
recovery finished waiting after 5001.455 ms: recovery conflict on buffer pin

With v3:

sigusr1_delta=2
recovery still waiting after 100.479 ms: recovery conflict on buffer pin
terminating connection due to conflict with recovery
recovery finished waiting after 5001.778 ms: recovery conflict on buffer pin

I interpret the two v3 SIGUSR1 syscalls as the one deadlock-check signal and
the final cancellation signal at max_standby_streaming_delay. So in this
repro, v3 removes the repeated deadlock-check signals every deadlock_timeout,
while keeping the "recovery still waiting" log near deadlock_timeout.

I did not find a new issue in the checked path.

I have not reviewed the backpatching question, and I did not run
installcheck-world.

Regards,
Ilmar Yunusov

The new status of this patch is: Ready for Committer