Hi,
On 2022-11-09 15:54:16 +0530, Bharath Rupireddy wrote:
> On Tue, Nov 1, 2022 at 12:46 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > > Updated patch attached.
> >
> > Thanks. It looks good to me. However, some minor comments on the v3 patch:
> >
> > 1.
> > - if (MyProc->lwWaiting)
> > + if (MyProc->lwWaiting != LW_WS_NOT_WAITING)
> > elog(PANIC, "queueing for lock while waiting on another one");
> >
> > Can the above condition be MyProc->lwWaiting == LW_WS_WAITING ||
> > MyProc->lwWaiting == LW_WS_PENDING_WAKEUP for better readability?
> >
> > Or add an assertion Assert(MyProc->lwWaiting != LW_WS_WAITING &&
> > MyProc->lwWaiting != LW_WS_PENDING_WAKEUP); before setting
> > LW_WS_WAITING?
I don't think that's a good idea - it'll just mean we have to modify more
places if we add another state, without making anything more robust.
> > 2.
> > /* Awaken any waiters I removed from the queue. */
> > proclist_foreach_modify(iter, &wakeup, lwWaitLink)
> > {
> >
> > @@ -1044,7 +1052,7 @@ LWLockWakeup(LWLock *lock)
> > * another lock.
> > */
> > pg_write_barrier();
> > - waiter->lwWaiting = false;
> > + waiter->lwWaiting = LW_WS_NOT_WAITING;
> > PGSemaphoreUnlock(waiter->sem);
> > }
> >
> > /*
> > * Awaken any waiters I removed from the queue.
> > */
> > proclist_foreach_modify(iter, &wakeup, lwWaitLink)
> > {
> > PGPROC *waiter = GetPGProcByNumber(iter.cur);
> >
> > proclist_delete(&wakeup, iter.cur, lwWaitLink);
> > /* check comment in LWLockWakeup() about this barrier */
> > pg_write_barrier();
> > waiter->lwWaiting = LW_WS_NOT_WAITING;
> >
> > Can we add an assertion Assert(waiter->lwWaiting ==
> > LW_WS_PENDING_WAKEUP) in the above two places? We prepare the wakeup
> > list and set the LW_WS_NOT_WAITING flag in above loops, having an
> > assertion is better here IMO.
I guess it can't hurt - but it's not really related to the changes in the
patch, no?
> I looked at the v3 patch again today and ran some performance tests.
> The results look impressive as they were earlier. Andres, any plans to
> get this in?
I definitely didn't want to backpatch before this point release. But it seems
we haven't quite got to an agreement what to do about backpatching. It's
probably best to just commit it to HEAD and let the backpatch discussion
happen concurrently.
I'm on a hike, without any connectivity, Thu afternoon - Sun. I think it's OK
to push it to HEAD if I get it done in the next few hours. Bigger issues,
which I do not expect, should show up before tomorrow afternoon. Smaller
things could wait till Sunday if necessary.
Greetings,
Andres Freund
