On Sun, Feb 27, 2022 at 10:29 PM Julien Rouhaud <rjuju...@gmail.com> wrote: > On Sun, Feb 27, 2022 at 06:10:45PM +0900, Michael Paquier wrote: > > On Sat, Feb 26, 2022 at 01:39:42PM -0800, Andres Freund wrote: > > > I suspect the easiest is to just convert that usleep to a WaitLatch(). > > > That'd > > > require adding a new enum value to WaitEventTimeout in 14. Which probably > > > is > > > fine? > > > > We've added wait events in back-branches in the past, so this does not > > strike me as a problem as long as you add the new entry at the end of > > the enum, while keeping things ordered on HEAD. > > +1
+1 Sleeps like these are also really bad for ProcSignalBarrier, which I was expecting to be the impetus for fixing any remaining loops like this. With the attached, 027_stream_regress.pl drops from ~29.5s to ~19.6s on my FreeBSD workstation! It seems a little strange to introduce a new wait event that will very often appear into a stable branch, but ... it is actually telling the truth, so there is that. The sleep/poll loop in RegisterSyncRequest() may also have another problem. The comment explains that it was a deliberate choice not to do CHECK_FOR_INTERRUPTS() here, which may be debatable, but I don't think there's an excuse to ignore postmaster death in a loop that presumably becomes infinite if the checkpointer exits. I guess we could do: - pg_usleep(10000L); + WaitLatch(NULL, WL_EXIT_ON_PM_DEATH | WL_TIMEOUT, 10, WAIT_EVENT_SYNC_REQUEST); But... really, this should be waiting on a condition variable that the checkpointer broadcasts on when the queue goes from full to not full, no? Perhaps for master only?
From 23abdf95dea74b914dc56a46739a63ff86035f51 Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.mu...@gmail.com> Date: Mon, 28 Feb 2022 11:27:05 +1300 Subject: [PATCH] Wake up for latches in CheckpointWriteDelay(). The checkpointer shouldn't ignore its latch. Other backends may be waiting for it to drain the request queue. Hopefully real systems don't have a full queue often, but the condition is reached easily when shared buffers is very small. This involves defining a new wait event, which will appear in the pg_stat_activity view often due to spread checkpoints. Back-patch only to 14. Even though the problem exists in earlier branches too, it's hard to hit there. In 14 we stopped using signal handers for latches on Linux, *BSD and macOS, which were previously hiding this problem by interrupting the sleep (though not reliably, as the signal could arrive before the sleep begins; precisely the problem latches address). Reported-by: Andres Freund <and...@anarazel.de> Discussion: https://postgr.es/m/20220226213942.nb7uvb2pamyu26dj%40alap3.anarazel.de --- doc/src/sgml/monitoring.sgml | 4 ++++ src/backend/postmaster/checkpointer.c | 5 ++++- src/backend/utils/activity/wait_event.c | 3 +++ src/include/utils/wait_event.h | 1 + 4 files changed, 12 insertions(+), 1 deletion(-) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index bf7625d988..9dc3978f35 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -2236,6 +2236,10 @@ postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34 0:00 postgres: ser <entry><literal>BaseBackupThrottle</literal></entry> <entry>Waiting during base backup when throttling activity.</entry> </row> + <row> + <entry><literal>CheckpointerWriteDelay</literal></entry> + <entry>Waiting between writes while performing a checkpoint.</entry> + </row> <row> <entry><literal>PgSleep</literal></entry> <entry>Waiting due to a call to <function>pg_sleep</function> or diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c index 4488e3a443..9d9aad166e 100644 --- a/src/backend/postmaster/checkpointer.c +++ b/src/backend/postmaster/checkpointer.c @@ -726,7 +726,10 @@ CheckpointWriteDelay(int flags, double progress) * Checkpointer and bgwriter are no longer related so take the Big * Sleep. */ - pg_usleep(100000L); + WaitLatch(MyLatch, WL_LATCH_SET | WL_EXIT_ON_PM_DEATH | WL_TIMEOUT, + 100, + WAIT_EVENT_CHECKPOINT_WRITE_DELAY); + ResetLatch(MyLatch); } else if (--absorb_counter <= 0) { diff --git a/src/backend/utils/activity/wait_event.c b/src/backend/utils/activity/wait_event.c index 60972c3a75..0706e922b5 100644 --- a/src/backend/utils/activity/wait_event.c +++ b/src/backend/utils/activity/wait_event.c @@ -485,6 +485,9 @@ pgstat_get_wait_timeout(WaitEventTimeout w) case WAIT_EVENT_BASE_BACKUP_THROTTLE: event_name = "BaseBackupThrottle"; break; + case WAIT_EVENT_CHECKPOINT_WRITE_DELAY: + event_name = "CheckpointWriteDelay"; + break; case WAIT_EVENT_PG_SLEEP: event_name = "PgSleep"; break; diff --git a/src/include/utils/wait_event.h b/src/include/utils/wait_event.h index 395d325c5f..d0345c6b49 100644 --- a/src/include/utils/wait_event.h +++ b/src/include/utils/wait_event.h @@ -141,6 +141,7 @@ typedef enum typedef enum { WAIT_EVENT_BASE_BACKUP_THROTTLE = PG_WAIT_TIMEOUT, + WAIT_EVENT_CHECKPOINT_WRITE_DELAY, WAIT_EVENT_PG_SLEEP, WAIT_EVENT_RECOVERY_APPLY_DELAY, WAIT_EVENT_RECOVERY_RETRIEVE_RETRY_INTERVAL, -- 2.35.1
