postgres.git / summary / log / commit / refs

commit    6b93fcd149329d4ee7319561b30fc15a573c6307
Author:   Andres Freund <andres@anarazel.de>
Date:     Wed Apr 13 22:28:29 2016 +0000
First released in: 9.6.0

    Avoid atomic operation in MarkLocalBufferDirty().
    
    The recent patch to make Pin/UnpinBuffer lockfree in the hot
    path (48354581a), accidentally used pg_atomic_fetch_or_u32() in
    MarkLocalBufferDirty(). Other code operating on local buffers was
    careful to only use pg_atomic_read/write_u32 which just read/write from
    memory; to avoid unnecessary overhead.
    
    On its own that'd just make MarkLocalBufferDirty() slightly less
    efficient, but in addition InitLocalBuffers() doesn't call
    pg_atomic_init_u32() - thus the spinlock fallback for the atomic
    operations isn't initialized. That in turn caused, as reported by Tom,
    buildfarm animal gaur to fail.  As those errors are actually useful
    against this type of error, continue to omit - intentionally this time -
    initialization of the atomic variable.
    
    In addition, add an explicit note about only using pg_atomic_read/write
    on local buffers's state to BufferDesc's description.
    
    Reported-By: Tom Lane
    Discussion: 1881.1460431476@sss.pgh.pa.us


 src/backend/storage/buffer/localbuf.c | 13 ++++++++++++-
 src/include/storage/buf_internals.h   |  6 ++++--
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c
index 68b402023a1..201ce2668fd 100644
--- a/src/backend/storage/buffer/localbuf.c
+++ b/src/backend/storage/buffer/localbuf.c
@@ -294,10 +294,14 @@ MarkLocalBufferDirty(Buffer buffer)
 
 	bufHdr = GetLocalBufferDescriptor(bufid);
 
-	buf_state = pg_atomic_fetch_or_u32(&bufHdr->state, BM_DIRTY);
+	buf_state = pg_atomic_read_u32(&bufHdr->state);
 
 	if (!(buf_state & BM_DIRTY))
 		pgBufferUsage.local_blks_dirtied++;
+
+	buf_state |= BM_DIRTY;
+
+	pg_atomic_write_u32(&bufHdr->state, buf_state);
 }
 
 /*
@@ -431,6 +435,13 @@ InitLocalBuffers(void)
 		 * is -1.)
 		 */
 		buf->buf_id = -i - 2;
+
+		/*
+		 * Intentionally do not initialize the buffer's atomic variable
+		 * (besides zeroing the underlying memory above). That way we get
+		 * errors on platforms without atomics, if somebody (re-)introduces
+		 * atomic operations for local buffers.
+		 */
 	}
 
 	/* Create the lookup hash table */
diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h
index f8f71255fdd..c9cae956a54 100644
--- a/src/include/storage/buf_internals.h
+++ b/src/include/storage/buf_internals.h
@@ -165,8 +165,10 @@ typedef struct buftag
  * wait_backend_pid and setting flag bit BM_PIN_COUNT_WAITER.  At present,
  * there can be only one such waiter per buffer.
  *
- * We use this same struct for local buffer headers, but the lock fields
- * are not used and not all of the flag bits are useful either.
+ * We use this same struct for local buffer headers, but the locks are not
+ * used and not all of the flag bits are useful either. To avoid unnecessary
+ * overhead, manipulations of the state field should be done without actual
+ * atomic operations (i.e. only pg_atomic_read/write).
  *
  * Be careful to avoid increasing the size of the struct when adding or
  * reordering members.  Keeping it below 64 bytes (the most common CPU


[parent: 95ef43c43081]