Date: Wed, 9 Sep 2020 10:39:00 -0400
From: Stephen Frost <sfrost@snowman.net>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: SIGQUIT handling, redux
Message-ID: <20200909143900.GQ29590@tamriel.snowman.net>
References: <1850884.1599601164@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="LgqlFyqd4cTt1Pxh"
Content-Disposition: inline
In-Reply-To: <1850884.1599601164@sss.pgh.pa.us>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk


--LgqlFyqd4cTt1Pxh
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> 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.)
>=20
> 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.
>=20
> 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

--LgqlFyqd4cTt1Pxh
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJfWOkEAAoJEO1sijiDR2RVi78QALJV7Cx88+Rb8YGx6rTWNaWZ
TD4gFSFWEJmhlU9eup2jX1oq+GIiXvrMUQ7gCBGhfVPdch2vEqQ5UXEJuC4aE9eY
cAndNZEOzg3L4Ir3Cc8Ua6BkRRHUacYgLzFTE7ithqJ/EWB/gotRCBYh/0bxG1Kk
7gsIAaR7CIUgksKHQDuB5Eyhii2/FcPb6tBoRYSGSZEi2Y8U5kmba+Tva6lV9RUB
5MZfDfGEfHbhjnilw92FW8TLrm6YbkHeh7/tnTLmGUmtLUYoP280yHOCHneRGyry
p7xo/ZEycij/hSs/iYmpCAYpq3v4hCdokDL/wstC0uoS+EhzDdNoraaAyZQIuXYU
NgUCkrqxhbhnhkJ3xT99jeGw/6LkDpojd6FE+dgzpSVIkWnbfRqtXawLbZeIU7BI
Geqva9OLcm8X2O53St8t7JBVI4wn6JISz+Z8yXiHDnvduF0y6Q1OEr76UdCu02HS
ww7TUfLQIGyC/6eqO95bSZ6122gXyt+IiF7N9qz8EpUsofpRkSxbg8mtOiSEKVdc
HTYFdWp7o8PC6hI98KxdxN2rulox7mMySHvZS2KMqjZF8lX6tNxpCLVIohOQzRip
cZqC16Zn5B9GdbU7uS+9VK440cR/ON9Lx7tQrKR6LZjdnmn+kojqHZAcFZi17xmD
Uqbf1M7Eg6jjq5QHH7RM
=wen7
-----END PGP SIGNATURE-----

--LgqlFyqd4cTt1Pxh--