Subject: SIGQUIT handling, redux

From: Tom Lane <[email-masked]>
Date: Tue, 08 Sep 2020 17:39:24 -0400

This is to pull together a couple of recent threads that are seemingly
unrelated to $SUBJECT.

Over in the long thread at [1] we've agreed to try to make the backend
code actually do what assorted comments claim it does, namely allow
SIGQUIT to be accepted at any point after a given process has established
its signal handlers.  (Right now, it mostly is not accepted during error
recovery, but there's no clear reason to preserve that exception.)

Of course, this is only safe if the SIGQUIT handler is safe to be invoked
anywhere, so I did a quick survey of backend signal handlers to see if
that is true.  I immediately found pgarch_exit() which surely is not.  It
turns out that Horiguchi-san already noticed that and proposed to fix it,
within the seemingly entirely unrelated patch series for a shared-memory
based stats collector (see patch 0001 in [2]).  I think we should go ahead
and commit that, and maybe even back-patch it.  There seems no good reason
for the archiver to treat SIGQUIT as nonfatal when other postmaster
children do not.

Another thing I found is startup_die() in postmaster.c, which catches
SIGQUIT during the authentication phase of a new backend.  This calls
proc_exit(1), which (a) is unsafe on its face, and (b) potentially
demotes a SIGQUIT into something that will not cause the parent postmaster
to perform a DB-wide restart.  There seems no good reason for that
behavior, either.  I propose that we use SignalHandlerForCrashExit
for SIGQUIT here too.

In passing, it's worth noting that startup_die() isn't really much safer
for SIGTERM than it is for SIGQUIT; the only argument for distinguishing
those is that code that applies BlockSig will at least manage to block the
former.  So it's slightly tempting to drop startup_die() altogether in
favor of using SignalHandlerForCrashExit for the SIGTERM-during-auth case
too.  However, that'd have the effect of causing a "fast shutdown" to get
converted to a panic if it happens while any sessions are in
authentication phase, so I think this cure is likely worse than the
disease.

Worth reading in connection with this is the thread that led up to
commits 8e19a8264 et al [3].  We had a long discussion about how
quickdie() and startup_die() might be made safe(r), without any
real consensus emerging about any alternatives being better than the
status quo.  I still don't have an improvement idea for quickdie();
I don't want to give up trying to send a message to the client.
Note, however, that quickdie() does end with _exit(2) so that at
least it's not trying to execute arbitrary process-cleanup code.

In short then, I want to ensure that postmaster children's SIGQUIT
handlers always end with _exit(2) rather than some other exit method.
We have two exceptions now and they need to get fixed.

			regards, tom lane

[1] https://www.postgresql.org/message-id/flat/CALDaNm1d1hHPZUg3xU4XjtWBOLCrA%2B-2cJcLpw-cePZ%3DGgDVfA%40mail.gmail.com
[2] https://www.postgresql.org/message-id/20200908.175557.617150409868541587.horikyota.ntt%40gmail.com
[3] https://www.postgresql.org/message-id/flat/CAOMx_OAuRUHiAuCg2YgicZLzPVv5d9_H4KrL_OFsFP%3DVPekigA%40mail.gmail.com

---

From: Stephen Frost <[email-masked]>
Date: Wed, 9 Sep 2020 10:39:00 -0400

Greetings,

> for the archiver to treat SIGQUIT as nonfatal when other postmaster

As I mentioned over there, I agree that we should do this and we should
further have the statistics collector also do so, which currently sets
up SIGQUIT with ShutdownRequestPending() and in its loop decides it's
fine to write out the stats file (which we're going to remove during
recovery anyway...) and then call exit(0).  I also think we should
back-patch these changes, as the commit mentioned in Horiguchi-san's
patch for pgarch_exit() was.

> behavior, either.  I propose that we use SignalHandlerForCrashExit

Yikes, agreed.

> authentication phase, so I think this cure is likely worse than the

Agreed, that's definitely no good.

> We have two exceptions now and they need to get fixed.

I agree we should fix these.

Thanks,

Stephen

---

From: Tom Lane <[email-masked]>
Date: Wed, 09 Sep 2020 10:46:28 -0400

> recovery anyway...) and then call exit(0).

I noticed that that was different from everything else, but it's not
actually signal-unsafe, so it seems like a different topic from what
I'm on about at the moment.  I don't mind if you or somebody else
wants to change it, but I don't see it as a back-patchable bug fix.

> I also think we should

Agreed; I'll go make it happen.

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Wed, 09 Sep 2020 10:55:23 -0400

> wants to change it, but I don't see it as a back-patchable bug fix.

Note also that the postmaster actually uses SIGQUIT to command normal
shutdown of the stats collector (cf reaper(), around line 3125 in HEAD).
So this needs a change in signaling conventions, not just internal
tweaks in the collector.  Not a big deal, but it reinforces my feeling
that it should be a HEAD-only change.

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Wed, 9 Sep 2020 12:26:20 -0700

Hi,

> for the archiver to treat SIGQUIT as nonfatal when other postmaster

+1

> behavior, either.  I propose that we use SignalHandlerForCrashExit

Yikes, yea, that seems like an important change.

I wish startup_die() weren't named startup_ - every single time I see
the name I think it's about the startup process...

I think StartupPacketTimeoutHandler is another case?

> former.

Which is pretty unconvincing...

I wonder if we could at least *try* to rely on CFR() for a while. It'd
not be pretty, but probably doable, to set up a timer inside the signal
handler and try to exit using normal mechanisms for a while (some
overlap with authentication timeout).

That'd leave the question of what we do once that timer expires. There's
quite a few possible ways that could reproducibly happen, e.g. if DNS
lookups are required during auth.

The long term correct way to handle this would obviously be to
restructure everything that happens covered by startup_die() in a
non-blocking manner and just rely on CFR(). But that's a tall order to
get done anytime soon, particularly things like DNS are IIRC pretty hard
without relying on custom libraries.

Shorter term I don't really see an alternative to escalating to SIGQUIT
which is obviously terrible.

> So it's slightly tempting to drop startup_die() altogether in favor of
> using SignalHandlerForCrashExit for the SIGTERM-during-auth case too.
> However, that'd have the effect of causing a "fast shutdown" to get

Indeed.

> I don't want to give up trying to send a message to the client.

That still doesn't make much sense to me. The potential for hanging
(e.g. inside malloc) is so much worse than not sending a message... And
we already infer things about the server dying in libpq when the
connection just closes (which I am admittedly also not happy about). But
I also think we can reduce the risk a bit, see below.

I only had one coffee so far (and it looks like the sun has died
outside), so maybe I'm just slow: But, uh, we don't currently send a
message startup_die(), right?

So that part is about quickdie()?

> status quo.  I still don't have an improvement idea for quickdie();

I still think that we should at least mitigate the risk to hang inside
quickdie() by at least disabling translations (since the rest happens
inside the pre-allocated error memory context, which should lower the
risk).  And perhaps do similarly for the memory required for encryption,
if enabled, and where possible.

Greetings,

Andres Freund

---

From: Tom Lane <[email-masked]>
Date: Wed, 09 Sep 2020 16:09:00 -0400

> the name I think it's about the startup process...

We could call it startup_packet_die or something?

> I think StartupPacketTimeoutHandler is another case?

Yeah.  Although it's a lot less risky, since if the timeout is reached
we're almost certainly waiting for client input.

> Which is pretty unconvincing...

Agreed, it'd be nice if this were less shaky.  On the other hand,
we've seen darn few complaints traceable to this AFAIR.  I'm not
really sure it's worth putting a lot of effort into.

> without relying on custom libraries.

Not only DNS, but all the various auth libraries would have to be
contended with.  Lots of work there compared to the likely rewards.

> (e.g. inside malloc) is so much worse than not sending a message...

We see backends going through this code on a very regular basis in the
buildfarm, but complete hangs are rare as can be.  I think you
overestimate the severity of the problem.

> So that part is about quickdie()?

Right.  Note that startup_die() is pre-authentication, so I'm doubtful
that we should tell the would-be client anything about the state of
the server at that point, even ignoring these risk factors.  (I'm a
bit inclined to remove the comment suggesting that'd be desirable.)

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Wed, 9 Sep 2020 13:22:01 -0700

Hi,

> We could call it startup_packet_die or something?

Yea, I think that'd be good.

> we're almost certainly waiting for client input.

An adversary could control that to a significant degree - but ordinarily
I agree...

> really sure it's worth putting a lot of effort into.

Not sure how many would plausibly reach us though.  A common reaction
will likely just to be to force-restart the server, not to fully
investigate the issue. Particularly because it'll often be once
something already has gone wrong...

> overestimate the severity of the problem.

I don't think the BF exercises the problmetic paths to a significant
degree. It's mostly local socket connections, and where not it's
localhost. There's no slow DNS, no more complicated authentication
methods, no packet loss. How often do we ever actually end up even
getting close to any of the paths but immediate shutdowns? And in the
SIGQUIT path, how often do we end up in the SIGKILL path, masking
potential deadlocks?

> bit inclined to remove the comment suggesting that'd be desirable.)

Yea, I think just putting in an editorialized version of your paragraph
would make sense.

Greetings,

Andres Freund

---

From: Tom Lane <[email-masked]>
Date: Wed, 09 Sep 2020 16:30:37 -0400

> Yea, I think that'd be good.

I'll make it so.

> getting close to any of the paths but immediate shutdowns?

Since we're talking about quickdie(), immediate shutdown/crash restart
is exactly the case of concern, and the buildfarm exercises it all the
time.

> And in the

True, we can't really tell that.  I wonder if we should make the
postmaster emit a log message when it times out and goes to SIGKILL.
After a few months we could scrape the buildfarm logs and get a
pretty good handle on it.

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Wed, 09 Sep 2020 16:50:26 -0400

> contended with.  Lots of work there compared to the likely rewards.

Wait a minute.  The entire authentication cycle happens inside
InitPostgres, using the backend's normal signal handlers.  So
maybe we are overthinking the problem.  What if we simply postpone
ProcessStartupPacket into that same place, and run it under the same
rules as we do for authentication?  We would waste more cycles than
we do now for the case where the client closes the connection without
sending a startup packet, but not enormously so, I think --- and
optimizing that case doesn't seem like a high-priority goal anyway.
And cases like DNS lookup taking forever don't seem like any more of
an issue than auth lookup taking forever.

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Wed, 9 Sep 2020 14:06:54 -0700

Hi,

> > Yea, I think that'd be good.

Thanks!

> is exactly the case of concern, and the buildfarm exercises it all the

Yea, but only in simple cases. Largely no SSL / kerberos. Largely
untranslated. Mostly the immediate shutdowns aren't when inside plpython
or such.

> pretty good handle on it.

I think that'd be a good idea.

Greetings,

Andres Freund

---

From: Tom Lane <[email-masked]>
Date: Wed, 09 Sep 2020 18:00:41 -0400

> rules as we do for authentication?

Or, continuing to look for other answers ...

During BackendInitialize we have not yet touched shared memory.
This is not incidental but must be so, per its header comment.
Therefore it seems like we could have these signal handlers (for
SIGTERM or timeout) do _exit(1), thereby resolving the signal
unsafety while not provoking a database restart.  We don't
care whether inside-the-process state is sane, and we shouldn't
have changed anything yet in shared memory.

This is obviously not 100.00% safe, since maybe something could
violate these assumptions, but it seems a lot safer than using
proc_exit() from a signal handler.

One way to help catch any such assumption-violations is to add
an assertion that no on_shmem_exit handlers have been set up yet.
If there are none, there should be no expectation of having to
clean up shared state.

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Wed, 09 Sep 2020 19:24:55 -0400

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)

---

From: Tom Lane <[email-masked]>
Date: Wed, 09 Sep 2020 22:07:06 -0400

I wrote:
> that is true.

This is straying a bit from the stated topic of this thread, but ...
I did some further looking around to see whether there were any
unsafe signal handlers besides SIGQUIT ones.  The situation is not
too awful, but I did find several issues not already mentioned
in this thread:

StartupProcShutdownHandler (SIGTERM)

This conditionally calls proc_exit(1).  The conditions boil down
to are-we-interrupting-a-system(3)-call, so who knows how safe
that is?  I wouldn't care to bet that system() doesn't use malloc,
for instance.  Still, the odds are very good that if a signal did
arrive, it'd be interrupting system()'s waitpid() or equivalent
kernel call, which is likely safe enough.

bgworker_die (SIGTERM)

Calls ereport(FATAL).  This is surely not any safer than, say,
quickdie().  No, it's worse, because at least that won't try
to go out via proc_exit().

FloatExceptionHandler (SIGFPE)

Calls ereport(ERROR).  This might be okay, though, since the
trap should be synchronous with the offending calculation.
Besides, if you're risking divide by zero or the like in
critical code, You're Doing It Wrong.

RecoveryConflictInterrupt (called from SIGUSR1)

Calls a whole boatload of state tests that were never designed
to be interrupt-safe, such as transaction-state-related inquiries
in xact.c.  The lack of any explicit awareness in this code that
it's in a signal handler doesn't discourage people from inserting
even more dubious stuff.  I think this needs to be burned to the
ground and rewritten.

StandbyDeadLockHandler (from SIGALRM)
StandbyTimeoutHandler (ditto)

Calls CancelDBBackends, which just for starters tries to acquire
an LWLock.  I think the only reason we've gotten away with this
for this long is the high probability that by the time either
timeout fires, we're going to be blocked on a semaphore.

I don't have any ideas about how to fix any of these things,
but I thought getting the problems on the record would be good.

			regards, tom lane

---

From: "[email-masked]" <[email-masked]>
Date: Thu, 10 Sep 2020 02:56:37 +0000

From: Tom Lane <[email-masked]>
> too awful, but I did find several issues not already mentioned

Wow, your eyes catch this many issues.  (I was just wondering about one or two of them.)

I'm not sure if this is related, but I had been wondering if the following can be what it is now.

(1)
When logical replication is used, pg_ctl stop with the default fast mode emits the message about termination of logical replication launcher.  Although it's not FATAL or ERROR, but I was a bit startled when I saw this message for the first time.  Why should this message be emitted while nothing about other postmaster children are reported?  The logical rep launcher ignores SIGINT (SIG_IGN).

LOG:  received fast shutdown request
LOG:  aborting any active transactions
LOG:  background worker "logical replication launcher" (PID 10056) exited with exit code 1
LOG:  shutting down
LOG:  database system is shut down

(2)
When the physical standby stops, a FATAL message is output.  It may be annoying to the DBA that monitors messages with high severity.

LOG:  received fast shutdown request
LOG:  aborting any active transactions
FATAL:  terminating walreceiver process due to administrator command
LOG:  shutting down
LOG:  database system is shut down

(3)
When WaitLatch(EXIT_ON_POSTMASTER_DEATH) detects postmaster death, it calls proc_exit(1).  Can we call _exit(1) here likewise?

Regards
Takayuki Tsunakawa

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Thu, 10 Sep 2020 19:02:06 +0900 (JST)

At Wed, 09 Sep 2020 10:46:28 -0400, Tom Lane <[email-masked]> wrote in 
> Agreed; I'll go make it happen.

Thank you for committing this!

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

---

From: Robert Haas <[email-masked]>
Date: Thu, 10 Sep 2020 10:36:23 -0400

> to go out via proc_exit().

I think bgworker_die() is pretty much a terrible idea. Every
background worker I've written has actually needed to use
CHECK_FOR_INTERRUPTS(). I think that the only way this could actually
be safe is if you have a background worker that never uses ereport()
itself, so that the ereport() in the signal handler can't be
interrupting one that's already happening. This seems unlikely to be
the normal case, or anything close to it. Most background workers
probably are shared-memory connected and use a lot of PostgreSQL
infrastructure and thus ereport() all over the place.

Now what to do about it I don't know exactly, but it would be nice to
do something.

> timeout fires, we're going to be blocked on a semaphore.

Yeah, I'm not sure these are so bad. In fact, in the deadlock case, I
believe the old coding was designed to make sure we *had to* be
blocked on a semaphore, but I'm not sure whether that's still true.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

---

From: Tom Lane <[email-masked]>
Date: Thu, 10 Sep 2020 12:56:55 -0400

> I think bgworker_die() is pretty much a terrible idea.

Yeah.  It'd likely be better to insist that bgworkers handle SIGTERM
the same way backends do, via CHECK_FOR_INTERRUPTS.  Not sure how big
a change that would be.

> I think that the only way this could actually
> interrupting one that's already happening.

That doesn't begin to make it safe, because quite aside from anything
that happens in elog.c, we're then going to go out via proc_exit()
which will invoke who knows what.

> blocked on a semaphore, but I'm not sure whether that's still true.

Looking at this closer, the only code that could get interrupted
by these timeouts is a call to ProcWaitForSignal, which is

void
ProcWaitForSignal(uint32 wait_event_info)
{
    (void) WaitLatch(MyLatch, WL_LATCH_SET | WL_EXIT_ON_PM_DEATH, 0,
                     wait_event_info);
    ResetLatch(MyLatch);
    CHECK_FOR_INTERRUPTS();
}

Interrupting the latch operations might be safe enough,
although now I'm casting a side-eye at Munro's recent
changes to share WaitEvent state all over the place.
I wonder whether blocking on an LWLock inside the
signal handler will break an interrupted WaitLatch.

Also, man that CHECK_FOR_INTERRUPTS() looks like trouble.
Could we take that out?

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Thu, 10 Sep 2020 14:26:59 -0400

> Also, man that CHECK_FOR_INTERRUPTS() looks like trouble.

Maybe I'm missing something, but why wouldn't that be a horrible idea?
We do not want to have long waits where we refuse to respond to
interrupts.

---

From: Tom Lane <[email-masked]>
Date: Thu, 10 Sep 2020 16:29:23 -0400

> We do not want to have long waits where we refuse to respond to

It might be appropriate for some of the callers to do it.  But I don't
see any great argument why ProcWaitForSignal itself has to do it.

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Thu, 10 Sep 2020 22:40:06 -0400

> but I'd be pretty hesitant to back-patch that.

Here's a patch for that; it passes light testing, including forcing
the backend through the SIGTERM and timeout code paths.  There's
not much to it except for changing the signal handlers, but I did
add a cross-check that no on-exit handlers have been registered
before we get done with using these signal handlers.

I looked briefly at the idea of postponing ProcessStartupPacket
until InitPostgres has set up a fairly normal environment.  It
seems like it'd take a fair amount of refactoring.  I really
doubt it's worth the effort, even though the result would be
arguably cleaner logic than what we have now.

			regards, tom lane

diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 15ad675bc1..081022a206 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -4298,6 +4298,8 @@ report_fork_failure_to_client(Port *port, int errnum)
  * returns: nothing.  Will not return at all if there's any failure.
  *
  * Note: this code does not depend on having any access to shared memory.
+ * Indeed, our approach to SIGTERM/timeout handling *requires* that
+ * shared memory not have been touched yet; see comments within.
  * In the EXEC_BACKEND case, we are physically attached to shared memory
  * but have not yet set up most of our local pointers to shmem structures.
  */
@@ -4341,27 +4343,14 @@ BackendInitialize(Port *port)
 	whereToSendOutput = DestRemote; /* now safe to ereport to client */
 
 	/*
-	 * 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.
+	 * We arrange to do _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.
+	 * Exiting with _exit(1) is only possible because we have not yet touched
+	 * shared memory; therefore no outside-the-process state needs to get
+	 * cleaned up.
 	 */
 	pqsignal(SIGTERM, process_startup_packet_die);
 	pqsignal(SIGQUIT, SignalHandlerForCrashExit);
@@ -4420,8 +4409,8 @@ BackendInitialize(Port *port)
 		port->remote_hostname = strdup(remote_host);
 
 	/*
-	 * 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
+	 * 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
 	 * indefinitely.  PreAuthDelay and any DNS interactions above don't count
 	 * against the time limit.
 	 *
@@ -4449,6 +4438,17 @@ BackendInitialize(Port *port)
 	disable_timeout(STARTUP_PACKET_TIMEOUT, false);
 	PG_SETMASK(&BlockSig);
 
+	/*
+	 * As a safety check that nothing in startup has yet performed
+	 * shared-memory modifications that would need to be undone if we had
+	 * exited through SIGTERM or timeout above, check that no on_shmem_exit
+	 * handlers have been registered yet.  (This isn't terribly bulletproof,
+	 * since someone might misuse an on_proc_exit handler for shmem cleanup,
+	 * but it's a cheap and helpful check.  We cannot disallow on_proc_exit
+	 * handlers unfortunately, since pq_init() already registered one.)
+	 */
+	check_on_shmem_exit_lists_are_empty();
+
 	/*
 	 * Stop here if it was bad or a cancel packet.  ProcessStartupPacket
 	 * already did any appropriate error reporting.
@@ -5369,23 +5369,21 @@ sigusr1_handler(SIGNAL_ARGS)
 
 /*
  * SIGTERM while processing startup packet.
- * Clean up and exit(1).
  *
- * 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.
+ * Running proc_exit() from a signal handler would be quite unsafe.
+ * However, since we have not yet touched shared memory, we can just
+ * pull the plug and exit without running any atexit handlers.
  *
- * 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.
+ * One might be tempted to try to send a message, or log one, indicating
+ * why we are disconnecting.  However, that would be quite unsafe in itself.
+ * Also, it seems undesirable to provide clues about the database's state
+ * to a client that has not yet completed authentication, or even sent us
+ * a startup packet.
  */
 static void
 process_startup_packet_die(SIGNAL_ARGS)
 {
-	proc_exit(1);
+	_exit(1);
 }
 
 /*
@@ -5404,16 +5402,12 @@ dummy_handler(SIGNAL_ARGS)
 
 /*
  * Timeout while processing startup packet.
- * As for process_startup_packet_die(), we clean up and exit(1).
- *
- * This is theoretically just as hazardous as in process_startup_packet_die(),
- * although in practice we're almost certainly waiting for client input,
- * which greatly reduces the risk.
+ * As for process_startup_packet_die(), we exit via _exit(1).
  */
 static void
 StartupPacketTimeoutHandler(void)
 {
-	proc_exit(1);
+	_exit(1);
 }

diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c
index 11c3f132a1..36a067c924 100644
--- a/src/backend/storage/ipc/ipc.c
+++ b/src/backend/storage/ipc/ipc.c
@@ -416,3 +416,20 @@ on_exit_reset(void)
 	on_proc_exit_index = 0;
 	reset_on_dsm_detach();
 }
+
+/* ----------------------------------------------------------------
+ *		check_on_shmem_exit_lists_are_empty
+ *
+ *		Debugging check that no shmem cleanup handlers have been registered
+ *		prematurely in the current process.
+ * ----------------------------------------------------------------
+ */
+void
+check_on_shmem_exit_lists_are_empty(void)
+{
+	if (before_shmem_exit_index)
+		elog(FATAL, "before_shmem_exit has been called prematurely");
+	if (on_shmem_exit_index)
+		elog(FATAL, "on_shmem_exit has been called prematurely");
+	/* Checking DSM detach state seems unnecessary given the above */
+}
diff --git a/src/include/storage/ipc.h b/src/include/storage/ipc.h
index 462fe46341..88994fdc26 100644
--- a/src/include/storage/ipc.h
+++ b/src/include/storage/ipc.h
@@ -72,6 +72,7 @@ extern void on_shmem_exit(pg_on_exit_callback function, Datum arg);
 extern void before_shmem_exit(pg_on_exit_callback function, Datum arg);
 extern void cancel_before_shmem_exit(pg_on_exit_callback function, Datum arg);
 extern void on_exit_reset(void);
+extern void check_on_shmem_exit_lists_are_empty(void);
 
 /* ipci.c */
 extern PGDLLIMPORT shmem_startup_hook_type shmem_startup_hook;

---

From: Tom Lane <[email-masked]>
Date: Fri, 11 Sep 2020 11:52:55 -0400

> arguably cleaner logic than what we have now.

I felt more ambitious this morning and decided to take a harder look
at this idea.  But I soon realized that there would be a concrete
disadvantage to delaying ProcessStartupPacket: until we have done that,
we don't have the correct value for FrontendProtocol so there is a
problem with reporting startup-time failures to the client.  At least
some such failures, such as "too many clients already", are pretty
routine so we don't want to downgrade their user-friendliness.

If memory serves, libpq has some ability to cope with a v2 error message
even when it's expecting v3.  But I wouldn't bet on that being true of
all client libraries, and anyway it's a very under-tested code path.

So I think we'd better go with the simple fix I showed before.
It's simple enough that maybe we could back-patch it, once it's
aged awhile in HEAD.  OTOH, given the lack of field reports of
trouble here, I'm not sure back-patching is worth the risk.

			regards, tom lane