Skip to content

Commit 8583402

Browse files
tglsfdctglsfdc
committed
In the postmaster, rely on the signal infrastructure to block signals.
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 9abb2bf. 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
1 parent cc1fc7b commit 8583402

File tree

5 files changed

+116
-57
lines changed

5 files changed

+116
-57
lines changed

‎src/backend/libpq/pqsignal.c

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,3 +95,52 @@ pqinitmask(void)
9595
sigdelset(&StartupBlockSig, SIGALRM);
9696
#endif
9797
}
98+
99+
/*
100+
* Set up a postmaster signal handler for signal "signo"
101+
*
102+
* Returns the previous handler.
103+
*
104+
* This is used only in the postmaster, which has its own odd approach to
105+
* signal handling. For signals with handlers, we block all signals for the
106+
* duration of signal handler execution. We also do not set the SA_RESTART
107+
* flag; this should be safe given the tiny range of code in which the
108+
* postmaster ever unblocks signals.
109+
*
110+
* pqinitmask() must have been invoked previously.
111+
*
112+
* On Windows, this function is just an alias for pqsignal()
113+
* (and note that it's calling the code in src/backend/port/win32/signal.c,
114+
* not src/port/pqsignal.c). On that platform, the postmaster's signal
115+
* handlers still have to block signals for themselves.
116+
*/
117+
pqsigfunc
118+
pqsignal_pm(int signo, pqsigfunc func)
119+
{
120+
#ifndef WIN32
121+
struct sigaction act,
122+
oact;
123+
124+
act.sa_handler = func;
125+
if (func == SIG_IGN || func == SIG_DFL)
126+
{
127+
/* in these cases, act the same as pqsignal() */
128+
sigemptyset(&act.sa_mask);
129+
act.sa_flags = SA_RESTART;
130+
}
131+
else
132+
{
133+
act.sa_mask = BlockSig;
134+
act.sa_flags = 0;
135+
}
136+
#ifdef SA_NOCLDSTOP
137+
if (signo == SIGCHLD)
138+
act.sa_flags |= SA_NOCLDSTOP;
139+
#endif
140+
if (sigaction(signo, &act, &oact) < 0)
141+
return SIG_ERR;
142+
return oact.sa_handler;
143+
#else /* WIN32 */
144+
return pqsignal(signo, func);
145+
#endif
146+
}

‎src/backend/postmaster/postmaster.c

Lines changed: 64 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -637,14 +637,25 @@ PostmasterMain(int argc, char *argv[])
637637
/*
638638
* Set up signal handlers for the postmaster process.
639639
*
640-
* In the postmaster, we want to install non-ignored handlers *without*
641-
* SA_RESTART. This is because they'll be blocked at all times except
642-
* when ServerLoop is waiting for something to happen, and during that
643-
* window, we want signals to exit the select(2) wait so that ServerLoop
644-
* can respond if anything interesting happened. On some platforms,
645-
* signals marked SA_RESTART would not cause the select() wait to end.
646-
* Child processes will generally want SA_RESTART, but we expect them to
647-
* set up their own handlers before unblocking signals.
640+
* In the postmaster, we use pqsignal_pm() rather than pqsignal() (which
641+
* is used by all child processes and client processes). That has a
642+
* couple of special behaviors:
643+
*
644+
* 1. Except on Windows, we tell sigaction() to block all signals for the
645+
* duration of the signal handler. This is faster than our old approach
646+
* of blocking/unblocking explicitly in the signal handler, and it should
647+
* also prevent excessive stack consumption if signals arrive quickly.
648+
*
649+
* 2. We do not set the SA_RESTART flag. This is because signals will be
650+
* blocked at all times except when ServerLoop is waiting for something to
651+
* happen, and during that window, we want signals to exit the select(2)
652+
* wait so that ServerLoop can respond if anything interesting happened.
653+
* On some platforms, signals marked SA_RESTART would not cause the
654+
* select() wait to end.
655+
*
656+
* Child processes will generally want SA_RESTART, so pqsignal() sets that
657+
* flag. We expect children to set up their own handlers before
658+
* unblocking signals.
648659
*
649660
* CAUTION: when changing this list, check for side-effects on the signal
650661
* handling setup of child processes. See tcop/postgres.c,
@@ -656,23 +667,21 @@ PostmasterMain(int argc, char *argv[])
656667
pqinitmask();
657668
PG_SETMASK(&BlockSig);
658669

659-
pqsignal_no_restart(SIGHUP, SIGHUP_handler); /* reread config file and
660-
* have children do same */
661-
pqsignal_no_restart(SIGINT, pmdie); /* send SIGTERM and shut down */
662-
pqsignal_no_restart(SIGQUIT, pmdie); /* send SIGQUIT and die */
663-
pqsignal_no_restart(SIGTERM, pmdie); /* wait for children and shut down */
664-
pqsignal(SIGALRM, SIG_IGN); /* ignored */
665-
pqsignal(SIGPIPE, SIG_IGN); /* ignored */
666-
pqsignal_no_restart(SIGUSR1, sigusr1_handler); /* message from child
667-
* process */
668-
pqsignal_no_restart(SIGUSR2, dummy_handler); /* unused, reserve for
669-
* children */
670-
pqsignal_no_restart(SIGCHLD, reaper); /* handle child termination */
671-
pqsignal(SIGTTIN, SIG_IGN); /* ignored */
672-
pqsignal(SIGTTOU, SIG_IGN); /* ignored */
670+
pqsignal_pm(SIGHUP, SIGHUP_handler); /* reread config file and have
671+
* children do same */
672+
pqsignal_pm(SIGINT, pmdie); /* send SIGTERM and shut down */
673+
pqsignal_pm(SIGQUIT, pmdie); /* send SIGQUIT and die */
674+
pqsignal_pm(SIGTERM, pmdie); /* wait for children and shut down */
675+
pqsignal_pm(SIGALRM, SIG_IGN); /* ignored */
676+
pqsignal_pm(SIGPIPE, SIG_IGN); /* ignored */
677+
pqsignal_pm(SIGUSR1, sigusr1_handler); /* message from child process */
678+
pqsignal_pm(SIGUSR2, dummy_handler); /* unused, reserve for children */
679+
pqsignal_pm(SIGCHLD, reaper); /* handle child termination */
680+
pqsignal_pm(SIGTTIN, SIG_IGN); /* ignored */
681+
pqsignal_pm(SIGTTOU, SIG_IGN); /* ignored */
673682
/* ignore SIGXFSZ, so that ulimit violations work like disk full */
674683
#ifdef SIGXFSZ
675-
pqsignal(SIGXFSZ, SIG_IGN); /* ignored */
684+
pqsignal_pm(SIGXFSZ, SIG_IGN); /* ignored */
676685
#endif
677686

678687
/*
@@ -2545,7 +2554,13 @@ SIGHUP_handler(SIGNAL_ARGS)
25452554
{
25462555
int save_errno = errno;
25472556

2557+
/*
2558+
* We rely on the signal mechanism to have blocked all signals ... except
2559+
* on Windows, which lacks sigaction(), so we have to do it manually.
2560+
*/
2561+
#ifdef WIN32
25482562
PG_SETMASK(&BlockSig);
2563+
#endif
25492564

25502565
if (Shutdown <= SmartShutdown)
25512566
{
@@ -2604,7 +2619,9 @@ SIGHUP_handler(SIGNAL_ARGS)
26042619
#endif
26052620
}
26062621

2622+
#ifdef WIN32
26072623
PG_SETMASK(&UnBlockSig);
2624+
#endif
26082625

26092626
errno = save_errno;
26102627
}
@@ -2618,7 +2635,13 @@ pmdie(SIGNAL_ARGS)
26182635
{
26192636
int save_errno = errno;
26202637

2638+
/*
2639+
* We rely on the signal mechanism to have blocked all signals ... except
2640+
* on Windows, which lacks sigaction(), so we have to do it manually.
2641+
*/
2642+
#ifdef WIN32
26212643
PG_SETMASK(&BlockSig);
2644+
#endif
26222645

26232646
ereport(DEBUG2,
26242647
(errmsg_internal("postmaster received signal %d",
@@ -2747,7 +2770,9 @@ pmdie(SIGNAL_ARGS)
27472770
break;
27482771
}
27492772

2773+
#ifdef WIN32
27502774
PG_SETMASK(&UnBlockSig);
2775+
#endif
27512776

27522777
errno = save_errno;
27532778
}
@@ -2762,7 +2787,13 @@ reaper(SIGNAL_ARGS)
27622787
int pid; /* process id of dead child process */
27632788
int exitstatus; /* its exit status */
27642789

2790+
/*
2791+
* We rely on the signal mechanism to have blocked all signals ... except
2792+
* on Windows, which lacks sigaction(), so we have to do it manually.
2793+
*/
2794+
#ifdef WIN32
27652795
PG_SETMASK(&BlockSig);
2796+
#endif
27662797

27672798
ereport(DEBUG4,
27682799
(errmsg_internal("reaping dead processes")));
@@ -3065,7 +3096,9 @@ reaper(SIGNAL_ARGS)
30653096
PostmasterStateMachine();
30663097

30673098
/* Done with signal handler */
3099+
#ifdef WIN32
30683100
PG_SETMASK(&UnBlockSig);
3101+
#endif
30693102

30703103
errno = save_errno;
30713104
}
@@ -5018,7 +5051,13 @@ sigusr1_handler(SIGNAL_ARGS)
50185051
{
50195052
int save_errno = errno;
50205053

5054+
/*
5055+
* We rely on the signal mechanism to have blocked all signals ... except
5056+
* on Windows, which lacks sigaction(), so we have to do it manually.
5057+
*/
5058+
#ifdef WIN32
50215059
PG_SETMASK(&BlockSig);
5060+
#endif
50225061

50235062
/* Process background worker state change. */
50245063
if (CheckPostmasterSignal(PMSIGNAL_BACKGROUND_WORKER_CHANGE))
@@ -5171,7 +5210,9 @@ sigusr1_handler(SIGNAL_ARGS)
51715210
signal_child(StartupPID, SIGUSR2);
51725211
}
51735212

5213+
#ifdef WIN32
51745214
PG_SETMASK(&UnBlockSig);
5215+
#endif
51755216

51765217
errno = save_errno;
51775218
}

‎src/include/libpq/pqsignal.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,4 +36,7 @@ extern sigset_t UnBlockSig,
3636

3737
extern void pqinitmask(void);
3838

39+
/* pqsigfunc is declared in src/include/port.h */
40+
extern pqsigfunc pqsignal_pm(int signo, pqsigfunc func);
41+
3942
#endif /* PQSIGNAL_H */

‎src/include/port.h

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -456,11 +456,6 @@ extern int pg_mkdir_p(char *path, int omode);
456456
/* port/pqsignal.c */
457457
typedef void (*pqsigfunc) (int signo);
458458
extern pqsigfunc pqsignal(int signo, pqsigfunc func);
459-
#ifndef WIN32
460-
extern pqsigfunc pqsignal_no_restart(int signo, pqsigfunc func);
461-
#else
462-
#define pqsignal_no_restart(signo, func) pqsignal(signo, func)
463-
#endif
464459

465460
/* port/quotes.c */
466461
extern char *escape_single_quotes_ascii(const char *src);

‎src/port/pqsignal.c

Lines changed: 0 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -58,33 +58,4 @@ pqsignal(int signo, pqsigfunc func)
5858
#endif
5959
}
6060

61-
/*
62-
* Set up a signal handler, without SA_RESTART, for signal "signo"
63-
*
64-
* Returns the previous handler.
65-
*
66-
* On Windows, this would be identical to pqsignal(), so don't bother.
67-
*/
68-
#ifndef WIN32
69-
70-
pqsigfunc
71-
pqsignal_no_restart(int signo, pqsigfunc func)
72-
{
73-
struct sigaction act,
74-
oact;
75-
76-
act.sa_handler = func;
77-
sigemptyset(&act.sa_mask);
78-
act.sa_flags = 0;
79-
#ifdef SA_NOCLDSTOP
80-
if (signo == SIGCHLD)
81-
act.sa_flags |= SA_NOCLDSTOP;
82-
#endif
83-
if (sigaction(signo, &act, &oact) < 0)
84-
return SIG_ERR;
85-
return oact.sa_handler;
86-
}
87-
88-
#endif /* !WIN32 */
89-
9061
#endif /* !defined(WIN32) || defined(FRONTEND) */

0 commit comments

Comments
 (0)