From: Andres Freund Date: Fri, 10 Jul 2026 17:25:13 +0000 (-0400) Subject: bufmgr: Fix order of operations in UnlockBufHdrExt X-Git-Tag: REL_19_BETA2~4 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=a1b74b7279c24638c44f9d3d1af2da30cfb7068d;p=thirdparty%2Fpostgresql.git bufmgr: Fix order of operations in UnlockBufHdrExt In c75ebc657ffc I (Andres) introduced UnlockBufHdrExt() which can set and clear bits in the buffer state using CAS. Unfortunately I added bits before subtracting them, which means that a bit that was both removed and set would remain unset. Fix the order of operations. The only known case where that is a problem is that BM_IO_ERROR would not actually remain set. It's unfortunately not trivial to add a decent, race-free, test to verify that BM_IO_ERROR remains set. That's therefore left for the 20 cycle. Reported-by: Yura Sokolov Discussion: https://postgr.es/m/ab0dcc9e-aba0-44e3-ac23-8d74c48888e6@postgrespro.ru Backpatch-through: 19, where c75ebc657ffc went in --- diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h index 89615a254a3..a23abf14f8f 100644 --- a/src/include/storage/buf_internals.h +++ b/src/include/storage/buf_internals.h @@ -463,12 +463,13 @@ UnlockBufHdr(BufferDesc *desc) } /* - * Unlock the buffer header, while atomically adding the flags in set_bits, - * unsetting the ones in unset_bits and changing the refcount by - * refcount_change. + * Unlock the buffer header, while atomically unsetting the ones in + * unset_bits, adding the flags in set_bits, and changing the refcount by + * refcount_change. If a flag is both cleared and added, it will end up being + * set. * - * Note that this approach would not work for usagecount, since we need to cap - * the usagecount at BM_MAX_USAGE_COUNT. + * Note that this approach would not trivially work for usagecount, since we + * need to cap the usagecount at BM_MAX_USAGE_COUNT. */ static inline uint64 UnlockBufHdrExt(BufferDesc *desc, uint64 old_buf_state, @@ -481,8 +482,12 @@ UnlockBufHdrExt(BufferDesc *desc, uint64 old_buf_state, Assert(buf_state & BM_LOCKED); - buf_state |= set_bits; + /* + * Set bits after clearing bits, so that a cleared and re-added flag + * survives. + */ buf_state &= ~unset_bits; + buf_state |= set_bits; buf_state &= ~BM_LOCKED; if (refcount_change != 0)