perform_spin_delay() vs wait events

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

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

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