pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #18961: Race scenario where max_standby_streaming_delay is not honored
9+ messages / 3 participants
[nested] [flat]
* BUG #18961: Race scenario where max_standby_streaming_delay is not honored
@ 2025-06-19 00:27 PG Bug reporting form <noreply@postgresql.org>
2025-06-19 15:17 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Dilip Kumar <dilipbalaut@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: PG Bug reporting form @ 2025-06-19 00:27 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: erwaman@gmail.com
The following bug has been logged on the website:
Bug reference: 18961
Logged by: Anthony Hsu
Email address: erwaman@gmail.com
PostgreSQL version: 17.5
Operating system: Linux
Description:
In the current ResolveRecoveryConflictWithBufferPin implementation in
standby.c, I think there's a race scenario where a backend holding a
conflicting buffer pin won't receive a PROCSIG_RECOVERY_CONFLICT_BUFFERPIN
message promptly:
1. Assume max_standby_streaming_delay has expired when the startup process
enters ResolveRecoveryConflictWithBufferPin
2. Assume backend 1 holds a conflicting buffer pin while backend 2 does not
3. Since we are past the standby limit time, the startup process broadcasts
PROCSIG_RECOVERY_CONFLICT_BUFFERPIN here [1] without enabling any timeouts
4. Then the startup process waits to be woken up via
ProcWaitForSignal(WAIT_EVENT_BUFFER_PIN) here [2]
5. Suppose backend 2 receives PROCSIG_RECOVERY_CONFLICT_BUFFERPIN first,
sees it does not hold a conflicting buffer pin, and *then* proceeds to pin
the buffer
6. Suppose then backend 1 receives PROCSIG_RECOVERY_CONFLICT_BUFFERPIN,
processes interrupts, and cancels itself. During cleanup, in UnpinBuffer(),
it will see the pin count is still > 1 (startup process + backend 2 have it
pinned), so it will NOT wake up the startup process.
7. The startup process will only get woken up once backend 2 unpins the
buffer and the pin count reaches 1 (or some other signal causes the startup
process's latch to be set). Only then will it try to acquire the cleanup
lock again and broadcast another PROCSIG_RECOVERY_CONFLICT_BUFFERPIN message
if it fails to acquire the cleanup lock.
The unexpected behavior is in step (7), where it could be arbitrarily long
until the startup process is woken up again. The expected behavior is that
since max_standby_streaming_delay has already expired, the startup process
should wake up quickly and broadcast another
PROCSIG_RECOVERY_CONFLICT_BUFFERPIN message if there are still conflicting
backends.
I was able to reproduce this scenario with some custom code to control the
execution sequence.
One way to fix this scenario is to just remove the `if` block here [3]
entirely so that we always enable the STANDBY_TIMEOUT and
STANDBY_DEADLOCK_TIMEOUT timeouts.
[1]
https://github.com/postgres/postgres/blob/45c357e0e85d2dffe7af5440806150124a725a01/src/backend/stora...
[2]
https://github.com/postgres/postgres/blob/45c357e0e85d2dffe7af5440806150124a725a01/src/backend/stora...
[3]
https://github.com/postgres/postgres/blob/45c357e0e85d2dffe7af5440806150124a725a01/src/backend/stora...
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored
2025-06-19 00:27 BUG #18961: Race scenario where max_standby_streaming_delay is not honored PG Bug reporting form <noreply@postgresql.org>
@ 2025-06-19 15:17 ` Dilip Kumar <dilipbalaut@gmail.com>
2025-06-19 15:58 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Anthony Hsu <erwaman@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Dilip Kumar @ 2025-06-19 15:17 UTC (permalink / raw)
To: erwaman@gmail.com; pgsql-bugs@lists.postgresql.org
On Thu, Jun 19, 2025 at 5:25 PM PG Bug reporting form
<noreply@postgresql.org> wrote:
>
> The following bug has been logged on the website:
>
> Bug reference: 18961
> Logged by: Anthony Hsu
> Email address: erwaman@gmail.com
> PostgreSQL version: 17.5
> Operating system: Linux
> Description:
>
> In the current ResolveRecoveryConflictWithBufferPin implementation in
> standby.c, I think there's a race scenario where a backend holding a
> conflicting buffer pin won't receive a PROCSIG_RECOVERY_CONFLICT_BUFFERPIN
> message promptly:
> 1. Assume max_standby_streaming_delay has expired when the startup process
> enters ResolveRecoveryConflictWithBufferPin
> 2. Assume backend 1 holds a conflicting buffer pin while backend 2 does not
> 3. Since we are past the standby limit time, the startup process broadcasts
> PROCSIG_RECOVERY_CONFLICT_BUFFERPIN here [1] without enabling any timeouts
> 4. Then the startup process waits to be woken up via
> ProcWaitForSignal(WAIT_EVENT_BUFFER_PIN) here [2]
> 5. Suppose backend 2 receives PROCSIG_RECOVERY_CONFLICT_BUFFERPIN first,
> sees it does not hold a conflicting buffer pin, and *then* proceeds to pin
> the buffer
> 6. Suppose then backend 1 receives PROCSIG_RECOVERY_CONFLICT_BUFFERPIN,
> processes interrupts, and cancels itself. During cleanup, in UnpinBuffer(),
> it will see the pin count is still > 1 (startup process + backend 2 have it
> pinned), so it will NOT wake up the startup process.
> 7. The startup process will only get woken up once backend 2 unpins the
> buffer and the pin count reaches 1 (or some other signal causes the startup
> process's latch to be set). Only then will it try to acquire the cleanup
> lock again and broadcast another PROCSIG_RECOVERY_CONFLICT_BUFFERPIN message
> if it fails to acquire the cleanup lock.
> The unexpected behavior is in step (7), where it could be arbitrarily long
> until the startup process is woken up again. The expected behavior is that
> since max_standby_streaming_delay has already expired, the startup process
> should wake up quickly and broadcast another
> PROCSIG_RECOVERY_CONFLICT_BUFFERPIN message if there are still conflicting
> backends.
> I was able to reproduce this scenario with some custom code to control the
> execution sequence.
> One way to fix this scenario is to just remove the `if` block here [3]
> entirely so that we always enable the STANDBY_TIMEOUT and
> STANDBY_DEADLOCK_TIMEOUT timeouts.
> [1]
> https://github.com/postgres/postgres/blob/45c357e0e85d2dffe7af5440806150124a725a01/src/backend/stora...
> [2]
> https://github.com/postgres/postgres/blob/45c357e0e85d2dffe7af5440806150124a725a01/src/backend/stora...
> [3]
> https://github.com/postgres/postgres/blob/45c357e0e85d2dffe7af5440806150124a725a01/src/backend/stora...
I agree this looks like a race condition, but I am not sure about the
proposed solution. ResolveRecoveryConflictWithBufferPin() is called by
LockBufferForCleanup() to wait for a process currently holding a
buffer pin. However, new processes can still acquire the pin.. So I
think the problem lies in the logic of the wakeup mechanism of the
UnpinBuffer() no? The intended behavior is for the startup process to
be woken regardless of Backend 2 subsequently acquiring the pin, but
the current tracking mechanism is insufficient.
--
Regards,
Dilip Kumar
Google
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored
2025-06-19 00:27 BUG #18961: Race scenario where max_standby_streaming_delay is not honored PG Bug reporting form <noreply@postgresql.org>
2025-06-19 15:17 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Dilip Kumar <dilipbalaut@gmail.com>
@ 2025-06-19 15:58 ` Anthony Hsu <erwaman@gmail.com>
2025-06-27 05:05 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Dilip Kumar <dilipbalaut@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Anthony Hsu @ 2025-06-19 15:58 UTC (permalink / raw)
To: Dilip Kumar <dilipbalaut@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
Yes, another option might be to change the wakeup logic so that the startup
process is still woken up even if new processes have acquired the pin. The
main thing is the startup process should be woken up promptly so that it
can recheck if it can acquire the cleanup lock and if not,
send PROCSIG_RECOVERY_CONFLICT_BUFFERPIN again to cancel any new backends.
Before the standby limit time (default 30s) is reached,
ResolveRecoveryConflictWithBufferPin will enable both a standby limit
timeout and a deadlock timeout (default 1s). So if someone is holding a
conflicting buffer pin for a long time, due to the deadlock timeout, the
startup process will get woken up every 1s and recheck until we reach the
standby limit, at which point it'll send the
PROCSIG_RECOVERY_CONFLICT_BUFFERPIN. But after reaching standby limit, the
next time the startup process does ResolveRecoveryConflictWithBufferPin, it
only sends PROCSIG_RECOVERY_CONFLICT_BUFFERPIN without enabling the
timeouts, which leads to the possibility of this race. So I thought a
simple solution to address this race would be to just always enable the
timeouts.
For context, the reason I noticed this issue and am interested in getting
it fixed is that I have seen cases where the startup process gets stalled
for minutes and sometimes hours due to conflicting buffer pins, leading to
high replay lag, even though the standby limit has not been changed (still
the default 30s). My expectation is that once the standby limit has
expired, conflicting backends should be promptly canceled.
On Thu, Jun 19, 2025 at 8:18 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> On Thu, Jun 19, 2025 at 5:25 PM PG Bug reporting form
> <noreply@postgresql.org> wrote:
> >
> > The following bug has been logged on the website:
> >
> > Bug reference: 18961
> > Logged by: Anthony Hsu
> > Email address: erwaman@gmail.com
> > PostgreSQL version: 17.5
> > Operating system: Linux
> > Description:
> >
> > In the current ResolveRecoveryConflictWithBufferPin implementation in
> > standby.c, I think there's a race scenario where a backend holding a
> > conflicting buffer pin won't receive a
> PROCSIG_RECOVERY_CONFLICT_BUFFERPIN
> > message promptly:
> > 1. Assume max_standby_streaming_delay has expired when the startup
> process
> > enters ResolveRecoveryConflictWithBufferPin
> > 2. Assume backend 1 holds a conflicting buffer pin while backend 2 does
> not
> > 3. Since we are past the standby limit time, the startup process
> broadcasts
> > PROCSIG_RECOVERY_CONFLICT_BUFFERPIN here [1] without enabling any
> timeouts
> > 4. Then the startup process waits to be woken up via
> > ProcWaitForSignal(WAIT_EVENT_BUFFER_PIN) here [2]
> > 5. Suppose backend 2 receives PROCSIG_RECOVERY_CONFLICT_BUFFERPIN first,
> > sees it does not hold a conflicting buffer pin, and *then* proceeds to
> pin
> > the buffer
> > 6. Suppose then backend 1 receives PROCSIG_RECOVERY_CONFLICT_BUFFERPIN,
> > processes interrupts, and cancels itself. During cleanup, in
> UnpinBuffer(),
> > it will see the pin count is still > 1 (startup process + backend 2 have
> it
> > pinned), so it will NOT wake up the startup process.
> > 7. The startup process will only get woken up once backend 2 unpins the
> > buffer and the pin count reaches 1 (or some other signal causes the
> startup
> > process's latch to be set). Only then will it try to acquire the cleanup
> > lock again and broadcast another PROCSIG_RECOVERY_CONFLICT_BUFFERPIN
> message
> > if it fails to acquire the cleanup lock.
> > The unexpected behavior is in step (7), where it could be arbitrarily
> long
> > until the startup process is woken up again. The expected behavior is
> that
> > since max_standby_streaming_delay has already expired, the startup
> process
> > should wake up quickly and broadcast another
> > PROCSIG_RECOVERY_CONFLICT_BUFFERPIN message if there are still
> conflicting
> > backends.
> > I was able to reproduce this scenario with some custom code to control
> the
> > execution sequence.
> > One way to fix this scenario is to just remove the `if` block here [3]
> > entirely so that we always enable the STANDBY_TIMEOUT and
> > STANDBY_DEADLOCK_TIMEOUT timeouts.
> > [1]
> >
> https://github.com/postgres/postgres/blob/45c357e0e85d2dffe7af5440806150124a725a01/src/backend/stora...
> > [2]
> >
> https://github.com/postgres/postgres/blob/45c357e0e85d2dffe7af5440806150124a725a01/src/backend/stora...
> > [3]
> >
> https://github.com/postgres/postgres/blob/45c357e0e85d2dffe7af5440806150124a725a01/src/backend/stora...
>
> I agree this looks like a race condition, but I am not sure about the
> proposed solution. ResolveRecoveryConflictWithBufferPin() is called by
> LockBufferForCleanup() to wait for a process currently holding a
> buffer pin. However, new processes can still acquire the pin.. So I
> think the problem lies in the logic of the wakeup mechanism of the
> UnpinBuffer() no? The intended behavior is for the startup process to
> be woken regardless of Backend 2 subsequently acquiring the pin, but
> the current tracking mechanism is insufficient.
>
> --
> Regards,
> Dilip Kumar
> Google
>
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored
2025-06-19 00:27 BUG #18961: Race scenario where max_standby_streaming_delay is not honored PG Bug reporting form <noreply@postgresql.org>
2025-06-19 15:17 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Dilip Kumar <dilipbalaut@gmail.com>
2025-06-19 15:58 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Anthony Hsu <erwaman@gmail.com>
@ 2025-06-27 05:05 ` Dilip Kumar <dilipbalaut@gmail.com>
2025-06-27 06:11 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Anthony Hsu <erwaman@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Dilip Kumar @ 2025-06-27 05:05 UTC (permalink / raw)
To: Anthony Hsu <erwaman@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
On Thu, Jun 19, 2025 at 9:28 PM Anthony Hsu <erwaman@gmail.com> wrote:
>
> Yes, another option might be to change the wakeup logic so that the startup process is still woken up even if new processes have acquired the pin. The main thing is the startup process should be woken up promptly so that it can recheck if it can acquire the cleanup lock and if not, send PROCSIG_RECOVERY_CONFLICT_BUFFERPIN again to cancel any new backends.
>
> Before the standby limit time (default 30s) is reached, ResolveRecoveryConflictWithBufferPin will enable both a standby limit timeout and a deadlock timeout (default 1s). So if someone is holding a conflicting buffer pin for a long time, due to the deadlock timeout, the startup process will get woken up every 1s and recheck until we reach the standby limit, at which point it'll send the PROCSIG_RECOVERY_CONFLICT_BUFFERPIN. But after reaching standby limit, the next time the startup process does ResolveRecoveryConflictWithBufferPin, it only sends PROCSIG_RECOVERY_CONFLICT_BUFFERPIN without enabling the timeouts, which leads to the possibility of this race. So I thought a simple solution to address this race would be to just always enable the timeouts.
Again looking at the code and I got confused with the code placement
of ProcWaitForSignal(WAIT_EVENT_BUFFER_PIN);, I mean if we are already
behind then we broadcast 'PROCSIG_RECOVERY_CONFLICT_BUFFERPIN' and
then wait for WAIT_EVENT_BUFFER_PIN signal, Otherwise, we will wait on
timeouts, and once woken up by timeout we broadcast
'PROCSIG_RECOVERY_CONFLICT_BUFFERPIN' but now we don't bother to wait
for WAIT_EVENT_BUFFER_PIN. If I am not missing something this
behavior seems inconsistent to me.
--
Regards,
Dilip Kumar
Google
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored
2025-06-19 00:27 BUG #18961: Race scenario where max_standby_streaming_delay is not honored PG Bug reporting form <noreply@postgresql.org>
2025-06-19 15:17 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Dilip Kumar <dilipbalaut@gmail.com>
2025-06-19 15:58 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Anthony Hsu <erwaman@gmail.com>
2025-06-27 05:05 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Dilip Kumar <dilipbalaut@gmail.com>
@ 2025-06-27 06:11 ` Anthony Hsu <erwaman@gmail.com>
2025-06-27 06:26 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Dilip Kumar <dilipbalaut@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Anthony Hsu @ 2025-06-27 06:11 UTC (permalink / raw)
To: Dilip Kumar <dilipbalaut@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
On Thu, Jun 26, 2025 at 10:05 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> On Thu, Jun 19, 2025 at 9:28 PM Anthony Hsu <erwaman@gmail.com> wrote:
> >
> > Yes, another option might be to change the wakeup logic so that the
> startup process is still woken up even if new processes have acquired the
> pin. The main thing is the startup process should be woken up promptly so
> that it can recheck if it can acquire the cleanup lock and if not, send
> PROCSIG_RECOVERY_CONFLICT_BUFFERPIN again to cancel any new backends.
> >
> > Before the standby limit time (default 30s) is reached,
> ResolveRecoveryConflictWithBufferPin will enable both a standby limit
> timeout and a deadlock timeout (default 1s). So if someone is holding a
> conflicting buffer pin for a long time, due to the deadlock timeout, the
> startup process will get woken up every 1s and recheck until we reach the
> standby limit, at which point it'll send the
> PROCSIG_RECOVERY_CONFLICT_BUFFERPIN. But after reaching standby limit, the
> next time the startup process does ResolveRecoveryConflictWithBufferPin, it
> only sends PROCSIG_RECOVERY_CONFLICT_BUFFERPIN without enabling the
> timeouts, which leads to the possibility of this race. So I thought a
> simple solution to address this race would be to just always enable the
> timeouts.
>
> Again looking at the code and I got confused with the code placement
> of ProcWaitForSignal(WAIT_EVENT_BUFFER_PIN);, I mean if we are already
> behind then we broadcast 'PROCSIG_RECOVERY_CONFLICT_BUFFERPIN' and
> then wait for WAIT_EVENT_BUFFER_PIN signal, Otherwise, we will wait on
> timeouts, and once woken up by timeout we broadcast
> 'PROCSIG_RECOVERY_CONFLICT_BUFFERPIN' but now we don't bother to wait
> for WAIT_EVENT_BUFFER_PIN.
If we get woken by the standby_delay timeout, we broadcast
PROCSIG_RECOVERY_CONFLICT_BUFFERPIN and then return from this method
(ResolveRecoveryConflictWithBufferPin) back to LockBufferForCleanup, which
will then loop back to the beginning of the for loop [1]. It will then
check if it is the exclusive pinner again, and if not,
re-enter ResolveRecoveryConflictWithBufferPin, and then wait for
WAIT_EVENT_BUFFER_PIN.
I've attached a patch fixing the race condition by always enabling the
standby limit and deadlock timeouts.
[1]
https://github.com/postgres/postgres/blob/94e2e150ec72a3b37e3847be99c4aca3320c38f9/src/backend/stora...
> If I am not missing something this
> behavior seems inconsistent to me.
>
> --
> Regards,
> Dilip Kumar
> Google
>
Attachments:
[application/octet-stream] 0001-Always-enable-timeouts-in-ResolveRecoveryConflictWit.patch (2.5K, ../../CALQc50idegfrVB-wYe33XF0xPOZ3-BX9KaQQjS-5DxioHix4Yg@mail.gmail.com/3-0001-Always-enable-timeouts-in-ResolveRecoveryConflictWit.patch)
download | inline diff:
From 0e6a1a86ac62f0aa8481363256cc503328e1e970 Mon Sep 17 00:00:00 2001
From: Anthony Hsu <erwaman@gmail.com>
Date: Thu, 26 Jun 2025 22:58:59 -0700
Subject: [PATCH] Always enable timeouts in
ResolveRecoveryConflictWithBufferPin
---
src/backend/storage/ipc/standby.c | 49 ++++++++++++-------------------
1 file changed, 19 insertions(+), 30 deletions(-)
diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c
index 7fa8d9247e0..81a04a571c0 100644
--- a/src/backend/storage/ipc/standby.c
+++ b/src/backend/storage/ipc/standby.c
@@ -769,8 +769,7 @@ cleanup:
* The ProcWaitForSignal() sleep normally done in LockBufferForCleanup()
* (when not InHotStandby) is performed here, for code clarity.
*
- * We either resolve conflicts immediately or set a timeout to wake us at
- * the limit of our patience.
+ * We set a timeout to wake us at the limit of our patience.
*
* Resolve conflicts by sending a PROCSIG signal to all backends to check if
* they hold one of the buffer pins that is blocking Startup process. If so,
@@ -798,38 +797,28 @@ ResolveRecoveryConflictWithBufferPin(void)
ltime = GetStandbyLimitTime();
- if (GetCurrentTimestamp() >= ltime && ltime != 0)
+ /*
+ * Wake up at ltime, and check for deadlocks as well if we will be
+ * waiting longer than deadlock_timeout
+ */
+ EnableTimeoutParams timeouts[2];
+ int cnt = 0;
+
+ if (ltime != 0)
{
- /*
- * We're already behind, so clear a path as quickly as possible.
- */
- SendRecoveryConflictWithBufferPin(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN);
+ timeouts[cnt].id = STANDBY_TIMEOUT;
+ timeouts[cnt].type = TMPARAM_AT;
+ timeouts[cnt].fin_time = ltime;
+ cnt++;
}
- else
- {
- /*
- * Wake up at ltime, and check for deadlocks as well if we will be
- * waiting longer than deadlock_timeout
- */
- EnableTimeoutParams timeouts[2];
- int cnt = 0;
- if (ltime != 0)
- {
- timeouts[cnt].id = STANDBY_TIMEOUT;
- timeouts[cnt].type = TMPARAM_AT;
- timeouts[cnt].fin_time = ltime;
- cnt++;
- }
-
- got_standby_deadlock_timeout = false;
- timeouts[cnt].id = STANDBY_DEADLOCK_TIMEOUT;
- timeouts[cnt].type = TMPARAM_AFTER;
- timeouts[cnt].delay_ms = DeadlockTimeout;
- cnt++;
+ got_standby_deadlock_timeout = false;
+ timeouts[cnt].id = STANDBY_DEADLOCK_TIMEOUT;
+ timeouts[cnt].type = TMPARAM_AFTER;
+ timeouts[cnt].delay_ms = DeadlockTimeout;
+ cnt++;
- enable_timeouts(timeouts, cnt);
- }
+ enable_timeouts(timeouts, cnt);
/*
* Wait to be signaled by UnpinBuffer() or for the wait to be interrupted
--
2.50.0.727.gbf7dc18ff4-goog
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored
2025-06-19 00:27 BUG #18961: Race scenario where max_standby_streaming_delay is not honored PG Bug reporting form <noreply@postgresql.org>
2025-06-19 15:17 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Dilip Kumar <dilipbalaut@gmail.com>
2025-06-19 15:58 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Anthony Hsu <erwaman@gmail.com>
2025-06-27 05:05 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Dilip Kumar <dilipbalaut@gmail.com>
2025-06-27 06:11 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Anthony Hsu <erwaman@gmail.com>
@ 2025-06-27 06:26 ` Dilip Kumar <dilipbalaut@gmail.com>
2025-06-28 23:37 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Anthony Hsu <erwaman@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Dilip Kumar @ 2025-06-27 06:26 UTC (permalink / raw)
To: Anthony Hsu <erwaman@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
On Fri, Jun 27, 2025 at 11:41 AM Anthony Hsu <erwaman@gmail.com> wrote:
>
>
>
> On Thu, Jun 26, 2025 at 10:05 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>>
>> On Thu, Jun 19, 2025 at 9:28 PM Anthony Hsu <erwaman@gmail.com> wrote:
>> >
>> > Yes, another option might be to change the wakeup logic so that the startup process is still woken up even if new processes have acquired the pin. The main thing is the startup process should be woken up promptly so that it can recheck if it can acquire the cleanup lock and if not, send PROCSIG_RECOVERY_CONFLICT_BUFFERPIN again to cancel any new backends.
>> >
>> > Before the standby limit time (default 30s) is reached, ResolveRecoveryConflictWithBufferPin will enable both a standby limit timeout and a deadlock timeout (default 1s). So if someone is holding a conflicting buffer pin for a long time, due to the deadlock timeout, the startup process will get woken up every 1s and recheck until we reach the standby limit, at which point it'll send the PROCSIG_RECOVERY_CONFLICT_BUFFERPIN. But after reaching standby limit, the next time the startup process does ResolveRecoveryConflictWithBufferPin, it only sends PROCSIG_RECOVERY_CONFLICT_BUFFERPIN without enabling the timeouts, which leads to the possibility of this race. So I thought a simple solution to address this race would be to just always enable the timeouts.
>>
>> Again looking at the code and I got confused with the code placement
>> of ProcWaitForSignal(WAIT_EVENT_BUFFER_PIN);, I mean if we are already
>> behind then we broadcast 'PROCSIG_RECOVERY_CONFLICT_BUFFERPIN' and
>> then wait for WAIT_EVENT_BUFFER_PIN signal, Otherwise, we will wait on
>> timeouts, and once woken up by timeout we broadcast
>> 'PROCSIG_RECOVERY_CONFLICT_BUFFERPIN' but now we don't bother to wait
>> for WAIT_EVENT_BUFFER_PIN.
>
>
> If we get woken by the standby_delay timeout, we broadcast PROCSIG_RECOVERY_CONFLICT_BUFFERPIN and then return from this method (ResolveRecoveryConflictWithBufferPin) back to LockBufferForCleanup, which will then loop back to the beginning of the for loop [1]. It will then check if it is the exclusive pinner again, and if not, re-enter ResolveRecoveryConflictWithBufferPin, and then wait for WAIT_EVENT_BUFFER_PIN.
>
Oh yeah, that's correct.
--
Regards,
Dilip Kumar
Google
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored
2025-06-19 00:27 BUG #18961: Race scenario where max_standby_streaming_delay is not honored PG Bug reporting form <noreply@postgresql.org>
2025-06-19 15:17 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Dilip Kumar <dilipbalaut@gmail.com>
2025-06-19 15:58 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Anthony Hsu <erwaman@gmail.com>
2025-06-27 05:05 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Dilip Kumar <dilipbalaut@gmail.com>
2025-06-27 06:11 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Anthony Hsu <erwaman@gmail.com>
2025-06-27 06:26 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Dilip Kumar <dilipbalaut@gmail.com>
@ 2025-06-28 23:37 ` Anthony Hsu <erwaman@gmail.com>
2025-06-30 03:28 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Dilip Kumar <dilipbalaut@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Anthony Hsu @ 2025-06-28 23:37 UTC (permalink / raw)
To: Dilip Kumar <dilipbalaut@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
Made a few changes to my patch and attached a new version. Changes include:
* avoid sending PROCSIG_RECOVERY_CONFLICT_BUFFERPIN in a tight loop after
standby limit is reached by adding a small sleep after each send, starting
at 1ms and doubling each time up to 1s (similar to what's done
in WaitExceedsMaxStandbyDelay here [1]). Sleep time is reset in
LockBufferForCleanup once cleanup lock is successfully acquired
* don't set deadlock timeout once standby limit has passed since after
that, the standby timeout will fire immediately, so no need to set deadlock
timeout as well
Let me know if you have any thoughts or comments. I am thinking of sending
it to pgsql-hackers soon.
[1]
https://github.com/postgres/postgres/blob/50fd428b2b9cb036c9c5982b56443d7e28119707/src/backend/stora...
On Thu, Jun 26, 2025 at 11:27 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> On Fri, Jun 27, 2025 at 11:41 AM Anthony Hsu <erwaman@gmail.com> wrote:
> >
> >
> >
> > On Thu, Jun 26, 2025 at 10:05 PM Dilip Kumar <dilipbalaut@gmail.com>
> wrote:
> >>
> >> On Thu, Jun 19, 2025 at 9:28 PM Anthony Hsu <erwaman@gmail.com> wrote:
> >> >
> >> > Yes, another option might be to change the wakeup logic so that the
> startup process is still woken up even if new processes have acquired the
> pin. The main thing is the startup process should be woken up promptly so
> that it can recheck if it can acquire the cleanup lock and if not, send
> PROCSIG_RECOVERY_CONFLICT_BUFFERPIN again to cancel any new backends.
> >> >
> >> > Before the standby limit time (default 30s) is reached,
> ResolveRecoveryConflictWithBufferPin will enable both a standby limit
> timeout and a deadlock timeout (default 1s). So if someone is holding a
> conflicting buffer pin for a long time, due to the deadlock timeout, the
> startup process will get woken up every 1s and recheck until we reach the
> standby limit, at which point it'll send the
> PROCSIG_RECOVERY_CONFLICT_BUFFERPIN. But after reaching standby limit, the
> next time the startup process does ResolveRecoveryConflictWithBufferPin, it
> only sends PROCSIG_RECOVERY_CONFLICT_BUFFERPIN without enabling the
> timeouts, which leads to the possibility of this race. So I thought a
> simple solution to address this race would be to just always enable the
> timeouts.
> >>
> >> Again looking at the code and I got confused with the code placement
> >> of ProcWaitForSignal(WAIT_EVENT_BUFFER_PIN);, I mean if we are already
> >> behind then we broadcast 'PROCSIG_RECOVERY_CONFLICT_BUFFERPIN' and
> >> then wait for WAIT_EVENT_BUFFER_PIN signal, Otherwise, we will wait on
> >> timeouts, and once woken up by timeout we broadcast
> >> 'PROCSIG_RECOVERY_CONFLICT_BUFFERPIN' but now we don't bother to wait
> >> for WAIT_EVENT_BUFFER_PIN.
> >
> >
> > If we get woken by the standby_delay timeout, we broadcast
> PROCSIG_RECOVERY_CONFLICT_BUFFERPIN and then return from this method
> (ResolveRecoveryConflictWithBufferPin) back to LockBufferForCleanup, which
> will then loop back to the beginning of the for loop [1]. It will then
> check if it is the exclusive pinner again, and if not, re-enter
> ResolveRecoveryConflictWithBufferPin, and then wait for
> WAIT_EVENT_BUFFER_PIN.
> >
> Oh yeah, that's correct.
>
> --
> Regards,
> Dilip Kumar
> Google
>
Attachments:
[application/octet-stream] v2-0001-Always-enable-standby-timeout-in-ResolveRecoveryC.patch (5.7K, ../../CALQc50i+3XaW18DzZ9y0mOXu0UtOpias5nev3GNY48J=9rX6AA@mail.gmail.com/3-v2-0001-Always-enable-standby-timeout-in-ResolveRecoveryC.patch)
download | inline diff:
From a6950d0b5592444b33035f03ec431efbd0d1a8d5 Mon Sep 17 00:00:00 2001
From: Anthony Hsu <erwaman@gmail.com>
Date: Thu, 26 Jun 2025 22:58:59 -0700
Subject: [PATCH v2] Always enable standby timeout in
ResolveRecoveryConflictWithBufferPin to ensure standby limit is honored.
---
src/backend/storage/buffer/bufmgr.c | 7 +++
src/backend/storage/ipc/standby.c | 68 ++++++++++++++++++-----------
src/include/storage/standby.h | 8 ++++
3 files changed, 57 insertions(+), 26 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 667aa0c0c78..6fb0f491633 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -5721,6 +5721,13 @@ LockBufferForCleanup(Buffer buffer)
/* Successfully acquired exclusive lock with pincount 1 */
UnlockBufHdr(bufHdr, buf_state);
+ /*
+ * Reset sleep time after sending
+ * PROCSIG_RECOVERY_CONFLICT_BUFFERPIN for the next buffer pin
+ * conflict.
+ */
+ bufferpin_wait_us = BUFFERPIN_INITIAL_WAIT_US;
+
/*
* Emit the log message if recovery conflict on buffer pin was
* resolved but the startup process waited longer than
diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c
index 7fa8d9247e0..778c269b13c 100644
--- a/src/backend/storage/ipc/standby.c
+++ b/src/backend/storage/ipc/standby.c
@@ -41,6 +41,12 @@ int max_standby_archive_delay = 30 * 1000;
int max_standby_streaming_delay = 30 * 1000;
bool log_recovery_conflict_waits = false;
+/*
+ * Sleep time after sending PROCSIG_RECOVERY_CONFLICT_BUFFERPIN. Reset in
+ * LockBufferForCleanup after successfully acquiring cleanup lock.
+ */
+int bufferpin_wait_us = BUFFERPIN_INITIAL_WAIT_US;
+
/*
* Keep track of all the exclusive locks owned by original transactions.
* For each known exclusive lock, there is a RecoveryLockEntry in the
@@ -769,8 +775,8 @@ cleanup:
* The ProcWaitForSignal() sleep normally done in LockBufferForCleanup()
* (when not InHotStandby) is performed here, for code clarity.
*
- * We either resolve conflicts immediately or set a timeout to wake us at
- * the limit of our patience.
+ * We set a timeout to wake us at the limit of our patience. This will trigger
+ * immediately if the deadline has already passed.
*
* Resolve conflicts by sending a PROCSIG signal to all backends to check if
* they hold one of the buffer pins that is blocking Startup process. If so,
@@ -798,39 +804,32 @@ ResolveRecoveryConflictWithBufferPin(void)
ltime = GetStandbyLimitTime();
- if (GetCurrentTimestamp() >= ltime && ltime != 0)
+ /*
+ * Wake up at ltime, and check for deadlocks as well if we will be
+ * waiting longer than deadlock_timeout
+ */
+ EnableTimeoutParams timeouts[2];
+ int cnt = 0;
+
+ if (ltime != 0)
{
- /*
- * We're already behind, so clear a path as quickly as possible.
- */
- SendRecoveryConflictWithBufferPin(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN);
+ timeouts[cnt].id = STANDBY_TIMEOUT;
+ timeouts[cnt].type = TMPARAM_AT;
+ timeouts[cnt].fin_time = ltime;
+ cnt++;
}
- else
- {
- /*
- * Wake up at ltime, and check for deadlocks as well if we will be
- * waiting longer than deadlock_timeout
- */
- EnableTimeoutParams timeouts[2];
- int cnt = 0;
-
- if (ltime != 0)
- {
- timeouts[cnt].id = STANDBY_TIMEOUT;
- timeouts[cnt].type = TMPARAM_AT;
- timeouts[cnt].fin_time = ltime;
- cnt++;
- }
+ if (ltime == 0 || GetCurrentTimestamp() < ltime)
+ {
got_standby_deadlock_timeout = false;
timeouts[cnt].id = STANDBY_DEADLOCK_TIMEOUT;
timeouts[cnt].type = TMPARAM_AFTER;
timeouts[cnt].delay_ms = DeadlockTimeout;
cnt++;
-
- enable_timeouts(timeouts, cnt);
}
+ enable_timeouts(timeouts, cnt);
+
/*
* Wait to be signaled by UnpinBuffer() or for the wait to be interrupted
* by one of the timeouts established above.
@@ -843,7 +842,24 @@ ResolveRecoveryConflictWithBufferPin(void)
ProcWaitForSignal(WAIT_EVENT_BUFFER_PIN);
if (got_standby_delay_timeout)
+ {
SendRecoveryConflictWithBufferPin(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN);
+
+ /*
+ * Sleep a bit to avoid busy-waiting and repeatedly sending
+ * PROCSIG_RECOVERY_CONFLICT_BUFFERPIN.
+ */
+ pgstat_report_wait_start(WAIT_EVENT_BUFFER_PIN);
+ pg_usleep(bufferpin_wait_us);
+ pgstat_report_wait_end();
+
+ /*
+ * Progressively increase the sleep time but to no more than 1 second.
+ */
+ bufferpin_wait_us *= 2;
+ if (bufferpin_wait_us > 1000000)
+ bufferpin_wait_us = 1000000;
+ }
else if (got_standby_deadlock_timeout)
{
/*
@@ -857,7 +873,7 @@ ResolveRecoveryConflictWithBufferPin(void)
* buffer is unpinned or ltime is reached. This would increase the
* workload in the startup process and backends. In practice it may
* not be so harmful because the period that the buffer is kept pinned
- * is basically no so long. But we should fix this?
+ * is basically not so long. But we should fix this?
*/
SendRecoveryConflictWithBufferPin(PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK);
}
diff --git a/src/include/storage/standby.h b/src/include/storage/standby.h
index 24e2f5082bc..a3691a1cac7 100644
--- a/src/include/storage/standby.h
+++ b/src/include/storage/standby.h
@@ -25,6 +25,14 @@ extern PGDLLIMPORT int max_standby_archive_delay;
extern PGDLLIMPORT int max_standby_streaming_delay;
extern PGDLLIMPORT bool log_recovery_conflict_waits;
+/*
+ * After sending a PROCSIG_RECOVERY_CONFLICT_BUFFERPIN, we sleep for this long
+ * to give backends a chance to cancel before checking again if we can acquire
+ * the cleanup lock.
+ */
+#define BUFFERPIN_INITIAL_WAIT_US 1000;
+extern int bufferpin_wait_us;
+
extern void InitRecoveryTransactionEnvironment(void);
extern void ShutdownRecoveryTransactionEnvironment(void);
--
2.50.0.727.gbf7dc18ff4-goog
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored
2025-06-19 00:27 BUG #18961: Race scenario where max_standby_streaming_delay is not honored PG Bug reporting form <noreply@postgresql.org>
2025-06-19 15:17 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Dilip Kumar <dilipbalaut@gmail.com>
2025-06-19 15:58 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Anthony Hsu <erwaman@gmail.com>
2025-06-27 05:05 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Dilip Kumar <dilipbalaut@gmail.com>
2025-06-27 06:11 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Anthony Hsu <erwaman@gmail.com>
2025-06-27 06:26 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Dilip Kumar <dilipbalaut@gmail.com>
2025-06-28 23:37 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Anthony Hsu <erwaman@gmail.com>
@ 2025-06-30 03:28 ` Dilip Kumar <dilipbalaut@gmail.com>
2025-07-10 22:36 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Anthony Hsu <erwaman@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Dilip Kumar @ 2025-06-30 03:28 UTC (permalink / raw)
To: Anthony Hsu <erwaman@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
On Sun, Jun 29, 2025 at 5:07 AM Anthony Hsu <erwaman@gmail.com> wrote:
>
> Made a few changes to my patch and attached a new version. Changes include:
>
> * avoid sending PROCSIG_RECOVERY_CONFLICT_BUFFERPIN in a tight loop after standby limit is reached by adding a small sleep after each send, starting at 1ms and doubling each time up to 1s (similar to what's done in WaitExceedsMaxStandbyDelay here [1]). Sleep time is reset in LockBufferForCleanup once cleanup lock is successfully acquired
> * don't set deadlock timeout once standby limit has passed since after that, the standby timeout will fire immediately, so no need to set deadlock timeout as well
>
> Let me know if you have any thoughts or comments. I am thinking of sending it to pgsql-hackers soon.
I haven't got time to look into your new patch, feel free to start a
thread in pgsql-hackers, I can review it there once I get time,
thanks.
--
Regards,
Dilip Kumar
Google
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored
2025-06-19 00:27 BUG #18961: Race scenario where max_standby_streaming_delay is not honored PG Bug reporting form <noreply@postgresql.org>
2025-06-19 15:17 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Dilip Kumar <dilipbalaut@gmail.com>
2025-06-19 15:58 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Anthony Hsu <erwaman@gmail.com>
2025-06-27 05:05 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Dilip Kumar <dilipbalaut@gmail.com>
2025-06-27 06:11 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Anthony Hsu <erwaman@gmail.com>
2025-06-27 06:26 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Dilip Kumar <dilipbalaut@gmail.com>
2025-06-28 23:37 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Anthony Hsu <erwaman@gmail.com>
2025-06-30 03:28 ` Re: BUG #18961: Race scenario where max_standby_streaming_delay is not honored Dilip Kumar <dilipbalaut@gmail.com>
@ 2025-07-10 22:36 ` Anthony Hsu <erwaman@gmail.com>
0 siblings, 0 replies; 9+ messages in thread
From: Anthony Hsu @ 2025-07-10 22:36 UTC (permalink / raw)
To: Dilip Kumar <dilipbalaut@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
Simplified my patch and posted it on pgsql-hackers here:
https://www.postgresql.org/message-id/flat/CALQc50gi-Kw9m1r6hytf12473-fCECy%3Dq9JtKS4ANeJFEyCBTw%40m...
On Sun, Jun 29, 2025 at 8:28 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> On Sun, Jun 29, 2025 at 5:07 AM Anthony Hsu <erwaman@gmail.com> wrote:
> >
> > Made a few changes to my patch and attached a new version. Changes
> include:
> >
> > * avoid sending PROCSIG_RECOVERY_CONFLICT_BUFFERPIN in a tight loop
> after standby limit is reached by adding a small sleep after each send,
> starting at 1ms and doubling each time up to 1s (similar to what's done in
> WaitExceedsMaxStandbyDelay here [1]). Sleep time is reset in
> LockBufferForCleanup once cleanup lock is successfully acquired
> > * don't set deadlock timeout once standby limit has passed since after
> that, the standby timeout will fire immediately, so no need to set deadlock
> timeout as well
> >
> > Let me know if you have any thoughts or comments. I am thinking of
> sending it to pgsql-hackers soon.
>
> I haven't got time to look into your new patch, feel free to start a
> thread in pgsql-hackers, I can review it there once I get time,
> thanks.
>
>
> --
> Regards,
> Dilip Kumar
> Google
>
^ permalink raw reply [nested|flat] 9+ messages in thread
end of thread, other threads:[~2025-07-10 22:36 UTC | newest]
Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-06-19 00:27 BUG #18961: Race scenario where max_standby_streaming_delay is not honored PG Bug reporting form <noreply@postgresql.org>
2025-06-19 15:17 ` Dilip Kumar <dilipbalaut@gmail.com>
2025-06-19 15:58 ` Anthony Hsu <erwaman@gmail.com>
2025-06-27 05:05 ` Dilip Kumar <dilipbalaut@gmail.com>
2025-06-27 06:11 ` Anthony Hsu <erwaman@gmail.com>
2025-06-27 06:26 ` Dilip Kumar <dilipbalaut@gmail.com>
2025-06-28 23:37 ` Anthony Hsu <erwaman@gmail.com>
2025-06-30 03:28 ` Dilip Kumar <dilipbalaut@gmail.com>
2025-07-10 22:36 ` Anthony Hsu <erwaman@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox