Thread: perform_spin_delay() vs wait events
Hi, The lwlock wait queue scalability issue [1] was quite hard to find because of the exponential backoff and because we adjust spins_per_delay over time within a backend. I think the least we could do to make this easier would be to signal spin delays as wait events. We surely don't want to do so for non-contended spins because of the overhead, but once we get to the point of calling pg_usleep() that's not an issue. I don't think it's worth trying to hand down more detailed information about the specific spinlock we're waiting on, at least for now. We'd have to invent a whole lot of new wait events because most spinlocks don't have ones yet. I couldn't quite decide what wait_event_type to best group this under? In the attached patch I put it under timeouts, which doesn't seem awful. I reverted a4adc31f690 and indeed it shows SpinDelay wait events before the fix and not after. Greetings, Andres Freund [1] https://postgr.es/m/20221027165914.2hofzp4cvutj6gin%40awork3.anarazel.de
Attachment
On Sun, Nov 20, 2022 at 3:43 PM Andres Freund <andres@anarazel.de> wrote: > The lwlock wait queue scalability issue [1] was quite hard to find because of > the exponential backoff and because we adjust spins_per_delay over time within > a backend. > > I think the least we could do to make this easier would be to signal spin > delays as wait events. We surely don't want to do so for non-contended spins > because of the overhead, but once we get to the point of calling pg_usleep() > that's not an issue. > > I don't think it's worth trying to hand down more detailed information about > the specific spinlock we're waiting on, at least for now. We'd have to invent > a whole lot of new wait events because most spinlocks don't have ones yet. > > I couldn't quite decide what wait_event_type to best group this under? In the > attached patch I put it under timeouts, which doesn't seem awful. I think it would be best to make it its own category, like we do with buffer pins. -- Robert Haas EDB: http://www.enterprisedb.com
Hi, On 2022-11-20 17:26:11 -0500, Robert Haas wrote: > On Sun, Nov 20, 2022 at 3:43 PM Andres Freund <andres@anarazel.de> wrote: > > I couldn't quite decide what wait_event_type to best group this under? In the > > attached patch I put it under timeouts, which doesn't seem awful. > > I think it would be best to make it its own category, like we do with > buffer pins. I was wondering about that too - but decided against it because it would only show a single wait event. And wouldn't really describe spinlocks as a whole, just the "extreme" delays. If we wanted to report the spin waits more granular, we'd presumably have to fit the wait events into the lwlock, buffers and some new category where we name individual spinlocks. But I guess a single spinlock wait event type with an ExponentialBackoff wait event or such wouldn't be too bad. Greetings, Andres Freund
On Sun, Nov 20, 2022 at 6:10 PM Andres Freund <andres@anarazel.de> wrote: > I was wondering about that too - but decided against it because it would only > show a single wait event. And wouldn't really describe spinlocks as a whole, > just the "extreme" delays. If we wanted to report the spin waits more > granular, we'd presumably have to fit the wait events into the lwlock, buffers > and some new category where we name individual spinlocks. > > But I guess a single spinlock wait event type with an ExponentialBackoff wait > event or such wouldn't be too bad. Oh, hmm. I guess it is actually bracketing a timed wait, now that I look closer at what you did. So perhaps your first idea was best after all. -- Robert Haas EDB: http://www.enterprisedb.com
On Mon, Nov 21, 2022 at 2:10 AM Andres Freund <andres@anarazel.de> wrote: > On 2022-11-20 17:26:11 -0500, Robert Haas wrote: > > On Sun, Nov 20, 2022 at 3:43 PM Andres Freund <andres@anarazel.de> wrote: > > > I couldn't quite decide what wait_event_type to best group this under? In the > > > attached patch I put it under timeouts, which doesn't seem awful. > > > > I think it would be best to make it its own category, like we do with > > buffer pins. > > I was wondering about that too - but decided against it because it would only > show a single wait event. And wouldn't really describe spinlocks as a whole, > just the "extreme" delays. If we wanted to report the spin waits more > granular, we'd presumably have to fit the wait events into the lwlock, buffers > and some new category where we name individual spinlocks. +1 for making a group of individual names spin delays. ------ Regards, Alexander Korotkov
Hi, On November 21, 2022 12:58:16 PM PST, Alexander Korotkov <aekorotkov@gmail.com> wrote: >On Mon, Nov 21, 2022 at 2:10 AM Andres Freund <andres@anarazel.de> wrote: >> On 2022-11-20 17:26:11 -0500, Robert Haas wrote: >> > On Sun, Nov 20, 2022 at 3:43 PM Andres Freund <andres@anarazel.de> wrote: >> > > I couldn't quite decide what wait_event_type to best group this under? In the >> > > attached patch I put it under timeouts, which doesn't seem awful. >> > >> > I think it would be best to make it its own category, like we do with >> > buffer pins. >> >> I was wondering about that too - but decided against it because it would only >> show a single wait event. And wouldn't really describe spinlocks as a whole, >> just the "extreme" delays. If we wanted to report the spin waits more >> granular, we'd presumably have to fit the wait events into the lwlock, buffers >> and some new category where we name individual spinlocks. > >+1 for making a group of individual names spin delays. Personally I'm not interested in doing that work, tbh. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Tue, Nov 22, 2022 at 12:01 AM Andres Freund <andres@anarazel.de> wrote: > On November 21, 2022 12:58:16 PM PST, Alexander Korotkov <aekorotkov@gmail.com> wrote: > >On Mon, Nov 21, 2022 at 2:10 AM Andres Freund <andres@anarazel.de> wrote: > >> On 2022-11-20 17:26:11 -0500, Robert Haas wrote: > >> > On Sun, Nov 20, 2022 at 3:43 PM Andres Freund <andres@anarazel.de> wrote: > >> > > I couldn't quite decide what wait_event_type to best group this under? In the > >> > > attached patch I put it under timeouts, which doesn't seem awful. > >> > > >> > I think it would be best to make it its own category, like we do with > >> > buffer pins. > >> > >> I was wondering about that too - but decided against it because it would only > >> show a single wait event. And wouldn't really describe spinlocks as a whole, > >> just the "extreme" delays. If we wanted to report the spin waits more > >> granular, we'd presumably have to fit the wait events into the lwlock, buffers > >> and some new category where we name individual spinlocks. > > > >+1 for making a group of individual names spin delays. > > Personally I'm not interested in doing that work, tbh. Oh, then I have no objection to the "as is" state, because it doesn't exclude the future improvements. But this is still my 2 cents though. ------ Regards, Alexander Korotkov
Hi, On 2022-11-22 00:03:23 +0300, Alexander Korotkov wrote: > On Tue, Nov 22, 2022 at 12:01 AM Andres Freund <andres@anarazel.de> wrote: > > On November 21, 2022 12:58:16 PM PST, Alexander Korotkov <aekorotkov@gmail.com> wrote: > > >On Mon, Nov 21, 2022 at 2:10 AM Andres Freund <andres@anarazel.de> wrote: > > >+1 for making a group of individual names spin delays. > > > > Personally I'm not interested in doing that work, tbh. > > Oh, then I have no objection to the "as is" state, because it doesn't > exclude the future improvements. But this is still my 2 cents though. I added a note about possibly extending this in the future to both code and commit message. Attached. I plan to push this soon unless somebody has further comments. Greetings, Andres Freund
Attachment
On Mon, Nov 21, 2022 at 07:01:18PM -0800, Andres Freund wrote: > I plan to push this soon unless somebody has further comments. > @@ -146,7 +146,8 @@ typedef enum > WAIT_EVENT_RECOVERY_RETRIEVE_RETRY_INTERVAL, > WAIT_EVENT_REGISTER_SYNC_REQUEST, > WAIT_EVENT_VACUUM_DELAY, > - WAIT_EVENT_VACUUM_TRUNCATE > + WAIT_EVENT_VACUUM_TRUNCATE, > + WAIT_EVENT_SPIN_DELAY > } WaitEventTimeout; That would be fine for stable branches, but could you keep that in an alphabetical order on HEAD? -- Michael
Attachment
Hi, On 2022-11-22 12:51:25 +0900, Michael Paquier wrote: > On Mon, Nov 21, 2022 at 07:01:18PM -0800, Andres Freund wrote: > > I plan to push this soon unless somebody has further comments. > > > @@ -146,7 +146,8 @@ typedef enum > > WAIT_EVENT_RECOVERY_RETRIEVE_RETRY_INTERVAL, > > WAIT_EVENT_REGISTER_SYNC_REQUEST, > > WAIT_EVENT_VACUUM_DELAY, > > - WAIT_EVENT_VACUUM_TRUNCATE > > + WAIT_EVENT_VACUUM_TRUNCATE, > > + WAIT_EVENT_SPIN_DELAY > > } WaitEventTimeout; > > That would be fine for stable branches, but could you keep that in an > alphabetical order on HEAD? Fair point. I wasn't planning to backpatch. Greetings, Andres Freund