Horton

Thread

Commits

  1. Log a message when resorting to SIGKILL during shutdown/crash recovery.

  2. Don't run atexit callbacks during signal exits from ProcessStartupPacket.

  3. Use _exit(2) for SIGQUIT during ProcessStartupPacket, too.

  4. Make archiver's SIGQUIT handler exit via _exit().

  1. SIGQUIT handling, redux

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-08T21:39:24Z

    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

  2. Re: SIGQUIT handling, redux

    Stephen Frost <sfrost@snowman.net> — 2020-09-09T14:39:00Z

    Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> 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.

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.

> 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.

Yikes, agreed.

> 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.

Agreed, that's definitely no good.

> 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.

I agree we should fix these.

Thanks,

Stephen

  3. Re: SIGQUIT handling, redux

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-09T14:46:28Z

    Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> 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.

> 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 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
> back-patch these changes, as the commit mentioned in Horiguchi-san's
> patch for pgarch_exit() was.

Agreed; I'll go make it happen.

			regards, tom lane

  4. Re: SIGQUIT handling, redux

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-09T14:55:23Z

    I wrote:
> Stephen Frost <sfrost@snowman.net> writes:
>> 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 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.

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

  5. Re: SIGQUIT handling, redux

    Andres Freund <andres@anarazel.de> — 2020-09-09T19:26:20Z

    Hi,

On 2020-09-08 17:39:24 -0400, Tom Lane wrote:
> 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.

+1


> 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.

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?



> 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.

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
> 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.

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()?


> 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 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

  6. Re: SIGQUIT handling, redux

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-09T20:09:00Z

    Andres Freund <andres@anarazel.de> writes:
> I wish startup_die() weren't named startup_ - every single time I see
> 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.

>> 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.

> 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.

> 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.

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

>> 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...

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.

> 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()?

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

  7. Re: SIGQUIT handling, redux

    Andres Freund <andres@anarazel.de> — 2020-09-09T20:22:01Z

    Hi,

On 2020-09-09 16:09:00 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > I wish startup_die() weren't named startup_ - every single time I see
> > the name I think it's about the startup process...
> 
> We could call it startup_packet_die or something?

Yea, I think that'd be good.


> > 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.

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


> >> 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.
> 
> > 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.

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...



> >> 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...
> 
> 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.

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?


> > 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()?
> 
> 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.)

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

Greetings,

Andres Freund

  8. Re: SIGQUIT handling, redux

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-09T20:30:37Z

    Andres Freund <andres@anarazel.de> writes:
> On 2020-09-09 16:09:00 -0400, Tom Lane wrote:
>> We could call it startup_packet_die or something?

> Yea, I think that'd be good.

I'll make it so.

>> 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.

> 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?

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
> SIGQUIT path, how often do we end up in the SIGKILL path, masking
> potential deadlocks?

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

  9. Re: SIGQUIT handling, redux

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-09T20:50:26Z

    I wrote:
> Not only DNS, but all the various auth libraries would have to be
> 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

  10. Re: SIGQUIT handling, redux

    Andres Freund <andres@anarazel.de> — 2020-09-09T21:06:54Z

    Hi,

On 2020-09-09 16:30:37 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2020-09-09 16:09:00 -0400, Tom Lane wrote:
> >> We could call it startup_packet_die or something?
> 
> > Yea, I think that'd be good.
> 
> I'll make it so.

Thanks!


> >> 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.
> 
> > 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?
> 
> Since we're talking about quickdie(), immediate shutdown/crash restart
> is exactly the case of concern, and the buildfarm exercises it all the
> time.

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


> > And in the
> > SIGQUIT path, how often do we end up in the SIGKILL path, masking
> > potential deadlocks?
> 
> 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.

I think that'd be a good idea.

Greetings,

Andres Freund

  11. Re: SIGQUIT handling, redux

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-09T22:00:41Z

    I wrote:
>> Not only DNS, but all the various auth libraries would have to be
>> 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?

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

  12. Re: SIGQUIT handling, redux

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-09T23:24:55Z

    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

  13. Re: SIGQUIT handling, redux

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-10T02:07:06Z

    I wrote:
> 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.

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

  14. RE: SIGQUIT handling, redux

    tsunakawa.takay@fujitsu.com <tsunakawa.takay@fujitsu.com> — 2020-09-10T02:56:37Z

    From: Tom Lane <tgl@sss.pgh.pa.us>
> 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:

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

  15. Re: SIGQUIT handling, redux

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-09-10T10:02:06Z

    At Wed, 09 Sep 2020 10:46:28 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> > I also think we should
> > back-patch these changes, as the commit mentioned in Horiguchi-san's
> > patch for pgarch_exit() was.
> 
> Agreed; I'll go make it happen.

Thank you for committing this!

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  16. Re: SIGQUIT handling, redux

    Robert Haas <robertmhaas@gmail.com> — 2020-09-10T14:36:23Z

    On Wed, Sep 9, 2020 at 10:07 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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().

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.

> 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.

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

  17. Re: SIGQUIT handling, redux

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-10T16:56:55Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Sep 9, 2020 at 10:07 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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().

> 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
> 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.

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.

>> 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.

> 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.

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

  18. Re: SIGQUIT handling, redux

    Robert Haas <robertmhaas@gmail.com> — 2020-09-10T18:26:59Z

    On Thu, Sep 10, 2020 at 12:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Also, man that CHECK_FOR_INTERRUPTS() looks like trouble.
> Could we take that out?

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.

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

  19. Re: SIGQUIT handling, redux

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-10T20:29:23Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Sep 10, 2020 at 12:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Also, man that CHECK_FOR_INTERRUPTS() looks like trouble.
>> Could we take that out?

> 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.

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

  20. Re: SIGQUIT handling, redux

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-11T02:40:06Z

    I wrote:
> I'll take a closer look at the idea of using _exit(1) tomorrow,
> 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

  21. Re: SIGQUIT handling, redux

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-11T15:52:55Z

    I wrote:
> 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.

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

  22. Re: SIGQUIT handling, redux

    Andres Freund <andres@anarazel.de> — 2023-08-02T16:25:50Z

    Hi,

On 2020-09-11 11:52:55 -0400, Tom Lane wrote:
> 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.

FWIW, looking at collected stack traces in azure, there's a slow but steady
stream of crashes below StartupPacketTimeoutHandler. Most seem to be things
like
libcrypto->malloc->StartupPacketTimeoutHandler->proc_exit->socket_close->free->crash
there's a few other variants, some where the stack apparently was not
decipherable for the relevant tooling.

Note that this wouldn't even include cases where this caused hangs - which is
quite common IME.


Unsurprisingly just in versions before 14, where this change went in.

I think that might be enough evidence for backpatching the commit? I've not
heard of issues due to the checks in check_on_shmem_exit_lists_are_empty().

Greetings,

Andres Freund

  23. Re: SIGQUIT handling, redux

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-08-02T16:35:19Z

    Andres Freund <andres@anarazel.de> writes:
> On 2020-09-11 11:52:55 -0400, Tom Lane wrote:
>> 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.

> FWIW, looking at collected stack traces in azure, there's a slow but steady
> stream of crashes below StartupPacketTimeoutHandler. ...
> Unsurprisingly just in versions before 14, where this change went in.
> I think that might be enough evidence for backpatching the commit? I've not
> heard of issues due to the checks in check_on_shmem_exit_lists_are_empty().

I'd be willing to take a look at this in a few weeks when $real_life
is a bit less demanding.  Right before minor releases would likely be
a bad idea anyway.

			regards, tom lane

  24. Re: SIGQUIT handling, redux

    Andres Freund <andres@anarazel.de> — 2023-08-02T16:48:40Z

    Hi,

On 2023-08-02 12:35:19 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2020-09-11 11:52:55 -0400, Tom Lane wrote:
> >> 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.
> 
> > FWIW, looking at collected stack traces in azure, there's a slow but steady
> > stream of crashes below StartupPacketTimeoutHandler. ...
> > Unsurprisingly just in versions before 14, where this change went in.
> > I think that might be enough evidence for backpatching the commit? I've not
> > heard of issues due to the checks in check_on_shmem_exit_lists_are_empty().
> 
> I'd be willing to take a look at this in a few weeks when $real_life
> is a bit less demanding.

Cool.


> Right before minor releases would likely be a bad idea anyway.

Agreed. I had just waded through the stacks, so I thought it'd be worth
bringing up, didn't intend to imply we should backpatch immediately.


Aside: Analyzing this kind of thing at scale is made considerably more painful
by "expected" ereport(PANIC)s (say out of disk space during WAL writes)
calling abort() and dumping core... While there are other PANICs you really
want cores of.

Greetings,

Andres Freund