[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

Hi Ilmar, JoongHyuk,

On Wed, Jun 3, 2026 at 6:23 PM Ilmar Yunusov <tanswis42@gmail.com> 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

While working on fixing [1]/messages/by-id/7685519a-0bf9-4e17-93ca-7e3aa10fa29c@gmail.com, I noticed the same TODO comment about
ResolveRecoveryConflictWithBufferPin() in the nearby code. That led me
to this thread while I was doing some background research.

It seems to me that this thread is closely related to the bug in [2]/messages/by-id/44c24dcf-5710-410f-b1b6-d10b315f3d51@postgrespro.ru.
In the latest version, both patches modify the ownership of the same
wait loop, the timeout lifecycle, and waiter registration. Because of
that, I wonder whether the bug fix should be landed first, with this
patch rebased on top of it.

If that's the case, I think the current patch status should probably
be changed to Waiting on Author, so that it isn't picked up for commit
prematurely, though both patches are already on Fujii-san's radar.

[1]: /messages/by-id/7685519a-0bf9-4e17-93ca-7e3aa10fa29c@gmail.com
[2]: /messages/by-id/44c24dcf-5710-410f-b1b6-d10b315f3d51@postgrespro.ru

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

Hi Xuneng,

Thanks for tying the threads together.
When Álvaro raised the possible connection back in May,
I concluded these were independent fixes for different problems,
even though both already touch ResolveRecoveryConflictWithBufferPin().

That still holds, but with the deadlock-detector fix now ready for commit
and heading to the back branches,
the two reworkings of that function will collide.
So I agree it should go in first and this patch should rebase on top.

--
JH Shin

Hi hackers,

I'm fairly new to this part of the tree, so before reviewing the patch
itself
I spent some time on the surrounding code to make sure I actually understand
the setting. Let me lay out that understanding first -- I'd appreciate
corrections where I have it wrong.

The question I kept coming back to is: why is the hot-standby case the only
one
that needs all this machinery, when the primary takes a single, untimed
ProcWaitForSignal(), and a cold standby never runs into the contention at
all?

My understanding of the chain:

- A cleanup lock is stronger than an exclusive content lock: it also
requires
sole pinning (refcount == 1). Its purpose is to let prune/VACUUM
physically
remove tuples and compact the page, so there must be no other backend
still
referencing a tuple in that page by pointer -- and a pin is how that
presence
is detected.

- On the primary, the waiter is just VACUUM/prune, and a stall delays only
that
one operation -- the rest of the system runs on -- so waiting indefinitely
(the untimed ProcWaitForSignal) is harmless.

- A cold standby (hot_standby = off) has no read-only queries, hence no
query
backend pinning the page to wait for, so it never even waits -- simpler
still
than the primary.

The special case is the hot standby.

- On a standby, VACUUM (and prune) is also carried out by WAL replay -- the
cleanup that happened on the primary arrives as a WAL record, which the
startup process replays under the cleanup lock.

- But a hot standby is also serving read-only queries, whose pins -- e.g. a
cursor holding a page across a client round-trip -- conflict with that
cleanup lock.

- Replay is a single serialized stream, so one stalled record stalls every
WAL
record behind it: the conflict is no longer isolated, it becomes
replication
lag for the whole standby.

- So the replay side cannot just wait -- it arms STANDBY_DEADLOCK_TIMEOUT
and
STANDBY_TIMEOUT, and ultimately cancels the pin holder at
max_standby_*_delay.

So the complexity is specific to doing two things at once on one node:
serving
queries and replaying a strictly ordered stream. The primary avoids it
because
its waits are isolated; the cold standby avoids it because it has no
queries.
The hot standby has neither escape, and this path is where that tension is
sharpest -- which is why a small change to it seems to warrant more careful
review than most.

Does the above match how others see it? Assuming it does, here is what I
found
tracing the patch itself.

The essence of this change is that, after the deadlock timeout fires,
instead of
returning to the caller the function re-waits toward the delay timeout
(STANDBY_TIMEOUT) inside itself -- the new second ProcWaitForSignal() in
ResolveRecoveryConflictWithBufferPin(). The code itself looks correct to
me.

It is also worth checking whether the two timeouts (deadlock / delay) can
fire
in the same SIGALRM and set both got_standby_delay_timeout and
got_standby_deadlock_timeout at once. And even if they cannot, whether the
code
should defensively account for that possibility -- rather than relying
implicitly
on the if/else ordering -- is worth considering.

And since this behavior depends on the relationship between
deadlock_timeout and
max_standby_streaming_delay (whether the deadlock branch is reached at all,
and
whether the second wait still has a timer), a test combining those
relationships
seems worthwhile. Mapping the four regimes against 054:

max_standby_streaming_delay | deadlock | how it ends |
patched | test
| branch | |
code |

--------------------------------+-------------+-----------------------+---------+----------

deadlock_timeout | reached | cancel at delay |

yes | yes (031,054)
-1 (wait forever, ltime == 0) | reached | UnpinBuffer only, |
yes | none
| | no cancel -> infinite |
|
0 (immediate) | not reached | fast-path cancel at |
no | none
| | entry |
|
0 < delay < deadlock_timeout | not reached | cancel before the |
no | none
| | deadlock branch |
|

So the delay > deadlock_timeout regime is already well covered -- 031
exercises
both the buffer-pin conflict and the buffer-pin deadlock (via the
backend-side
check), and 054 adds the log timing -- though all with a finite delay. The
other three regimes are untested, and -1 in particular is the patch's core
path:
the only mode where the second wait has no startup-side timer and relies
solely
on the backend-side deadlock check plus UnpinBuffer.

I realize asking for that extra coverage may be a lot. But the -1 case in
particular has no startup-side timer to fall back on, so it seemed worth
widening coverage a little.

Regards,
Henson

Hi Henson,

Thanks for your review and showing the "chain of thought"!

On Sat, Jun 27, 2026 at 1:11 AM Henson Choi <assam258@gmail.com> wrote:

Hi hackers,

I'm fairly new to this part of the tree, so before reviewing the patch itself
I spent some time on the surrounding code to make sure I actually understand
the setting. Let me lay out that understanding first -- I'd appreciate
corrections where I have it wrong.

The question I kept coming back to is: why is the hot-standby case the only one
that needs all this machinery, when the primary takes a single, untimed
ProcWaitForSignal(), and a cold standby never runs into the contention at all?

My understanding of the chain:

- A cleanup lock is stronger than an exclusive content lock: it also requires
sole pinning (refcount == 1). Its purpose is to let prune/VACUUM physically
remove tuples and compact the page, so there must be no other backend still
referencing a tuple in that page by pointer -- and a pin is how that presence
is detected.

I agree with your analysis here. The cleanup lock is the key to
enforce rule of 2

Buffer access rules:
2. Once one has determined that a tuple is interesting (visible to the
current transaction) one may drop the content lock, yet continue to access
the tuple's data for as long as one holds the buffer pin. This is what is
typically done by heap scans, since the tuple returned by heap_fetch
contains a pointer to tuple data in the shared buffer. Therefore the
tuple cannot go away while the pin is held (see rule #5). Its state could
change, but that is assumed not to matter after the initial determination
of visibility is made.

The pin itself is just a mechanism to ensure the stability of mapping
from page to buffer frame. It cannot guarantee the stability of tuples
on its own. To ensure scan can keep using a tuple pointer after
dropping the content lock(buffer access rule of 2), we need the HOT
prune / vacuum operations to adhere to the cleanup lock rule(sole
pinning): do not change the physical layout of the page unless nobody
other than us referencing it.

- On the primary, the waiter is just VACUUM/prune, and a stall delays only that
one operation -- the rest of the system runs on -- so waiting indefinitely
(the untimed ProcWaitForSignal) is harmless.

I would not frame this as "harmless", the stall can still delay
VACUUM/pruning/index cleanup and contribute to bloat or local stalls
in primary.

- A cold standby (hot_standby = off) has no read-only queries, hence no query
backend pinning the page to wait for, so it never even waits -- simpler still
than the primary.

I agree that a cold standby lacks the normal source of long-lived
conflicting pins. But lockBufferForCleanup() seems to still use the
simple non-hot-standby ProcWaitForSignal() path if some short-lived
internal pin exists.

The special case is the hot standby.

- On a standby, VACUUM (and prune) is also carried out by WAL replay -- the
cleanup that happened on the primary arrives as a WAL record, which the
startup process replays under the cleanup lock.

- But a hot standby is also serving read-only queries, whose pins -- e.g. a
cursor holding a page across a client round-trip -- conflict with that
cleanup lock.

- Replay is a single serialized stream, so one stalled record stalls every WAL
record behind it: the conflict is no longer isolated, it becomes replication
lag for the whole standby.

- So the replay side cannot just wait -- it arms STANDBY_DEADLOCK_TIMEOUT and
STANDBY_TIMEOUT, and ultimately cancels the pin holder at max_standby_*_delay.

So the complexity is specific to doing two things at once on one node: serving
queries and replaying a strictly ordered stream. The primary avoids it because
its waits are isolated; the cold standby avoids it because it has no queries.
The hot standby has neither escape, and this path is where that tension is
sharpest -- which is why a small change to it seems to warrant more careful
review than most.

Does the above match how others see it? Assuming it does, here is what I found
tracing the patch itself.

I think another important reason for this design of primary is: the
workload of primary is hybrid, mixed with write and read operations.
We cannot say this to a backend / application from system level: you
guys have been writing/reading too much for too long, so we decide to
kill you or let you decide your own fate right now just because what
you are doing here is in conflict with the maintenance operations
whose purpose are hopefully to let you continue your work in the
future. So we choose to let vaccum/prune to wait since it would harm
both availability of the system and the confidence of the user if it
is done otherwise. For hot standby, that's another story as you said.

The essence of this change is that, after the deadlock timeout fires, instead of
returning to the caller the function re-waits toward the delay timeout
(STANDBY_TIMEOUT) inside itself -- the new second ProcWaitForSignal() in
ResolveRecoveryConflictWithBufferPin(). The code itself looks correct to me.

It is also worth checking whether the two timeouts (deadlock / delay) can fire
in the same SIGALRM and set both got_standby_delay_timeout and
got_standby_deadlock_timeout at once. And even if they cannot, whether the code
should defensively account for that possibility -- rather than relying implicitly
on the if/else ordering -- is worth considering.

And since this behavior depends on the relationship between deadlock_timeout and
max_standby_streaming_delay (whether the deadlock branch is reached at all, and
whether the second wait still has a timer), a test combining those relationships
seems worthwhile. Mapping the four regimes against 054:

max_standby_streaming_delay | deadlock | how it ends | patched | test
| branch | | code |
--------------------------------+-------------+-----------------------+---------+----------

deadlock_timeout | reached | cancel at delay | yes | yes (031,054)

-1 (wait forever, ltime == 0) | reached | UnpinBuffer only, | yes | none
| | no cancel -> infinite | |
0 (immediate) | not reached | fast-path cancel at | no | none
| | entry | |
0 < delay < deadlock_timeout | not reached | cancel before the | no | none
| | deadlock branch | |

So the delay > deadlock_timeout regime is already well covered -- 031 exercises
both the buffer-pin conflict and the buffer-pin deadlock (via the backend-side
check), and 054 adds the log timing -- though all with a finite delay. The
other three regimes are untested, and -1 in particular is the patch's core path:
the only mode where the second wait has no startup-side timer and relies solely
on the backend-side deadlock check plus UnpinBuffer.

I realize asking for that extra coverage may be a lot. But the -1 case in
particular has no startup-side timer to fall back on, so it seemed worth
widening coverage a little.

I haven't reviewed the patch yet.

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

Hi Henson,

Thanks for the careful read.

Both timeouts can be set in a single SIGALRM, since handle_sig_alarm() fires
every timeout that is already due. The if/else order is deliberate. The
delay
timeout is checked first because the cancel supersedes the deadlock check,
so
the both-set case resolves to the cancel, and no defensive branch is needed.

You're right about the coverage too. Only the delay > deadlock_timeout
regime
is exercised today, and -1 is the one where the second wait has no
startup-side
timer to fall back on. This patch will be rebased on top of the
deadlock-detector
fix that lands first, and I'll add a -1 regime test as part of that rebase.

--
JH Shin

On Sun, Jun 28, 2026 at 6:06 PM JoongHyuk Shin <sjh910805@gmail.com> wrote:

Hi Henson,

Thanks for the careful read.

Both timeouts can be set in a single SIGALRM, since handle_sig_alarm() fires
every timeout that is already due. The if/else order is deliberate. The delay
timeout is checked first because the cancel supersedes the deadlock check, so
the both-set case resolves to the cancel, and no defensive branch is needed.

You're right about the coverage too. Only the delay > deadlock_timeout regime
is exercised today, and -1 is the one where the second wait has no startup-side
timer to fall back on. This patch will be rebased on top of the deadlock-detector
fix that lands first, and I'll add a -1 regime test as part of that rebase.

The patch LGTM overall. However, there seems to be a rare race issue
that could lead to lost wake-up and reply-stall.

A problematic interleaving seems possible:

1) The startup process is waiting for a buffer pin, with
BM_PIN_COUNT_WAITER set.
2) STANDBY_DEADLOCK_TIMEOUT fires. handle_sig_alarm() sets the startup
process latch, and StandbyDeadLockHandler() sets
got_standby_deadlock_timeout = true.
3) WaitLatch() returns, but before ProcWaitForSignal() calls
ResetLatch(), the final unpinner unpins the buffer.
4) WakePinCountWaiter() sees BUF_STATE_GET_REFCOUNT(buf_state) == 1,
clears BM_PIN_COUNT_WAITER, and calls ProcSendSignal() for the startup
process.
5) ProcSendSignal() calls SetLatch(), but the latch is already set by
the deadlock timeout, so this wakeup is coalesced.
6) ProcWaitForSignal() then calls ResetLatch(), clearing the latch.
7) v3 sees got_standby_deadlock_timeout, sends the deadlock-check
signal, clears the flag, then enters the new second
ProcWaitForSignal().
8) At that point the buffer is already ready, but there may be no
future unpin to wake the startup process as BM_PIN_COUNT_WAITER has
already been cleared.

If a finite standby delay is set, startup might sleep until
STANDBY_TIMEOUT. If the delay is set to -1, no standby timeout is
armed, this could lead to an infinite waiting until an unrelated latch
event occurs.

To address this, I think this optimization should be rebased on the
code in the related fix [1]/messages/by-id/44c24dcf-5710-410f-b1b6-d10b315f3d51@postgrespro.ru. Let the resolve rechecks after each wake:

ProcWaitForSignal(WAIT_EVENT_BUFFER_CLEANUP);

if (BufferIsReadyForCleanup(bufid + 1))
break;

If we are already the last pinner here, just return rather than enter
a second wait; otherwise, re-registers the startup as the pin-count
waiter before waiting again.

[1]: /messages/by-id/44c24dcf-5710-410f-b1b6-d10b315f3d51@postgrespro.ru

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

Hi,

Sorry for the late status update. As Fujii and Xuneng already know,
this patch is waiting on a prerequisite fix in the same function [1]/messages/by-id/44c24dcf-5710-410f-b1b6-d10b315f3d51@postgrespro.ru.
That one is under review, and I will pick this up again once it is
committed.

[1]: /messages/by-id/44c24dcf-5710-410f-b1b6-d10b315f3d51@postgrespro.ru
/messages/by-id/44c24dcf-5710-410f-b1b6-d10b315f3d51@postgrespro.ru

--
JH Shin