Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 6b93fcd

Browse files
committed
Avoid atomic operation in MarkLocalBufferDirty().
The recent patch to make Pin/UnpinBuffer lockfree in the hot path (4835458), 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
1 parent 95ef43c commit 6b93fcd

File tree

2 files changed

+16
-3
lines changed

2 files changed

+16
-3
lines changed

src/backend/storage/buffer/localbuf.c

+12-1
Original file line numberDiff line numberDiff line change
@@ -294,10 +294,14 @@ MarkLocalBufferDirty(Buffer buffer)
294294

295295
bufHdr = GetLocalBufferDescriptor(bufid);
296296

297-
buf_state = pg_atomic_fetch_or_u32(&bufHdr->state, BM_DIRTY);
297+
buf_state = pg_atomic_read_u32(&bufHdr->state);
298298

299299
if (!(buf_state & BM_DIRTY))
300300
pgBufferUsage.local_blks_dirtied++;
301+
302+
buf_state |= BM_DIRTY;
303+
304+
pg_atomic_write_u32(&bufHdr->state, buf_state);
301305
}
302306

303307
/*
@@ -431,6 +435,13 @@ InitLocalBuffers(void)
431435
* is -1.)
432436
*/
433437
buf->buf_id = -i - 2;
438+
439+
/*
440+
* Intentionally do not initialize the buffer's atomic variable
441+
* (besides zeroing the underlying memory above). That way we get
442+
* errors on platforms without atomics, if somebody (re-)introduces
443+
* atomic operations for local buffers.
444+
*/
434445
}
435446

436447
/* Create the lookup hash table */

src/include/storage/buf_internals.h

+4-2
Original file line numberDiff line numberDiff line change
@@ -165,8 +165,10 @@ typedef struct buftag
165165
* wait_backend_pid and setting flag bit BM_PIN_COUNT_WAITER. At present,
166166
* there can be only one such waiter per buffer.
167167
*
168-
* We use this same struct for local buffer headers, but the lock fields
169-
* are not used and not all of the flag bits are useful either.
168+
* We use this same struct for local buffer headers, but the locks are not
169+
* used and not all of the flag bits are useful either. To avoid unnecessary
170+
* overhead, manipulations of the state field should be done without actual
171+
* atomic operations (i.e. only pg_atomic_read/write).
170172
*
171173
* Be careful to avoid increasing the size of the struct when adding or
172174
* reordering members. Keeping it below 64 bytes (the most common CPU

0 commit comments

Comments
 (0)