public inbox for [email protected]
help / color / mirror / Atom feedFix signal handler in pgpool main
3+ messages / 2 participants
[nested] [flat]
* Fix signal handler in pgpool main
@ 2026-06-08 01:33 Tatsuo Ishii <[email protected]>
2026-06-23 12:54 ` Re: Fix signal handler in pgpool main Tatsuo Ishii <[email protected]>
0 siblings, 1 reply; 3+ messages in thread
From: Tatsuo Ishii @ 2026-06-08 01:33 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]
I got a bug report regarding pgpool main process from Emond Papegaaij:
its exit/reload signal handler uses non signal safe functions, and it
could lead to a crash of the process.
Attached is the patch from him generated by Claude Code, with slight
modifications by me.
Basically it changes the signal handler to async-signal-safe calls
only: capture the signal number into a new volatile sig_atomic_t
main_exit_request, write one byte to the existing self-pipe to wake
the main loop, restore errno, and return. The actual shutdown is
performed synchronously by a new do_shutdown() function called from
the pgpol main loop at the top of every iteration (via
check_requests()) and also right after the inner pool_pause() returns,
so a signal arriving during the 2-second select() sleep is acted on
without an extra tick of latency.
I think this patch is important and worth to apply to all supported
branches.
However, since it touches one of the most critical part of pgpool, I
would like to apply to master branch first and see how our build-farm
acts before back patching (it will take a few days).
Comments?
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Attachments:
[text/x-patch] v1-0001-Do-not-use-signal-unsafe-functions-in-pgpool-main.patch (8.1K, ../../[email protected]/2-v1-0001-Do-not-use-signal-unsafe-functions-in-pgpool-main.patch)
download | inline diff:
From 9f4022f16be5a482da95f608afe686803ebba0d4 Mon Sep 17 00:00:00 2001
From: Tatsuo Ishii <[email protected]>
Date: Mon, 8 Jun 2026 09:18:23 +0900
Subject: [PATCH v1] Do not use signal unsafe functions in pgpool main process
signal handler.
The pgpool main process SIGTERM/SIGINT/SIGQUIT handler did the full
shutdown work inline: ereport() (twice),
pool_semaphore_lock(MAIN_EXIT_HANDLER_SEM), terminate_all_childrens()
with a blocking waitpid(-1, ..., 0) and more ereport() calls, kill()
of the follow-child group, and finally exit(3) which runs atexit
handlers and stdio flush. None of these are async-signal-safe (POSIX
2024 section 2.4.3 [1]). In particular, SIGTERM arriving while pgpool
is mid-ereport() / mid-palloc() / mid-semop() could crash, hang, or
corrupt heap state. Because the pgpool main process is the parent, a
crash here takes down every session and the pgpool cluster.
This commit restricts the handler to async-signal-safe calls only:
capture the signal number into a new volatile sig_atomic_t
main_exit_request, write one byte to the existing self-pipe to wake
the main loop, restore errno, and return. The actual shutdown is
performed synchronously by a new do_shutdown() function called from
the pgpol main loop at the top of every iteration (via
check_requests()) and also right after the inner pool_pause() returns,
so a signal arriving during the 2-second select() sleep is acted on
without an extra tick of latency.
do_shutdown() carries the previous body verbatim - the
non-async-safe calls (ereport, pool_semaphore_lock,
terminate_all_childrens with its blocking waitpid, exit(3)) are now
invoked from normal context where they are safe.
[1] https://pubs.opengroup.org/onlinepubs/9799919799/functions/V2_chap02.html#tag_16_04_03
Reported-by: Emond Papegaaij <[email protected]>
Reported-by: Claude code
Author: Tatsuo Ishii <[email protected]>
Discussion:
Backpatch-through: v4.3
---
src/main/pgpool_main.c | 111 +++++++++++++++++++++++++++++++++++------
1 file changed, 95 insertions(+), 16 deletions(-)
diff --git a/src/main/pgpool_main.c b/src/main/pgpool_main.c
index dbf1bfd14..5bb33e8de 100644
--- a/src/main/pgpool_main.c
+++ b/src/main/pgpool_main.c
@@ -171,6 +171,7 @@ static void kill_all_children(int sig);
static pid_t fork_follow_child(int old_main_node, int new_primary, int old_primary);
static int read_status_file(bool discard_status);
static RETSIGTYPE exit_handler(int sig);
+static void do_shutdown(int sig);
static RETSIGTYPE reap_handler(int sig);
static RETSIGTYPE sigusr1_handler(int sig);
static void sigusr1_interrupt_processor(void);
@@ -253,6 +254,12 @@ volatile sig_atomic_t reload_config_request = 0;
static volatile sig_atomic_t sigusr1_request = 0;
static volatile sig_atomic_t sigchld_request = 0;
static volatile sig_atomic_t wakeup_request = 0;
+static volatile sig_atomic_t main_exit_request = 0; /* set by exit_handler;
+ * carries the captured signal
+ * number (SIGTERM/INT/QUIT)
+ * so the main loop can
+ * perform the (non-async-safe)
+ * shutdown work itself. */
static int pipe_fds[2]; /* for delivering signals */
@@ -708,6 +715,13 @@ PgpoolMain(bool discard_status, bool clear_memcache_oidmaps)
r = pool_pause(&t);
POOL_SETMASK(&BlockSig);
+ /*
+ * If a SIGTERM/SIGINT/SIGQUIT was queued by exit_handler,
+ * service it immediately rather than waiting another tick.
+ */
+ if (main_exit_request)
+ do_shutdown(main_exit_request);
+
if (pool_config->process_management == PM_DYNAMIC)
service_child_processes();
@@ -1290,48 +1304,104 @@ terminate_all_childrens(int sig)
/*
- * Pgpool main process exit handler
+ * Pgpool main process exit handler.
+ *
+ * This handler runs in async-signal context: it MUST only call
+ * async-signal-safe primitives (POSIX 2024 2.4.3). ereport(),
+ * pool_semaphore_lock(), MemoryContext operations, blocking waitpid()
+ * and exit(3) (atexit chain + stdio flush) are all unsafe here. In a
+ * previous version this body did the full shutdown inline, which could
+ * deadlock or corrupt heap state when SIGTERM/SIGINT/SIGQUIT arrived
+ * while PT_MAIN was inside ereport(), palloc(), or another semop.
+ *
+ * The handler now only records the requested signal number into a
+ * sig_atomic_t flag and writes one byte to the self-pipe so the main
+ * loop's select() returns. The actual shutdown is performed
+ * synchronously by do_shutdown() from the main loop, where calling
+ * non-async-safe code is permitted.
+ *
+ * If the same signal is delivered to a forked child that has not yet
+ * reset this handler, fall through to proc_exit() unchanged.
*/
static RETSIGTYPE exit_handler(int sig)
{
- int *walk;
int save_errno = errno;
- ereport(LOG,
- (errmsg("exit handler called (signal: %d)", sig)));
-
- POOL_SETMASK(&AuthBlockSig);
-
/*
* this could happen in a child process if a signal has been sent before
- * resetting signal handler
+ * resetting signal handler. proc_exit() ultimately reduces to _exit()
+ * for non-PT_MAIN processes; calling it from a handler in a child that
+ * hasn't yet rewired its own handlers is the historical behaviour.
*/
if (getpid() != mypid)
{
- POOL_SETMASK(&UnBlockSig);
proc_exit(0);
+ errno = save_errno;
+ return;
}
if (sig != SIGTERM && sig != SIGINT && sig != SIGQUIT)
{
- POOL_SETMASK(&UnBlockSig);
errno = save_errno;
return;
}
/*
- * Check if another exit handler instance is already running. It is
- * possible that exit_handler is interrupted in the middle by other
- * signal.
+ * Record the signal for the main loop. Repeated signals coalesce; the
+ * first one wins, which matches the previous "exiting" guard.
+ */
+ if (main_exit_request == 0)
+ main_exit_request = sig;
+
+ /* Wake up the main loop if the self-pipe is set up. */
+ if (pipe_fds[1])
+ {
+ /*
+ * write() is async-signal-safe. We deliberately ignore the
+ * return value: if the pipe is full the main loop will pick the
+ * flag up on its next select() wake-up anyway.
+ *
+ * We do not use "(void) write()" here because it warns "warning:
+ * ignoring return value of ‘write’ declared with attribute
+ * ‘warn_unused_result’ [-Wunused-result] on gcc -O2 -Wall and some
+ * platforms enabling _FORTIFY_SOURCE.
+ */
+ ssize_t w = write(pipe_fds[1], "\0", 1);
+
+ (void) w;
+ }
+
+ errno = save_errno;
+}
+
+/*
+ * Synchronous shutdown body, invoked from the PT_MAIN main loop when
+ * main_exit_request has been set by exit_handler(). All the work that used
+ * to live inside the signal handler (ereport, pool_semaphore_lock,
+ * waitpid, kill of child group, exit(3)) lives here, in normal context
+ * where it is safe.
+ */
+static void
+do_shutdown(int sig)
+{
+ int *walk;
+
+ ereport(LOG,
+ (errmsg("exit handler called (signal: %d)", sig)));
+
+ POOL_SETMASK(&AuthBlockSig);
+
+ /*
+ * Check if another exit handler instance is already running. Since the
+ * shutdown now runs serially from the main loop this is mostly a
+ * belt-and-braces guard, but cheap and harmless to keep.
*/
if (exiting)
{
POOL_SETMASK(&UnBlockSig);
- errno = save_errno;
return;
}
- /* Check to make sure that other exit handler is not running */
pool_semaphore_lock(MAIN_EXIT_HANDLER_SEM);
if (exiting == 0)
{
@@ -1344,7 +1414,6 @@ static RETSIGTYPE exit_handler(int sig)
ereport(LOG,
(errmsg("exit handler (signal: %d) called. but exit handler is already in progress", sig)));
POOL_SETMASK(&UnBlockSig);
- errno = save_errno;
return;
}
@@ -5031,6 +5100,16 @@ check_requests(void)
{
sigset_t sig;
+ /*
+ * Shutdown request? exit_handler() set main_exit_request to the captured
+ * signal number; perform the actual (non-async-safe) shutdown here, in
+ * normal context. do_shutdown() does not return.
+ */
+ if (main_exit_request)
+ {
+ do_shutdown(main_exit_request);
+ }
+
/*
* Waking child request?
*/
--
2.43.0
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Fix signal handler in pgpool main
2026-06-08 01:33 Fix signal handler in pgpool main Tatsuo Ishii <[email protected]>
@ 2026-06-23 12:54 ` Tatsuo Ishii <[email protected]>
2026-06-24 13:19 ` Re: Fix signal handler in pgpool main Emond Papegaaij <[email protected]>
0 siblings, 1 reply; 3+ messages in thread
From: Tatsuo Ishii @ 2026-06-23 12:54 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]
Hi Emond,
> I got a bug report regarding pgpool main process from Emond Papegaaij:
> its exit/reload signal handler uses non signal safe functions, and it
> could lead to a crash of the process.
>
> Attached is the patch from him generated by Claude Code, with slight
> modifications by me.
>
> Basically it changes the signal handler to async-signal-safe calls
> only: capture the signal number into a new volatile sig_atomic_t
> main_exit_request, write one byte to the existing self-pipe to wake
> the main loop, restore errno, and return. The actual shutdown is
> performed synchronously by a new do_shutdown() function called from
> the pgpol main loop at the top of every iteration (via
> check_requests()) and also right after the inner pool_pause() returns,
> so a signal arriving during the 2-second select() sleep is acted on
> without an extra tick of latency.
>
> I think this patch is important and worth to apply to all supported
> branches.
>
> However, since it touches one of the most critical part of pgpool, I
> would like to apply to master branch first and see how our build-farm
> acts before back patching (it will take a few days).
>
> Comments?
Patch pushed to v4.4.
Peng and Koshino reviewed the patch off-list. No change was necessary.
I just removed some non-ASCII characters in comments (probably I
mistakenly brought them in by copy-and-paste). I tried to back-patch
to v4.3 but it seems it needs invasive changes and I gave up it. So I
decided to back patch through v4.4.
Thank you!
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Fix signal handler in pgpool main
2026-06-08 01:33 Fix signal handler in pgpool main Tatsuo Ishii <[email protected]>
2026-06-23 12:54 ` Re: Fix signal handler in pgpool main Tatsuo Ishii <[email protected]>
@ 2026-06-24 13:19 ` Emond Papegaaij <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Emond Papegaaij @ 2026-06-24 13:19 UTC (permalink / raw)
To: Tatsuo Ishii <[email protected]>; +Cc: [email protected]
Thank you for taking care of this! I agree with your decision to not
patch 4.3 given that the code doesn't match. You have to weigh the
benefits against the risks in such cases.
Best regards,
Emond
Op di 23 jun 2026 om 14:54 schreef Tatsuo Ishii <[email protected]>:
>
> Hi Emond,
>
> > I got a bug report regarding pgpool main process from Emond Papegaaij:
> > its exit/reload signal handler uses non signal safe functions, and it
> > could lead to a crash of the process.
> >
> > Attached is the patch from him generated by Claude Code, with slight
> > modifications by me.
> >
> > Basically it changes the signal handler to async-signal-safe calls
> > only: capture the signal number into a new volatile sig_atomic_t
> > main_exit_request, write one byte to the existing self-pipe to wake
> > the main loop, restore errno, and return. The actual shutdown is
> > performed synchronously by a new do_shutdown() function called from
> > the pgpol main loop at the top of every iteration (via
> > check_requests()) and also right after the inner pool_pause() returns,
> > so a signal arriving during the 2-second select() sleep is acted on
> > without an extra tick of latency.
> >
> > I think this patch is important and worth to apply to all supported
> > branches.
> >
> > However, since it touches one of the most critical part of pgpool, I
> > would like to apply to master branch first and see how our build-farm
> > acts before back patching (it will take a few days).
> >
> > Comments?
>
> Patch pushed to v4.4.
>
> Peng and Koshino reviewed the patch off-list. No change was necessary.
> I just removed some non-ASCII characters in comments (probably I
> mistakenly brought them in by copy-and-paste). I tried to back-patch
> to v4.3 but it seems it needs invasive changes and I gave up it. So I
> decided to back patch through v4.4.
>
> Thank you!
> --
> Tatsuo Ishii
> SRA OSS K.K.
> English: http://www.sraoss.co.jp/index_en/
> Japanese:http://www.sraoss.co.jp
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2026-06-24 13:19 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-06-08 01:33 Fix signal handler in pgpool main Tatsuo Ishii <[email protected]>
2026-06-23 12:54 ` Tatsuo Ishii <[email protected]>
2026-06-24 13:19 ` Emond Papegaaij <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox