Improving the comments in pqsignal()

Started by Thomas Munroabout 2 years ago3 messages
#1Thomas Munro
thomas.munro@gmail.com
1 attachment(s)

Hi,

While following along with Tristan and Heikki's thread about signals
in psql, it occurred to me that the documentation atop pqsignal() is
not very good:

* we don't explain what problem it originally solved
* we don't explain why it's still needed today
* we don't explain what else it does for us today
* we describe the backend implementation for Windows incorrectly (mea culpa)
* we vaguely mention one issue with Windows frontend code, but I
think the point made is misleading, and we don't convey the scale of
the differences

Here is my attempt to improve it.

Attachments:

0001-Improve-comments-about-pqsignal.patchtext/x-patch; charset=US-ASCII; name=0001-Improve-comments-about-pqsignal.patchDownload
From 49b979d9a8333169aed4f1bc549f66e0322f01b0 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Thu, 23 Nov 2023 17:30:15 +1300
Subject: [PATCH] Improve comments about pqsignal().

Explain where pqsignal() came from, what problem it originally solved
without assuming the reader is familiar with historical Unixen, why we
still need it, what it does for us now, and the key differences in
frontend code on Windows.
---
 src/port/pqsignal.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/src/port/pqsignal.c b/src/port/pqsignal.c
index 83d876db6c..817a0a5030 100644
--- a/src/port/pqsignal.c
+++ b/src/port/pqsignal.c
@@ -11,16 +11,31 @@
  * IDENTIFICATION
  *	  src/port/pqsignal.c
  *
- *	We now assume that all Unix-oid systems have POSIX sigaction(2)
- *	with support for restartable signals (SA_RESTART).  We used to also
- *	support BSD-style signal(2), but there really shouldn't be anything
- *	out there anymore that doesn't have the POSIX API.
+ *	This is program 10.12 from Advanced Programming in the UNIX Environment,
+ *	with minor changes.  It was originally a replacement needed for old SVR4
+ *	systems whose signal() behaved as if sa_flags = SA_RESETHAND | SA_NODEFER,
+ *	also known as "unreliable" signals due to races when the handler was reset.
+ *
+ *	By now, all known modern Unix systems have a "reliable" signal() call.
+ *	We still don't want to use it though, because it remains
+ *	implementation-defined by both C99 and POSIX whether the handler is reset
+ *	or signals are blocked when the handler runs, and default restart behavior
+ *	is underdocumented.  Therefore we take POSIX's advice and call sigaction()
+ *	so we can provide explicit sa_flags, but wrap it in this more convenient
+ *	traditional interface style.  It also provides a place to set any extra
+ *	flags we want everywhere, such as SA_NOCLDSTOP.
  *
  *	Windows, of course, is resolutely in a class by itself.  In the backend,
- *	we don't use this file at all; src/backend/port/win32/signal.c provides
- *	pqsignal() for the backend environment.  Frontend programs can use
- *	this version of pqsignal() if they wish, but beware that this does
- *	not provide restartable signals on Windows.
+ *	this relies on pqsigaction() in src/backend/port/win32/signal.c, which
+ *	provides limited emulation of reliable signals.  Frontend programs can use
+ *	this version of pqsignal() to forward to the native Windows signal() call
+ *	if they wish, but beware that Windows signals behave quite differently.
+ *	Only the 6 signals required by C are supported.  SIGINT handlers run in
+ *	another thread instead of interrupting an existing thread, and the others
+ *	don't interrupt system calls either, so SA_RESTART is moot.  All except
+ *	SIGFPE have SA_RESETHAND semantics, meaning the handler is reset to SIG_DFL
+ *	each time it runs.  The set of things you are allowed to do in a handler is
+ *	also much more restricted than on Unix, according to the documentation.
  *
  * ------------------------------------------------------------------------
  */
-- 
2.42.1

#2Heikki Linnakangas
hlinnaka@iki.fi
In reply to: Thomas Munro (#1)
Re: Improving the comments in pqsignal()

On 24/11/2023 00:33, Thomas Munro wrote:

Hi,

While following along with Tristan and Heikki's thread about signals
in psql, it occurred to me that the documentation atop pqsignal() is
not very good:

* we don't explain what problem it originally solved
* we don't explain why it's still needed today
* we don't explain what else it does for us today
* we describe the backend implementation for Windows incorrectly (mea culpa)
* we vaguely mention one issue with Windows frontend code, but I
think the point made is misleading, and we don't convey the scale of
the differences

Here is my attempt to improve it.

Thanks!

This is program 10.12 from Advanced Programming in the UNIX
Environment, with minor changes.

In the copy I found online (3rd edition), it's "Figure 10.18", not
"program 10.12".

Other than that, looks good.

--
Heikki Linnakangas
Neon (https://neon.tech)

#3Thomas Munro
thomas.munro@gmail.com
In reply to: Heikki Linnakangas (#2)
Re: Improving the comments in pqsignal()

On Fri, Nov 24, 2023 at 8:55 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:

On 24/11/2023 00:33, Thomas Munro wrote:

This is program 10.12 from Advanced Programming in the UNIX
Environment, with minor changes.

In the copy I found online (3rd edition), it's "Figure 10.18", not
"program 10.12".

Other than that, looks good.

Thanks. I removed that number (it's easy enough to find), replaced
"underdocumented" with "unspecified" (a word from the later edition of
Stevens) and added a line break to break up that final paragraph, and
pushed. Time to upgrade my treeware copy of that book...

One thing I worried about while writing that text: why is it OK that
win32_port.h redefines SIG_DFL etc, if they might be exposed to the
system signal()? But it seems we picked the same numerical values. A
little weird, but not going to break anything.