mirror of
https://git.postgresql.org/git/postgresql.git
synced 2024-09-04 21:07:18 +02:00
Fix fallback implementation of pg_atomic_write_u32().
I somehow had assumed that in the spinlock (in turn possibly using semaphores) based fallback atomics implementation 32 bit writes could be done without a lock. As far as the write goes that's correct, since postgres supports only platforms with single-copy atomicity for aligned 32bit writes. But writing without holding the spinlock breaks read-modify-write operations like pg_atomic_compare_exchange_u32(), since they'll potentially "miss" a concurrent write, which can't happen in actual hardware implementations. In 9.6+ when using the fallback atomics implementation this could lead to buffer header locks not being properly marked as released, and potentially some related state corruption. I don't see a related danger in 9.5 (earliest release with the API), because pg_atomic_write_u32() wasn't used in a concurrent manner there. The state variable of local buffers, before this change, were manipulated using pg_atomic_write_u32(), to avoid unnecessary synchronization overhead. As that'd not be the case anymore, introduce and use pg_atomic_unlocked_write_u32(), which does not correctly interact with RMW operations. This bug only caused issues when postgres is compiled on platforms without atomics support (i.e. no common new platform), or when compiled with --disable-atomics, which explains why this wasn't noticed in testing. Reported-By: Tom Lane Discussion: <14947.1475690465@sss.pgh.pa.us> Backpatch: 9.5-, where the atomic operations API was introduced.
This commit is contained in:
parent
00d7e2f8c8
commit
93fd6de141
@ -112,6 +112,19 @@ pg_atomic_init_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 val_)
|
|||||||
ptr->value = val_;
|
ptr->value = val_;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
void
|
||||||
|
pg_atomic_write_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 val)
|
||||||
|
{
|
||||||
|
/*
|
||||||
|
* One might think that an unlocked write doesn't need to acquire the
|
||||||
|
* spinlock, but one would be wrong. Even an unlocked write has to cause a
|
||||||
|
* concurrent pg_atomic_compare_exchange_u32() (et al) to fail.
|
||||||
|
*/
|
||||||
|
SpinLockAcquire((slock_t *) &ptr->sema);
|
||||||
|
ptr->value = val;
|
||||||
|
SpinLockRelease((slock_t *) &ptr->sema);
|
||||||
|
}
|
||||||
|
|
||||||
bool
|
bool
|
||||||
pg_atomic_compare_exchange_u32_impl(volatile pg_atomic_uint32 *ptr,
|
pg_atomic_compare_exchange_u32_impl(volatile pg_atomic_uint32 *ptr,
|
||||||
uint32 *expected, uint32 newval)
|
uint32 *expected, uint32 newval)
|
||||||
|
@ -297,10 +297,12 @@ pg_atomic_read_u32(volatile pg_atomic_uint32 *ptr)
|
|||||||
}
|
}
|
||||||
|
|
||||||
/*
|
/*
|
||||||
* pg_atomic_write_u32 - unlocked write to atomic variable.
|
* pg_atomic_write_u32 - write to atomic variable.
|
||||||
*
|
*
|
||||||
* The write is guaranteed to succeed as a whole, i.e. it's not possible to
|
* The write is guaranteed to succeed as a whole, i.e. it's not possible to
|
||||||
* observe a partial write for any reader.
|
* observe a partial write for any reader. Note that this correctly interacts
|
||||||
|
* with pg_atomic_compare_exchange_u32, in contrast to
|
||||||
|
* pg_atomic_unlocked_write_u32().
|
||||||
*
|
*
|
||||||
* No barrier semantics.
|
* No barrier semantics.
|
||||||
*/
|
*/
|
||||||
@ -312,6 +314,25 @@ pg_atomic_write_u32(volatile pg_atomic_uint32 *ptr, uint32 val)
|
|||||||
pg_atomic_write_u32_impl(ptr, val);
|
pg_atomic_write_u32_impl(ptr, val);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/*
|
||||||
|
* pg_atomic_unlocked_write_u32 - unlocked write to atomic variable.
|
||||||
|
*
|
||||||
|
* The write is guaranteed to succeed as a whole, i.e. it's not possible to
|
||||||
|
* observe a partial write for any reader. But note that writing this way is
|
||||||
|
* not guaranteed to correctly interact with read-modify-write operations like
|
||||||
|
* pg_atomic_compare_exchange_u32. This should only be used in cases where
|
||||||
|
* minor performance regressions due to atomics emulation are unacceptable.
|
||||||
|
*
|
||||||
|
* No barrier semantics.
|
||||||
|
*/
|
||||||
|
static inline void
|
||||||
|
pg_atomic_unlocked_write_u32(volatile pg_atomic_uint32 *ptr, uint32 val)
|
||||||
|
{
|
||||||
|
AssertPointerAlignment(ptr, 4);
|
||||||
|
|
||||||
|
pg_atomic_unlocked_write_u32_impl(ptr, val);
|
||||||
|
}
|
||||||
|
|
||||||
/*
|
/*
|
||||||
* pg_atomic_exchange_u32 - exchange newval with current value
|
* pg_atomic_exchange_u32 - exchange newval with current value
|
||||||
*
|
*
|
||||||
|
@ -135,6 +135,9 @@ pg_atomic_unlocked_test_flag_impl(volatile pg_atomic_flag *ptr)
|
|||||||
#define PG_HAVE_ATOMIC_INIT_U32
|
#define PG_HAVE_ATOMIC_INIT_U32
|
||||||
extern void pg_atomic_init_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 val_);
|
extern void pg_atomic_init_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 val_);
|
||||||
|
|
||||||
|
#define PG_HAVE_ATOMIC_WRITE_U32
|
||||||
|
extern void pg_atomic_write_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 val);
|
||||||
|
|
||||||
#define PG_HAVE_ATOMIC_COMPARE_EXCHANGE_U32
|
#define PG_HAVE_ATOMIC_COMPARE_EXCHANGE_U32
|
||||||
extern bool pg_atomic_compare_exchange_u32_impl(volatile pg_atomic_uint32 *ptr,
|
extern bool pg_atomic_compare_exchange_u32_impl(volatile pg_atomic_uint32 *ptr,
|
||||||
uint32 *expected, uint32 newval);
|
uint32 *expected, uint32 newval);
|
||||||
|
@ -60,6 +60,15 @@ pg_atomic_write_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 val)
|
|||||||
}
|
}
|
||||||
#endif
|
#endif
|
||||||
|
|
||||||
|
#ifndef PG_HAVE_ATOMIC_UNLOCKED_WRITE_U32
|
||||||
|
#define PG_HAVE_ATOMIC_UNLOCKED_WRITE_U32
|
||||||
|
static inline void
|
||||||
|
pg_atomic_unlocked_write_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 val)
|
||||||
|
{
|
||||||
|
ptr->value = val;
|
||||||
|
}
|
||||||
|
#endif
|
||||||
|
|
||||||
/*
|
/*
|
||||||
* provide fallback for test_and_set using atomic_exchange if available
|
* provide fallback for test_and_set using atomic_exchange if available
|
||||||
*/
|
*/
|
||||||
|
Loading…
Reference in New Issue
Block a user