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.

diff --git a/src/backend/port/atomics.c b/src/backend/port/atomics.c
index 439b3c18c7b26ad3aa062bf1ca3cbac00b0b1438..aa15df5a47ab2011057a4dc038a5bb4b83b4d450 100644 (file)
--- a/src/backend/port/atomics.c
+++ b/src/backend/port/atomics.c
@@ -112,6 +112,19 @@ pg_atomic_init_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 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
 pg_atomic_compare_exchange_u32_impl(volatile pg_atomic_uint32 *ptr,
                                    uint32 *expected, uint32 newval)

diff --git a/src/include/port/atomics.h b/src/include/port/atomics.h
index d94fea60f150300e74e39c04429c896b269171b8..0b506fcbc0b19ad0233554bb9a50aa751bd27b19 100644 (file)
--- a/src/include/port/atomics.h
+++ b/src/include/port/atomics.h
@@ -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
- * 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.
  */
@@ -312,6 +314,25 @@ pg_atomic_write_u32(volatile pg_atomic_uint32 *ptr, uint32 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
  *

diff --git a/src/include/port/atomics/fallback.h b/src/include/port/atomics/fallback.h
index 4e04f9758b971ac8a97d727c653e7d9ef8ea17b9..a1380034ba98539dc6f70f5f85935938536a5dda 100644 (file)
--- a/src/include/port/atomics/fallback.h
+++ b/src/include/port/atomics/fallback.h
@@ -135,6 +135,9 @@ pg_atomic_unlocked_test_flag_impl(volatile pg_atomic_flag *ptr)
 #define PG_HAVE_ATOMIC_INIT_U32
 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
 extern bool pg_atomic_compare_exchange_u32_impl(volatile pg_atomic_uint32 *ptr,
                                                uint32 *expected, uint32 newval);

diff --git a/src/include/port/atomics/generic.h b/src/include/port/atomics/generic.h
index bb31df3623715f2356a1f647f39c2cf4a493e219..ec140e4a1e031b9ae19a1c03f9ecc976c2ea8a4c 100644 (file)
--- a/src/include/port/atomics/generic.h
+++ b/src/include/port/atomics/generic.h
@@ -60,6 +60,15 @@ pg_atomic_write_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 val)
 }
 #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
  */