Here's a draft patch that I think would be reasonable to back-patch.
(Before v13, we'd need a bespoke SIGQUIT handler to substitute for
SignalHandlerForCrashExit, but that's easy enough.)
Aside from comment updates, this
* uses SignalHandlerForCrashExit for SIGQUIT
* renames startup_die per your request
* moves BackendInitialize's interrupt-re-disabling code up a bit to
reduce the scope where these interrupts are active. This doesn't
make things a whole lot safer, but it can't hurt.
I'll take a closer look at the idea of using _exit(1) tomorrow,
but I'd be pretty hesitant to back-patch that.
regards, tom lane
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 42223c0f61..e65086f7b4 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -112,6 +112,7 @@
#include "postmaster/autovacuum.h"
#include "postmaster/bgworker_internals.h"
#include "postmaster/fork_process.h"
+#include "postmaster/interrupt.h"
#include "postmaster/pgarch.h"
#include "postmaster/postmaster.h"
#include "postmaster/syslogger.h"
@@ -405,7 +406,7 @@ static void SIGHUP_handler(SIGNAL_ARGS);
static void pmdie(SIGNAL_ARGS);
static void reaper(SIGNAL_ARGS);
static void sigusr1_handler(SIGNAL_ARGS);
-static void startup_die(SIGNAL_ARGS);
+static void startup_packet_die(SIGNAL_ARGS);
static void dummy_handler(SIGNAL_ARGS);
static void StartupPacketTimeoutHandler(void);
static void CleanupBackend(int pid, int exitstatus);
@@ -4340,22 +4341,30 @@ BackendInitialize(Port *port)
whereToSendOutput = DestRemote; /* now safe to ereport to client */
/*
- * We arrange for a simple exit(1) if we receive SIGTERM or SIGQUIT or
- * timeout while trying to collect the startup packet. Otherwise the
- * postmaster cannot shutdown the database FAST or IMMED cleanly if a
- * buggy client fails to send the packet promptly. XXX it follows that
- * the remainder of this function must tolerate losing control at any
- * instant. Likewise, any pg_on_exit_callback registered before or during
- * this function must be prepared to execute at any instant between here
- * and the end of this function. Furthermore, affected callbacks execute
- * partially or not at all when a second exit-inducing signal arrives
- * after proc_exit_prepare() decrements on_proc_exit_index. (Thanks to
- * that mechanic, callbacks need not anticipate more than one call.) This
- * is fragile; it ought to instead follow the norm of handling interrupts
- * at selected, safe opportunities.
- */
- pqsignal(SIGTERM, startup_die);
- pqsignal(SIGQUIT, startup_die);
+ * We arrange to do proc_exit(1) if we receive SIGTERM or timeout while
+ * trying to collect the startup packet; while SIGQUIT results in
+ * _exit(2). Otherwise the postmaster cannot shutdown the database FAST
+ * or IMMED cleanly if a buggy client fails to send the packet promptly.
+ *
+ * XXX this is pretty dangerous; signal handlers should not call anything
+ * as complex as proc_exit() directly. We minimize the hazard by not
+ * keeping these handlers active for longer than we must. However, it
+ * seems necessary to be able to escape out of DNS lookups as well as the
+ * startup packet reception proper, so we can't narrow the scope further
+ * than is done here.
+ *
+ * XXX it follows that the remainder of this function must tolerate losing
+ * control at any instant. Likewise, any pg_on_exit_callback registered
+ * before or during this function must be prepared to execute at any
+ * instant between here and the end of this function. Furthermore,
+ * affected callbacks execute partially or not at all when a second
+ * exit-inducing signal arrives after proc_exit_prepare() decrements
+ * on_proc_exit_index. (Thanks to that mechanic, callbacks need not
+ * anticipate more than one call.) This is fragile; it ought to instead
+ * follow the norm of handling interrupts at selected, safe opportunities.
+ */
+ pqsignal(SIGTERM, startup_packet_die);
+ pqsignal(SIGQUIT, SignalHandlerForCrashExit);
InitializeTimeouts(); /* establishes SIGALRM handler */
PG_SETMASK(&StartupBlockSig);
@@ -4411,8 +4420,8 @@ BackendInitialize(Port *port)
port->remote_hostname = strdup(remote_host);
/*
- * Ready to begin client interaction. We will give up and exit(1) after a
- * time delay, so that a broken client can't hog a connection
+ * Ready to begin client interaction. We will give up and proc_exit(1)
+ * after a time delay, so that a broken client can't hog a connection
* indefinitely. PreAuthDelay and any DNS interactions above don't count
* against the time limit.
*
@@ -4434,6 +4443,12 @@ BackendInitialize(Port *port)
*/
status = ProcessStartupPacket(port, false, false);
+ /*
+ * Disable the timeout, and prevent SIGTERM again.
+ */
+ disable_timeout(STARTUP_PACKET_TIMEOUT, false);
+ PG_SETMASK(&BlockSig);
+
/*
* Stop here if it was bad or a cancel packet. ProcessStartupPacket
* already did any appropriate error reporting.
@@ -4459,12 +4474,6 @@ BackendInitialize(Port *port)
pfree(ps_data.data);
set_ps_display("initializing");
-
- /*
- * Disable the timeout, and prevent SIGTERM/SIGQUIT again.
- */
- disable_timeout(STARTUP_PACKET_TIMEOUT, false);
- PG_SETMASK(&BlockSig);
}
@@ -5359,16 +5368,22 @@ sigusr1_handler(SIGNAL_ARGS)
}
/*
- * SIGTERM or SIGQUIT while processing startup packet.
+ * SIGTERM while processing startup packet.
* Clean up and exit(1).
*
- * XXX: possible future improvement: try to send a message indicating
- * why we are disconnecting. Problem is to be sure we don't block while
- * doing so, nor mess up SSL initialization. In practice, if the client
- * has wedged here, it probably couldn't do anything with the message anyway.
+ * Running proc_exit() from a signal handler is pretty unsafe, since we
+ * can't know what code we've interrupted. But the alternative of using
+ * _exit(2) is also unpalatable, since it'd mean that a "fast shutdown"
+ * would cause a database crash cycle (forcing WAL replay at restart)
+ * if any sessions are in authentication. So we live with it for now.
+ *
+ * One might be tempted to try to send a message indicating why we are
+ * disconnecting. However, that would make this even more unsafe. Also,
+ * it seems undesirable to provide clues about the database's state to
+ * a client that has not yet completed authentication.
*/
static void
-startup_die(SIGNAL_ARGS)
+startup_packet_die(SIGNAL_ARGS)
{
proc_exit(1);
}
@@ -5389,7 +5404,11 @@ dummy_handler(SIGNAL_ARGS)
/*
* Timeout while processing startup packet.
- * As for startup_die(), we clean up and exit(1).
+ * As for startup_packet_die(), we clean up and exit(1).
+ *
+ * This is theoretically just as hazardous as it is in startup_packet_die(),
+ * although in practice we're almost certainly waiting for client input,
+ * which greatly reduces the risk.
*/
static void
StartupPacketTimeoutHandler(void)
