upstream/postgresql
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2026-07-07 00:30:57 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions

bufmgr: Fix race in LockBufferForCleanup()

Browse source 
LockBufferForCleanup() acquires the exclusive content lock, checks the
buffer's shared pin count, and, if other pins remain, registers itself
as the BM_PIN_COUNT_WAITER before waiting for an unpin notification.

Since commits 5310fac6e0 and c75ebc657f, however, a shared buffer
pin can be released while BM_LOCKED is set, introducing the following
race:

- LockBufferForCleanup() observes a refcount greater than one.
- Before it sets BM_PIN_COUNT_WAITER, another backend releases the
  last conflicting pin.
- Since BM_PIN_COUNT_WAITER is not yet set, no wakeup is sent.
- LockBufferForCleanup() then sets BM_PIN_COUNT_WAITER and goes to
  sleep, even though only its own pin remains.

As a result, LockBufferForCleanup() can sleep indefinitely because
the wakeup corresponding to the last conflicting unpin has already been
missed.

Fix this by setting BM_PIN_COUNT_WAITER while holding the buffer
header lock, then rechecking the refcount before releasing the content
lock. If only our pin remains, clear the waiter state and proceed
without sleeping. Otherwise, wait as before.

This issue was reported by buildfarm member skink, where it manifested
as intermittent timeouts in 048_vacuum_horizon_floor.pl.

Backpatch to v19, where commits 5310fac6e0 and c75ebc657f
introduced the race.

Reported-by: Alexander Lakhin <exclusion@gmail.com>
Author: Xuneng Zhou <xunengzhou@gmail.com>
Reviewed-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/7685519a-0bf9-4e17-93ca-7e3aa10fa29c@gmail.com
Backpatch-through: 19
This commit is contained in:
Fujii Masao 2026-06-30 10:30:47 +09:00
parent d8113095c4
commit 8d85cb889a
1 changed files with 47 additions and 21 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

68
src/backend/storage/buffer/bufmgr.c
View file

@ -6715,24 +6715,7 @@ LockBufferForCleanup(Buffer buffer)
{
/* Successfully acquired exclusive lock with pincount 1 */
UnlockBufHdr(bufHdr);
/*
* Emit the log message if recovery conflict on buffer pin was
* resolved but the startup process waited longer than
* deadlock_timeout for it.
*/
if (logged_recovery_conflict)
LogRecoveryConflict(RECOVERY_CONFLICT_BUFFERPIN,
waitStart, GetCurrentTimestamp(),
NULL, false);
if (waiting)
{
/* reset ps display to remove the suffix if we added one */
set_ps_display_remove_suffix();
waiting = false;
}
return;
goto cleanup_lock_acquired;
}
/* Failed, so mark myself as waiting for pincount 1 */
if (buf_state & BM_PIN_COUNT_WAITER)
@ -6743,9 +6726,32 @@ LockBufferForCleanup(Buffer buffer)
}
bufHdr->wait_backend_pgprocno = MyProcNumber;
PinCountWaitBuf = bufHdr;
UnlockBufHdrExt(bufHdr, buf_state,
BM_PIN_COUNT_WAITER, 0,
0);
/*
* Publish BM_PIN_COUNT_WAITER while retaining the buffer header lock.
* The shared refcount can be decremented while BM_LOCKED is set, so
* use an atomic operation that preserves concurrent refcount changes.
*/
pg_atomic_fetch_or_u64(&bufHdr->state, BM_PIN_COUNT_WAITER);
/*
* Recheck the refcount after publishing the waiter flag, while shared
* refcount increments are still prevented by BM_LOCKED. If only our
* pin remains, the cleanup-lock condition has already been satisfied,
* so remove the waiter state and return without sleeping.
*/
buf_state = pg_atomic_read_u64(&bufHdr->state);
if (BUF_STATE_GET_REFCOUNT(buf_state) == 1)
{
UnlockBufHdrExt(bufHdr, buf_state,
0, BM_PIN_COUNT_WAITER,
0);
PinCountWaitBuf = NULL;
goto cleanup_lock_acquired;
}
UnlockBufHdr(bufHdr);
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
/* Wait to be signaled by UnpinBuffer() */
@ -6816,6 +6822,26 @@ LockBufferForCleanup(Buffer buffer)
PinCountWaitBuf = NULL;
/* Loop back and try again */
}
cleanup_lock_acquired:
/*
* Emit the log message if recovery conflict on buffer pin was resolved
* but the startup process waited longer than deadlock_timeout for it.
*/
if (logged_recovery_conflict)
LogRecoveryConflict(RECOVERY_CONFLICT_BUFFERPIN,
waitStart, GetCurrentTimestamp(),
NULL, false);
if (waiting)
{
/* reset ps display to remove the suffix if we added one */
set_ps_display_remove_suffix();
waiting = false;
}
return;
}
/*