Skip to content

Commit 4e95733

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 a5c77e6 commit 4e95733

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
@@ -632,14 +632,25 @@ PostmasterMain(int argc, char *argv[])
632632
/*
633633
* Set up signal handlers for the postmaster process.
634634
*
635-
* In the postmaster, we want to install non-ignored handlers *without*
636-
* SA_RESTART. This is because they'll be blocked at all times except
637-
* when ServerLoop is waiting for something to happen, and during that
638-
* window, we want signals to exit the select(2) wait so that ServerLoop
639-
* can respond if anything interesting happened. On some platforms,
640-
* signals marked SA_RESTART would not cause the select() wait to end.
641-
* Child processes will generally want SA_RESTART, but we expect them to
642-
* set up their own handlers before unblocking signals.
635+
* In the postmaster, we use pqsignal_pm() rather than pqsignal() (which
636+
* is used by all child processes and client processes). That has a
637+
* couple of special behaviors:
638+
*
639+
* 1. Except on Windows, we tell sigaction() to block all signals for the
640+
* duration of the signal handler. This is faster than our old approach
641+
* of blocking/unblocking explicitly in the signal handler, and it should
642+
* also prevent excessive stack consumption if signals arrive quickly.
643+
*
644+
* 2. We do not set the SA_RESTART flag. This is because signals will be
645+
* blocked at all times except when ServerLoop is waiting for something to
646+
* happen, and during that window, we want signals to exit the select(2)
647+
* wait so that ServerLoop can respond if anything interesting happened.
648+
* On some platforms, signals marked SA_RESTART would not cause the
649+
* select() wait to end.
650+
*
651+
* Child processes will generally want SA_RESTART, so pqsignal() sets that
652+
* flag. We expect children to set up their own handlers before
653+
* unblocking signals.
643654
*
644655
* CAUTION: when changing this list, check for side-effects on the signal
645656
* handling setup of child processes. See tcop/postgres.c,
@@ -651,23 +662,21 @@ PostmasterMain(int argc, char *argv[])
651662
pqinitmask();
652663
PG_SETMASK(&BlockSig);
653664

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

673682
/*
@@ -2594,7 +2603,13 @@ SIGHUP_handler(SIGNAL_ARGS)
25942603
{
25952604
int save_errno = errno;
25962605

2606+
/*
2607+
* We rely on the signal mechanism to have blocked all signals ... except
2608+
* on Windows, which lacks sigaction(), so we have to do it manually.
2609+
*/
2610+
#ifdef WIN32
25972611
PG_SETMASK(&BlockSig);
2612+
#endif
25982613

25992614
if (Shutdown <= SmartShutdown)
26002615
{
@@ -2653,7 +2668,9 @@ SIGHUP_handler(SIGNAL_ARGS)
26532668
#endif
26542669
}
26552670

2671+
#ifdef WIN32
26562672
PG_SETMASK(&UnBlockSig);
2673+
#endif
26572674

26582675
errno = save_errno;
26592676
}
@@ -2667,7 +2684,13 @@ pmdie(SIGNAL_ARGS)
26672684
{
26682685
int save_errno = errno;
26692686

2687+
/*
2688+
* We rely on the signal mechanism to have blocked all signals ... except
2689+
* on Windows, which lacks sigaction(), so we have to do it manually.
2690+
*/
2691+
#ifdef WIN32
26702692
PG_SETMASK(&BlockSig);
2693+
#endif
26712694

26722695
ereport(DEBUG2,
26732696
(errmsg_internal("postmaster received signal %d",
@@ -2796,7 +2819,9 @@ pmdie(SIGNAL_ARGS)
27962819
break;
27972820
}
27982821

2822+
#ifdef WIN32
27992823
PG_SETMASK(&UnBlockSig);
2824+
#endif
28002825

28012826
errno = save_errno;
28022827
}
@@ -2811,7 +2836,13 @@ reaper(SIGNAL_ARGS)
28112836
int pid; /* process id of dead child process */
28122837
int exitstatus; /* its exit status */
28132838

2839+
/*
2840+
* We rely on the signal mechanism to have blocked all signals ... except
2841+
* on Windows, which lacks sigaction(), so we have to do it manually.
2842+
*/
2843+
#ifdef WIN32
28142844
PG_SETMASK(&BlockSig);
2845+
#endif
28152846

28162847
ereport(DEBUG4,
28172848
(errmsg_internal("reaping dead processes")));
@@ -3114,7 +3145,9 @@ reaper(SIGNAL_ARGS)
31143145
PostmasterStateMachine();
31153146

31163147
/* Done with signal handler */
3148+
#ifdef WIN32
31173149
PG_SETMASK(&UnBlockSig);
3150+
#endif
31183151

31193152
errno = save_errno;
31203153
}
@@ -5048,7 +5081,13 @@ sigusr1_handler(SIGNAL_ARGS)
50485081
{
50495082
int save_errno = errno;
50505083

5084+
/*
5085+
* We rely on the signal mechanism to have blocked all signals ... except
5086+
* on Windows, which lacks sigaction(), so we have to do it manually.
5087+
*/
5088+
#ifdef WIN32
50515089
PG_SETMASK(&BlockSig);
5090+
#endif
50525091

50535092
/* Process background worker state change. */
50545093
if (CheckPostmasterSignal(PMSIGNAL_BACKGROUND_WORKER_CHANGE))
@@ -5201,7 +5240,9 @@ sigusr1_handler(SIGNAL_ARGS)
52015240
signal_child(StartupPID, SIGUSR2);
52025241
}
52035242

5243+
#ifdef WIN32
52045244
PG_SETMASK(&UnBlockSig);
5245+
#endif
52055246

52065247
errno = save_errno;
52075248
}

‎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
@@ -475,11 +475,6 @@ extern int pg_mkdir_p(char *path, int omode);
475475
/* port/pqsignal.c */
476476
typedef void (*pqsigfunc) (int signo);
477477
extern pqsigfunc pqsignal(int signo, pqsigfunc func);
478-
#ifndef WIN32
479-
extern pqsigfunc pqsignal_no_restart(int signo, pqsigfunc func);
480-
#else
481-
#define pqsignal_no_restart(signo, func) pqsignal(signo, func)
482-
#endif
483478

484479
/* port/quotes.c */
485480
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)