On Mon, Nov 16, 2020 at 8:56 PM Michael Paquier <mich...@paquier.xyz> wrote: > On Thu, Sep 24, 2020 at 05:55:17PM +1200, Thomas Munro wrote: > > Right, RestoreArchivedFile() uses system(), so I guess it can hang > > around for a long time after unexpected postmaster exit on every OS if > > the command waits. To respond to various kinds of important > > interrupts, I suppose that'd ideally use something like > > OpenPipeStream() and a typical WaitLatch() loop with CFI(). I'm not > > sure what our policy is or should be for exiting while we have running > > subprocesses. I guess that is a separate issue. > > - if (IsUnderPostmaster && !PostmasterIsAlive()) > + if (IsUnderPostmaster && > +#ifndef USE_POSTMASTER_DEATH_SIGNAL > + count++ % 1024 == 0 && > +#endif > + !PostmasterIsAlive()) > That's pretty hack-ish, still efficient. Could we consider a > different approach like something relying on > PostmasterIsAliveInternal() with repetitive call handling? This may > not be the only place where we care about that, particularly for > non-core code.
As far as I know there aren't any other places that do polling of PostmasterIsAlive() in a loop like this. All others have been removed from core code: either they already had a WaitLatch() or similar so it we just had to add WL_EXIT_ON_PM_DEATH, or they do pure CPU-bound and don't actively try to detect postmaster death. That's why it seems utterly insane that here we try to detect it X million times per second. > No objections with the two changes from pg_usleep() to WaitLatch() so > they could be applied separately first. I thought about committing that first part, and got as far as splitting the patch into two (see attached), but then I re-read Fujii-san's message about the speed of promotion and realised that we really should have something like a condition variable for walRcvState changes. I'll look into that.
From f90c4b58989168a1fe5a2dd48181444a18351bfb Mon Sep 17 00:00:00 2001 From: Thomas Munro <tmu...@postgresql.org> Date: Mon, 1 Mar 2021 23:08:55 +1300 Subject: [PATCH v5 1/2] Replace some sleep/poll loops with WaitLatch(). Although the replaced sleeps are fairly short, a proposed patch to reduce the frequency of reads of the postmaster pipe could cause the loops to take a very long time to react to postmaster exit. Instead, use the standard waiting infrastructure (with no latch), which can exit automatically based on readiness. Reviewed-by: Heikki Linnakangas <hlinn...@iki.fi> Reviewed-by: Fujii Masao <masao.fu...@oss.nttdata.com> Reviewed-by: Michael Paquier <mich...@paquier.xyz> Discussion: https://postgr.es/m/CA%2BhUKGK1607VmtrDUHQXrsooU%3Dap4g4R2yaoByWOOA3m8xevUQ%40mail.gmail.com --- doc/src/sgml/monitoring.sgml | 4 ++++ src/backend/access/transam/xlog.c | 7 +++---- src/backend/postmaster/pgstat.c | 3 +++ src/backend/replication/walreceiverfuncs.c | 6 ++++-- src/include/pgstat.h | 1 + 5 files changed, 15 insertions(+), 6 deletions(-) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 3513e127b7..cf00210cb3 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -1757,6 +1757,10 @@ postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34 0:00 postgres: ser <entry>Waiting for confirmation from a remote server during synchronous replication.</entry> </row> + <row> + <entry><literal>WalrcvExit</literal></entry> + <entry>Waiting for the walreceiver to exit.</entry> + </row> <row> <entry><literal>XactGroupUpdate</literal></entry> <entry>Waiting for the group leader to update transaction status at diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 377afb8732..e63e19eed3 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -6017,7 +6017,7 @@ recoveryStopsAfter(XLogReaderState *record) * the paused state starts at the end of recovery because of * recovery_target_action=pause, and false otherwise. * - * XXX Could also be done with shared latch, avoiding the pg_usleep loop. + * XXX Could also be done with shared latch, avoiding the WL_TIMEOUT loop. * Probably not worth the trouble though. This state shouldn't be one that * anyone cares about server power consumption in. */ @@ -6046,9 +6046,8 @@ recoveryPausesHere(bool endOfRecovery) HandleStartupProcInterrupts(); if (CheckForStandbyTrigger()) return; - pgstat_report_wait_start(WAIT_EVENT_RECOVERY_PAUSE); - pg_usleep(1000000L); /* 1000 ms */ - pgstat_report_wait_end(); + (void) WaitLatch(NULL, WL_TIMEOUT | WL_EXIT_ON_PM_DEATH, 1000, + WAIT_EVENT_RECOVERY_PAUSE); } } diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c index f75b52719d..2dbfb81e40 100644 --- a/src/backend/postmaster/pgstat.c +++ b/src/backend/postmaster/pgstat.c @@ -4122,6 +4122,9 @@ pgstat_get_wait_ipc(WaitEventIPC w) case WAIT_EVENT_SYNC_REP: event_name = "SyncRep"; break; + case WAIT_EVENT_WALRCV_EXIT: + event_name = "WalrcvExit"; + break; case WAIT_EVENT_XACT_GROUP_UPDATE: event_name = "XactGroupUpdate"; break; diff --git a/src/backend/replication/walreceiverfuncs.c b/src/backend/replication/walreceiverfuncs.c index 63e60478ea..f51dd2fa20 100644 --- a/src/backend/replication/walreceiverfuncs.c +++ b/src/backend/replication/walreceiverfuncs.c @@ -23,8 +23,10 @@ #include <signal.h> #include "access/xlog_internal.h" +#include "pgstat.h" #include "postmaster/startup.h" #include "replication/walreceiver.h" +#include "storage/latch.h" #include "storage/pmsignal.h" #include "storage/shmem.h" #include "utils/timestamp.h" @@ -208,8 +210,8 @@ ShutdownWalRcv(void) * process. */ HandleStartupProcInterrupts(); - - pg_usleep(100000); /* 100ms */ + (void) WaitLatch(NULL, WL_TIMEOUT | WL_EXIT_ON_PM_DEATH, 100, + WAIT_EVENT_WALRCV_EXIT); } } diff --git a/src/include/pgstat.h b/src/include/pgstat.h index 724068cf87..222e38037c 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -998,6 +998,7 @@ typedef enum WAIT_EVENT_REPLICATION_SLOT_DROP, WAIT_EVENT_SAFE_SNAPSHOT, WAIT_EVENT_SYNC_REP, + WAIT_EVENT_WALRCV_EXIT, WAIT_EVENT_XACT_GROUP_UPDATE } WaitEventIPC; -- 2.30.1
From 8988e877cbdc5a7f19f481352b72598aee7314c8 Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.mu...@gmail.com> Date: Thu, 24 Sep 2020 17:37:54 +1200 Subject: [PATCH v5 2/2] Poll postmaster less frequently in recovery. Since commits 9f095299 and f98b8476 we don't poll the postmaster pipe at all during crash recovery on Linux and FreeBSD, but on other operating systems we were still doing it for every WAL record. Do it less frequently on operating systems where system calls are required, at the cost of delaying exit a bit after postmaster death, to avoid expensive system calls reported to slow down CPU-bound recovery by as much as 10-30%. Reviewed-by: Heikki Linnakangas <hlinn...@iki.fi> Reviewed-by: Fujii Masao <masao.fu...@oss.nttdata.com> Reviewed-by: Michael Paquier <mich...@paquier.xyz> Discussion: https://postgr.es/m/CA%2BhUKGK1607VmtrDUHQXrsooU%3Dap4g4R2yaoByWOOA3m8xevUQ%40mail.gmail.com Discussion: https://postgr.es/m/7261eb39-0369-f2f4-1bb5-62f3b6083...@iki.fi --- src/backend/postmaster/startup.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c index f781fdc6fc..682fc675ab 100644 --- a/src/backend/postmaster/startup.c +++ b/src/backend/postmaster/startup.c @@ -134,6 +134,10 @@ StartupRereadConfig(void) void HandleStartupProcInterrupts(void) { +#ifndef USE_POSTMASTER_DEATH_SIGNAL + static int count = 0; +#endif + /* * Process any requests or signals received recently. */ @@ -151,9 +155,17 @@ HandleStartupProcInterrupts(void) /* * Emergency bailout if postmaster has died. This is to avoid the - * necessity for manual cleanup of all postmaster children. + * necessity for manual cleanup of all postmaster children. Do this less + * frequently on systems for which we don't have signals to make that + * cheap. Any loop that sleeps should be using WaitLatch or similar and + * handling postmaster death that way; the check here is intended only to + * deal with CPU-bound loops such as the main redo loop. */ - if (IsUnderPostmaster && !PostmasterIsAlive()) + if (IsUnderPostmaster && +#ifndef USE_POSTMASTER_DEATH_SIGNAL + count++ % 1024 == 0 && +#endif + !PostmasterIsAlive()) exit(1); /* Process barrier events */ -- 2.30.1
