On Thu, Feb 9, 2023 at 3:36 AM Nathan Bossart <nathandboss...@gmail.com> wrote: > > + pg_atomic_exchange_u64(valptr, val); > > nitpick: I'd add a (void) at the beginning of these calls to > pg_atomic_exchange_u64() so that it's clear that we are discarding the > return value.
I did that in the attached v5 patch although it's a mix elsewhere; some doing explicit return value cast with (void) and some not. > + /* > + * Update the lock variable atomically first without having to > acquire wait > + * list lock, so that if anyone looking for the lock will have chance > to > + * grab it a bit quickly. > + * > + * NB: Note the use of pg_atomic_exchange_u64 as opposed to just > + * pg_atomic_write_u64 to update the value. Since > pg_atomic_exchange_u64 is > + * a full barrier, we're guaranteed that the subsequent atomic read > of lock > + * state to check if it has any waiters happens after we set the lock > + * variable to new value here. Without a barrier, we could end up > missing > + * waiters that otherwise should have been woken up. > + */ > + pg_atomic_exchange_u64(valptr, val); > + > + /* > + * Quick exit when there are no waiters. This avoids unnecessary > lwlock's > + * wait list lock acquisition and release. > + */ > + if ((pg_atomic_read_u32(&lock->state) & LW_FLAG_HAS_WAITERS) == 0) > + return; > > I think this makes sense. A waiter could queue itself after the exchange, > but it'll recheck after queueing. IIUC this is basically how this works > today. We update the value and release the lock before waking up any > waiters, so the same principle applies. Yes, a waiter right after self-queuing (LWLockQueueSelf) checks for the value (LWLockConflictsWithVar) before it goes and waits until awakened in LWLockWaitForVar. A waiter added to the queue is guaranteed to be woken up by the LWLockUpdateVar but before that the lock value is set and we have pg_atomic_exchange_u64 as a memory barrier, so no memory reordering. Essentially, the order of these operations aren't changed. The benefit that we're seeing is from avoiding LWLock's waitlist lock for reading and updating the lock value relying on 64-bit atomics. > Overall, I think this patch is in reasonable shape. Thanks for reviewing. Please see the attached v5 patch. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
v5-0001-Optimize-WAL-insertion-lock-acquisition-and-relea.patch
Description: Binary data
