On Sat, Jul 16, 2022 at 5:18 PM Thomas Munro <thomas.mu...@gmail.com> wrote: > On Sat, Jul 16, 2022 at 1:28 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > > Thomas Munro <thomas.mu...@gmail.com> writes: > > > On Fri, Jul 15, 2022 at 9:34 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > > >> (Someday we oughta go ahead and make our Windows signal API look more > > >> like POSIX, as I suggested back in 2015. I'm still not taking > > >> point on that, though.) > > > > > For the sigprocmask() part, here's a patch that passes CI. Only the > > > SIG_SETMASK case is actually exercised by our current code, though. > > > > Passes an eyeball check, but I can't actually test it. > > Thanks. Pushed. > > I'm not brave enough to try to write a replacement sigaction() yet, > but it does appear that we could rip more ugliness and inconsistencies > that way, eg sa_mask.
Here's a draft patch that adds a minimal sigaction() implementation for Windows. It doesn't implement stuff we're not using, for sample sa_sigaction functions, but it has the sa_mask feature so we can harmonize the stuff that I believe you were talking about.
From f00f169e10b884dab337b73b31de89577fb662de Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.mu...@gmail.com> Date: Wed, 17 Aug 2022 07:36:53 +1200 Subject: [PATCH] Provide sigaction for Windows. Harmonize Unix and Windows code by modernizing our signal emulation to use sigaction() instead of signal(). Discussion: https://postgr.es/m/CA%2BhUKGK5ax1AoFMvXksLROrxwB1%3D-EsT%3D7%2B6oUtCn47GbC9zjA%40mail.gmail.com --- src/backend/libpq/pqsignal.c | 9 ----- src/backend/port/win32/signal.c | 41 +++++++++++++++------ src/backend/postmaster/postmaster.c | 57 ++--------------------------- src/include/libpq/pqsignal.h | 14 +++++++ src/port/pqsignal.c | 22 +++++------ 5 files changed, 56 insertions(+), 87 deletions(-) diff --git a/src/backend/libpq/pqsignal.c b/src/backend/libpq/pqsignal.c index 26ed671d1a..1ab34c5214 100644 --- a/src/backend/libpq/pqsignal.c +++ b/src/backend/libpq/pqsignal.c @@ -110,16 +110,10 @@ pqinitmask(void) * 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; @@ -142,7 +136,4 @@ pqsignal_pm(int signo, pqsigfunc func) if (sigaction(signo, &act, &oact) < 0) return SIG_ERR; return oact.sa_handler; -#else /* WIN32 */ - return pqsignal(signo, func); -#endif } diff --git a/src/backend/port/win32/signal.c b/src/backend/port/win32/signal.c index 53b93a50b2..d533de1bc6 100644 --- a/src/backend/port/win32/signal.c +++ b/src/backend/port/win32/signal.c @@ -34,7 +34,7 @@ HANDLE pgwin32_initial_signal_pipe = INVALID_HANDLE_VALUE; static CRITICAL_SECTION pg_signal_crit_sec; /* Note that array elements 0 are unused since they correspond to signal 0 */ -static pqsigfunc pg_signal_array[PG_SIGNAL_COUNT]; +static struct sigaction pg_signal_array[PG_SIGNAL_COUNT]; static pqsigfunc pg_signal_defaults[PG_SIGNAL_COUNT]; @@ -85,7 +85,9 @@ pgwin32_signal_initialize(void) for (i = 0; i < PG_SIGNAL_COUNT; i++) { - pg_signal_array[i] = SIG_DFL; + pg_signal_array[i].sa_handler = SIG_DFL; + pg_signal_array[i].sa_mask = 0; + pg_signal_array[i].sa_flags = 0; pg_signal_defaults[i] = SIG_IGN; } pg_signal_mask = 0; @@ -131,15 +133,27 @@ pgwin32_dispatch_queued_signals(void) if (exec_mask & sigmask(i)) { /* Execute this signal */ - pqsigfunc sig = pg_signal_array[i]; + struct sigaction *act = &pg_signal_array[i]; + pqsigfunc sig = act->sa_handler; if (sig == SIG_DFL) sig = pg_signal_defaults[i]; pg_signal_queue &= ~sigmask(i); if (sig != SIG_ERR && sig != SIG_IGN && sig != SIG_DFL) { + sigset_t block_mask; + sigset_t save_mask; + LeaveCriticalSection(&pg_signal_crit_sec); + + block_mask = act->sa_mask; + if ((act->sa_flags & SA_NODEFER) == 0) + block_mask |= sigmask(i); + + sigprocmask(SIG_BLOCK, &block_mask, &save_mask); sig(i); + sigprocmask(SIG_SETMASK, &save_mask, NULL); + EnterCriticalSection(&pg_signal_crit_sec); break; /* Restart outer loop, in case signal mask or * queue has been modified inside signal @@ -187,22 +201,25 @@ pqsigprocmask(int how, const sigset_t *set, sigset_t *oset) return 0; } - /* * Unix-like signal handler installation * * Only called on main thread, no sync required */ -pqsigfunc -pqsignal(int signum, pqsigfunc handler) +int +pqsigaction(int signum, const struct sigaction *act, + struct sigaction *oldact) { - pqsigfunc prevfunc; - if (signum >= PG_SIGNAL_COUNT || signum < 0) - return SIG_ERR; - prevfunc = pg_signal_array[signum]; - pg_signal_array[signum] = handler; - return prevfunc; + { + errno = EINVAL; + return -1; + } + if (oldact) + *oldact = pg_signal_array[signum]; + if (act) + pg_signal_array[signum] = *act; + return 0; } /* Create the signal listener pipe for specified PID */ diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 8a038d1b2a..9227061cad 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -620,10 +620,10 @@ PostmasterMain(int argc, char *argv[]) * 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. + * 1. 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 @@ -2730,14 +2730,6 @@ 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) { ereport(LOG, @@ -2794,10 +2786,6 @@ SIGHUP_handler(SIGNAL_ARGS) #endif } -#ifdef WIN32 - PG_SETMASK(&UnBlockSig); -#endif - errno = save_errno; } @@ -2810,14 +2798,6 @@ 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", postgres_signal_arg))); @@ -2942,10 +2922,6 @@ pmdie(SIGNAL_ARGS) break; } -#ifdef WIN32 - PG_SETMASK(&UnBlockSig); -#endif - errno = save_errno; } @@ -2959,14 +2935,6 @@ 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"))); @@ -3259,11 +3227,6 @@ reaper(SIGNAL_ARGS) */ PostmasterStateMachine(); - /* Done with signal handler */ -#ifdef WIN32 - PG_SETMASK(&UnBlockSig); -#endif - errno = save_errno; } @@ -5110,14 +5073,6 @@ 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 - /* * RECOVERY_STARTED and BEGIN_HOT_STANDBY signals are ignored in * unexpected states. If the startup process quickly starts up, completes @@ -5258,10 +5213,6 @@ sigusr1_handler(SIGNAL_ARGS) signal_child(StartupPID, SIGUSR2); } -#ifdef WIN32 - PG_SETMASK(&UnBlockSig); -#endif - errno = save_errno; } diff --git a/src/include/libpq/pqsignal.h b/src/include/libpq/pqsignal.h index d17ddb787e..7890b426a8 100644 --- a/src/include/libpq/pqsignal.h +++ b/src/include/libpq/pqsignal.h @@ -21,12 +21,26 @@ /* Emulate POSIX sigset_t APIs on Windows */ typedef int sigset_t; +#define SA_RESTART 1 +#define SA_NODEFER 2 + +struct sigaction +{ + void (*sa_handler) (int); + /* sa_sigaction not yet implemented */ + sigset_t sa_mask; + int sa_flags; +}; + extern int pqsigprocmask(int how, const sigset_t *set, sigset_t *oset); +extern int pqsigaction(int signum, const struct sigaction *act, + struct sigaction *oldact); #define SIG_BLOCK 1 #define SIG_UNBLOCK 2 #define SIG_SETMASK 3 #define sigprocmask(how, set, oset) pqsigprocmask((how), (set), (oset)) +#define sigaction(signum, act, oldact) pqsigaction((signum), (act), (oldact)) #define sigemptyset(set) (*(set) = 0) #define sigfillset(set) (*(set) = ~0) #define sigaddset(set, signum) (*(set) |= (sigmask(signum))) diff --git a/src/port/pqsignal.c b/src/port/pqsignal.c index 6cb0320edb..fbb35005f4 100644 --- a/src/port/pqsignal.c +++ b/src/port/pqsignal.c @@ -12,15 +12,10 @@ * src/port/pqsignal.c * * We now assume that all Unix-oid systems have POSIX sigaction(2) - * with support for restartable signals (SA_RESTART). We used to also - * support BSD-style signal(2), but there really shouldn't be anything - * out there anymore that doesn't have the POSIX API. + * with support for restartable signals (SA_RESTART). * - * Windows, of course, is resolutely in a class by itself. In the backend, - * we don't use this file at all; src/backend/port/win32/signal.c provides - * pqsignal() for the backend environment. Frontend programs can use - * this version of pqsignal() if they wish, but beware that this does - * not provide restartable signals on Windows. + * Frontend programs can use this version of pqsignal() if they wish, but + * beware that this does not provide restartable signals on Windows. * * ------------------------------------------------------------------------ */ @@ -29,7 +24,9 @@ #include <signal.h> -#if !defined(WIN32) || defined(FRONTEND) +#ifndef FRONTEND +#include "libpq/pqsignal.h" +#endif /* * Set up a signal handler, with SA_RESTART, for signal "signo" @@ -39,7 +36,7 @@ pqsigfunc pqsignal(int signo, pqsigfunc func) { -#ifndef WIN32 +#if !(defined(WIN32) && defined(FRONTEND)) struct sigaction act, oact; @@ -53,9 +50,8 @@ pqsignal(int signo, pqsigfunc func) if (sigaction(signo, &act, &oact) < 0) return SIG_ERR; return oact.sa_handler; -#else /* WIN32 */ +#else + /* Forward to Windows native signal system. */ return signal(signo, func); #endif } - -#endif /* !defined(WIN32) || defined(FRONTEND) */ -- 2.37.1
