bufmgr: Fix ordering of checks in PinBuffer()

The check for skip_if_not_valid added in 819dc118c0f6 was put at the start of
the loop. A CAS loop in theory does allow to make that check in a race free
manner. However, just after the check, there's a
    old_buf_state = WaitBufHdrUnlocked(buf);
which introduces a race, because it would allow BM_VALID to be cleared, after
the skip_if_not_valid check.

Fix by restarting the loop after WaitBufHdrUnlocked().

Reported-by: Yura Sokolov <y.sokolov@postgrespro.ru>
Discussion: https://postgr.es/m/5bf667f3-5270-4b19-a08f-0facbecdff68@postgrespro.ru

diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index cd21ae3fc36d64ce997672e6e00a4db89da05487..17499451ad2e7c9ec3c18311e520377a76c89a61 100644 (file)
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -3281,9 +3281,14 @@ PinBuffer(BufferDesc *buf, BufferAccessStrategy strategy,
                         * We're not allowed to increase the refcount while the buffer
                         * header spinlock is held. Wait for the lock to be released.
                         */
-                       if (old_buf_state & BM_LOCKED)
+                       if (unlikely(old_buf_state & BM_LOCKED))
+                       {
                                old_buf_state = WaitBufHdrUnlocked(buf);
 
+                               /* perform checks at the top of the loop again */
+                               continue;
+                       }
+
                        buf_state = old_buf_state;
 
                        /* increase refcount */