In the postmaster, rely on the signal infrastructure to block signals.
authorTom Lane
Thu, 15 Oct 2020 16:50:57 +0000 (12:50 -0400)
committerTom Lane
Thu, 15 Oct 2020 16:50:57 +0000 (12:50 -0400)
POSIX sigaction(2) can be told to block a set of signals while a
signal handler executes.  Make use of that instead of manually
blocking and unblocking signals in the postmaster's signal handlers.
This should save a few cycles, but more importantly it prevents
recursive invocation of signal handlers when many signals arrive in
close succession.  (Assuming that the platform's signal infrastructure
is designed to avoid consuming stack space in that case, but this is
demonstrably true at least on Linux.)  The existing code has been seen
to recurse to the point of stack overflow, either in the postmaster
or in a forked-off child.

Back-patch of commit 9abb2bfc0.  At the time, we'd only seen excess
postmaster stack consumption in the buildfarm; but we now have a
user report of it, and that commit has aged enough to have a fair
amount of confidence that it doesn't break anything.

This still doesn't change anything about the way that it works on
Windows.  Perhaps someone else would like to fix that?

Per bug #16673 from David Geier.  Back-patch to 9.6.  Although
the problem exists in principle before that, we've only seen it
actually materialize in connection with heavy use of parallel
workers, so it doesn't seem necessary to do anything in 9.5;
and the relevant code is different there, too.

Discussion: https://postgr.es/m/16673-d278c604f8e34ec0@postgresql.org
Discussion: https://postgr.es/m/14878.1570820201@sss.pgh.pa.us

src/backend/libpq/pqsignal.c
src/backend/postmaster/postmaster.c
src/include/libpq/pqsignal.h
src/include/port.h
src/port/pqsignal.c

index 476e883a687520cde47bb8dd43a085f091778b4d..63bee7dc4c007aff8cfe1072ef1ee44fab650309 100644 (file)
@@ -95,3 +95,52 @@ pqinitmask(void)
    sigdelset(&StartupBlockSig, SIGALRM);
 #endif
 }
+
+/*
+ * Set up a postmaster signal handler for signal "signo"
+ *
+ * Returns the previous handler.
+ *
+ * This is used only in the postmaster, which has its own odd approach to
+ * signal handling.  For signals with handlers, we block all signals for the
+ * duration of signal handler execution.  We also do not set the SA_RESTART
+ * flag; this should be safe given the tiny range of code in which the
+ * postmaster ever unblocks signals.
+ *
+ * pqinitmask() must have been invoked previously.
+ *
+ * On Windows, this function is just an alias for pqsignal()
+ * (and note that it's calling the code in src/backend/port/win32/signal.c,
+ * not src/port/pqsignal.c).  On that platform, the postmaster's signal
+ * handlers still have to block signals for themselves.
+ */
+pqsigfunc
+pqsignal_pm(int signo, pqsigfunc func)
+{
+#ifndef WIN32
+   struct sigaction act,
+               oact;
+
+   act.sa_handler = func;
+   if (func == SIG_IGN || func == SIG_DFL)
+   {
+       /* in these cases, act the same as pqsignal() */
+       sigemptyset(&act.sa_mask);
+       act.sa_flags = SA_RESTART;
+   }
+   else
+   {
+       act.sa_mask = BlockSig;
+       act.sa_flags = 0;
+   }
+#ifdef SA_NOCLDSTOP
+   if (signo == SIGCHLD)
+       act.sa_flags |= SA_NOCLDSTOP;
+#endif
+   if (sigaction(signo, &act, &oact) < 0)
+       return SIG_ERR;
+   return oact.sa_handler;
+#else                          /* WIN32 */
+   return pqsignal(signo, func);
+#endif
+}
index 509b1e72fb2cdd6dd0dabc868269db9fb2424fec..047e43b9ccd949ceb6467ab09c7dc48c49610e3c 100644 (file)
@@ -632,14 +632,25 @@ PostmasterMain(int argc, char *argv[])
    /*
     * Set up signal handlers for the postmaster process.
     *
-    * In the postmaster, we want to install non-ignored handlers *without*
-    * SA_RESTART.  This is because they'll be blocked at all times except
-    * when ServerLoop is waiting for something to happen, and during that
-    * window, we want signals to exit the select(2) wait so that ServerLoop
-    * can respond if anything interesting happened.  On some platforms,
-    * signals marked SA_RESTART would not cause the select() wait to end.
-    * Child processes will generally want SA_RESTART, but we expect them to
-    * set up their own handlers before unblocking signals.
+    * In the postmaster, we use pqsignal_pm() rather than pqsignal() (which
+    * is used by all child processes and client processes).  That has a
+    * couple of special behaviors:
+    *
+    * 1. Except on Windows, we tell sigaction() to block all signals for the
+    * duration of the signal handler.  This is faster than our old approach
+    * of blocking/unblocking explicitly in the signal handler, and it should
+    * also prevent excessive stack consumption if signals arrive quickly.
+    *
+    * 2. We do not set the SA_RESTART flag.  This is because signals will be
+    * blocked at all times except when ServerLoop is waiting for something to
+    * happen, and during that window, we want signals to exit the select(2)
+    * wait so that ServerLoop can respond if anything interesting happened.
+    * On some platforms, signals marked SA_RESTART would not cause the
+    * select() wait to end.
+    *
+    * Child processes will generally want SA_RESTART, so pqsignal() sets that
+    * flag.  We expect children to set up their own handlers before
+    * unblocking signals.
     *
     * CAUTION: when changing this list, check for side-effects on the signal
     * handling setup of child processes.  See tcop/postgres.c,
@@ -651,23 +662,21 @@ PostmasterMain(int argc, char *argv[])
    pqinitmask();
    PG_SETMASK(&BlockSig);
 
-   pqsignal_no_restart(SIGHUP, SIGHUP_handler);    /* reread config file and
-                                                    * have children do same */
-   pqsignal_no_restart(SIGINT, pmdie); /* send SIGTERM and shut down */
-   pqsignal_no_restart(SIGQUIT, pmdie);    /* send SIGQUIT and die */
-   pqsignal_no_restart(SIGTERM, pmdie);    /* wait for children and shut down */
-   pqsignal(SIGALRM, SIG_IGN); /* ignored */
-   pqsignal(SIGPIPE, SIG_IGN); /* ignored */
-   pqsignal_no_restart(SIGUSR1, sigusr1_handler);  /* message from child
-                                                    * process */
-   pqsignal_no_restart(SIGUSR2, dummy_handler);    /* unused, reserve for
-                                                    * children */
-   pqsignal_no_restart(SIGCHLD, reaper);   /* handle child termination */
-   pqsignal(SIGTTIN, SIG_IGN); /* ignored */
-   pqsignal(SIGTTOU, SIG_IGN); /* ignored */
+   pqsignal_pm(SIGHUP, SIGHUP_handler);    /* reread config file and have
+                                            * children do same */
+   pqsignal_pm(SIGINT, pmdie); /* send SIGTERM and shut down */
+   pqsignal_pm(SIGQUIT, pmdie);    /* send SIGQUIT and die */
+   pqsignal_pm(SIGTERM, pmdie);    /* wait for children and shut down */
+   pqsignal_pm(SIGALRM, SIG_IGN);  /* ignored */
+   pqsignal_pm(SIGPIPE, SIG_IGN);  /* ignored */
+   pqsignal_pm(SIGUSR1, sigusr1_handler);  /* message from child process */
+   pqsignal_pm(SIGUSR2, dummy_handler);    /* unused, reserve for children */
+   pqsignal_pm(SIGCHLD, reaper);   /* handle child termination */
+   pqsignal_pm(SIGTTIN, SIG_IGN);  /* ignored */
+   pqsignal_pm(SIGTTOU, SIG_IGN);  /* ignored */
    /* ignore SIGXFSZ, so that ulimit violations work like disk full */
 #ifdef SIGXFSZ
-   pqsignal(SIGXFSZ, SIG_IGN); /* ignored */
+   pqsignal_pm(SIGXFSZ, SIG_IGN);  /* ignored */
 #endif
 
    /*
@@ -2594,7 +2603,13 @@ SIGHUP_handler(SIGNAL_ARGS)
 {
    int         save_errno = errno;
 
+   /*
+    * We rely on the signal mechanism to have blocked all signals ... except
+    * on Windows, which lacks sigaction(), so we have to do it manually.
+    */
+#ifdef WIN32
    PG_SETMASK(&BlockSig);
+#endif
 
    if (Shutdown <= SmartShutdown)
    {
@@ -2653,7 +2668,9 @@ SIGHUP_handler(SIGNAL_ARGS)
 #endif
    }
 
+#ifdef WIN32
    PG_SETMASK(&UnBlockSig);
+#endif
 
    errno = save_errno;
 }
@@ -2667,7 +2684,13 @@ pmdie(SIGNAL_ARGS)
 {
    int         save_errno = errno;
 
+   /*
+    * We rely on the signal mechanism to have blocked all signals ... except
+    * on Windows, which lacks sigaction(), so we have to do it manually.
+    */
+#ifdef WIN32
    PG_SETMASK(&BlockSig);
+#endif
 
    ereport(DEBUG2,
            (errmsg_internal("postmaster received signal %d",
@@ -2796,7 +2819,9 @@ pmdie(SIGNAL_ARGS)
            break;
    }
 
+#ifdef WIN32
    PG_SETMASK(&UnBlockSig);
+#endif
 
    errno = save_errno;
 }
@@ -2811,7 +2836,13 @@ reaper(SIGNAL_ARGS)
    int         pid;            /* process id of dead child process */
    int         exitstatus;     /* its exit status */
 
+   /*
+    * We rely on the signal mechanism to have blocked all signals ... except
+    * on Windows, which lacks sigaction(), so we have to do it manually.
+    */
+#ifdef WIN32
    PG_SETMASK(&BlockSig);
+#endif
 
    ereport(DEBUG4,
            (errmsg_internal("reaping dead processes")));
@@ -3114,7 +3145,9 @@ reaper(SIGNAL_ARGS)
    PostmasterStateMachine();
 
    /* Done with signal handler */
+#ifdef WIN32
    PG_SETMASK(&UnBlockSig);
+#endif
 
    errno = save_errno;
 }
@@ -5048,7 +5081,13 @@ sigusr1_handler(SIGNAL_ARGS)
 {
    int         save_errno = errno;
 
+   /*
+    * We rely on the signal mechanism to have blocked all signals ... except
+    * on Windows, which lacks sigaction(), so we have to do it manually.
+    */
+#ifdef WIN32
    PG_SETMASK(&BlockSig);
+#endif
 
    /* Process background worker state change. */
    if (CheckPostmasterSignal(PMSIGNAL_BACKGROUND_WORKER_CHANGE))
@@ -5201,7 +5240,9 @@ sigusr1_handler(SIGNAL_ARGS)
        signal_child(StartupPID, SIGUSR2);
    }
 
+#ifdef WIN32
    PG_SETMASK(&UnBlockSig);
+#endif
 
    errno = save_errno;
 }
index af4e61ba4d9330a3e9203808d84bb0f319e25c33..83cc559d2ec23ce360dd345a93edb3aec5f4730f 100644 (file)
@@ -36,4 +36,7 @@ extern sigset_t UnBlockSig,
 
 extern void pqinitmask(void);
 
+/* pqsigfunc is declared in src/include/port.h */
+extern pqsigfunc pqsignal_pm(int signo, pqsigfunc func);
+
 #endif                         /* PQSIGNAL_H */
index f2b966fdca57b90ed18aac9722184b8bde1541e1..52deede1939c4b566031333aff6e1b7b6c67c6a3 100644 (file)
@@ -475,11 +475,6 @@ extern int pg_mkdir_p(char *path, int omode);
 /* port/pqsignal.c */
 typedef void (*pqsigfunc) (int signo);
 extern pqsigfunc pqsignal(int signo, pqsigfunc func);
-#ifndef WIN32
-extern pqsigfunc pqsignal_no_restart(int signo, pqsigfunc func);
-#else
-#define pqsignal_no_restart(signo, func) pqsignal(signo, func)
-#endif
 
 /* port/quotes.c */
 extern char *escape_single_quotes_ascii(const char *src);
index f176387ca201f580f7dd1f4ae74118ade9d28a06..5e0e6028ccf525e0cd078934c4591acc5389bf85 100644 (file)
@@ -58,33 +58,4 @@ pqsignal(int signo, pqsigfunc func)
 #endif
 }
 
-/*
- * Set up a signal handler, without SA_RESTART, for signal "signo"
- *
- * Returns the previous handler.
- *
- * On Windows, this would be identical to pqsignal(), so don't bother.
- */
-#ifndef WIN32
-
-pqsigfunc
-pqsignal_no_restart(int signo, pqsigfunc func)
-{
-   struct sigaction act,
-               oact;
-
-   act.sa_handler = func;
-   sigemptyset(&act.sa_mask);
-   act.sa_flags = 0;
-#ifdef SA_NOCLDSTOP
-   if (signo == SIGCHLD)
-       act.sa_flags |= SA_NOCLDSTOP;
-#endif
-   if (sigaction(signo, &act, &oact) < 0)
-       return SIG_ERR;
-   return oact.sa_handler;
-}
-
-#endif                         /* !WIN32 */
-
 #endif                         /* !defined(WIN32) || defined(FRONTEND) */