On Wed Jul 12, 2023 at 3:56 AM CDT, Peter Eisentraut wrote:
> On 06.07.23 22:43, Tristan Partin wrote:
> > /* Finish incomplete line on stdout */
> > - puts("");
> > - exit(1);
> > + write(STDOUT_FILENO, "", 1);
> > + _exit(1);
>
> puts() writes a newline, so it should probably be something like
>
> write(STDOUT_FILENO, "\n", 1);
Silly mistake. Thanks. v2 attached.
It has come to my attention that STDOUT_FILENO might not be portable and
fileno(3) isn't marked as signal-safe, so I have just used the raw 1 for
stdout, which as far as I know is portable.
--
Tristan Partin
Neon (https://neon.tech)
From 2ec5f78e8c8b54ea8953a943d672d02b0d6f448d Mon Sep 17 00:00:00 2001
From: Tristan Partin <[email protected]>
Date: Thu, 6 Jul 2023 15:17:36 -0500
Subject: [PATCH v2 2/2] Cleanup some signal usage on Windows
SIGTERM can be handled the same as on a UNIX-like system. SIGINT, on the
other hand, requires special considerations. The Windows documentation
says the following:
> SIGINT is not supported for any Win32 application. When a CTRL+C
> interrupt occurs, Win32 operating systems generate a new thread to
> specifically handle that interrupt. This can cause a single-thread
> application, such as one in UNIX, to become multithreaded and cause
> unexpected behavior.
Therefore, if all the SIGINT handler does is read from memory and/or
exit, we can use the same handler. If the signal handler writes to
memory, such as setting a boolean, the original thread would never
become aware of the changed state.
---
src/bin/initdb/initdb.c | 17 ++++-------------
src/bin/pg_basebackup/pg_receivewal.c | 5 +----
src/bin/pg_basebackup/pg_recvlogical.c | 9 ++++-----
src/bin/pg_test_fsync/pg_test_fsync.c | 3 ---
4 files changed, 9 insertions(+), 25 deletions(-)
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 3f4167682a..cac58eae2a 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -2773,26 +2773,17 @@ void
setup_signals(void)
{
/* some of these are not valid on Windows */
-#ifdef SIGHUP
- pqsignal(SIGHUP, trapsig);
-#endif
-#ifdef SIGINT
+ pqsignal(SIGTERM, trapsig);
+
+#ifndef WIN32
pqsignal(SIGINT, trapsig);
-#endif
-#ifdef SIGQUIT
+ pqsignal(SIGHUP, trapsig);
pqsignal(SIGQUIT, trapsig);
-#endif
-#ifdef SIGTERM
- pqsignal(SIGTERM, trapsig);
-#endif
/* Ignore SIGPIPE when writing to backend, so we can clean up */
-#ifdef SIGPIPE
pqsignal(SIGPIPE, SIG_IGN);
-#endif
/* Prevent SIGSYS so we can probe for kernel calls that might not work */
-#ifdef SIGSYS
pqsignal(SIGSYS, SIG_IGN);
#endif
}
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index d0a4079d50..55143c4037 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -611,14 +611,11 @@ StreamLog(void)
* When SIGINT/SIGTERM are caught, just tell the system to exit at the next
* possible moment.
*/
-#ifndef WIN32
-
static void
sigexit_handler(SIGNAL_ARGS)
{
time_to_stop = true;
}
-#endif
int
main(int argc, char **argv)
@@ -838,9 +835,9 @@ main(int argc, char **argv)
* Trap signals. (Don't do this until after the initial password prompt,
* if one is needed, in GetConnection.)
*/
+ pqsignal(SIGTERM, sigexit_handler);
#ifndef WIN32
pqsignal(SIGINT, sigexit_handler);
- pqsignal(SIGTERM, sigexit_handler);
#endif
/*
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index f3c7937a1d..1228dc03db 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -644,10 +644,6 @@ error:
conn = NULL;
}
-/*
- * Unfortunately we can't do sensible signal handling on windows...
- */
-#ifndef WIN32
/*
* When SIGINT/SIGTERM are caught, just tell the system to exit at the next
@@ -659,6 +655,9 @@ sigexit_handler(SIGNAL_ARGS)
time_to_abort = true;
}
+/* SIGHUP is not defined on Windows */
+#ifndef WIN32
+
/*
* Trigger the output file to be reopened.
*/
@@ -921,9 +920,9 @@ main(int argc, char **argv)
* Trap signals. (Don't do this until after the initial password prompt,
* if one is needed, in GetConnection.)
*/
+ pqsignal(SIGTERM, sigexit_handler);
#ifndef WIN32
pqsignal(SIGINT, sigexit_handler);
- pqsignal(SIGTERM, sigexit_handler);
pqsignal(SIGHUP, sighup_handler);
#endif
diff --git a/src/bin/pg_test_fsync/pg_test_fsync.c b/src/bin/pg_test_fsync/pg_test_fsync.c
index 42d0845b06..a3787d6d39 100644
--- a/src/bin/pg_test_fsync/pg_test_fsync.c
+++ b/src/bin/pg_test_fsync/pg_test_fsync.c
@@ -109,9 +109,6 @@ main(int argc, char *argv[])
pqsignal(SIGTERM, signal_cleanup);
#ifndef WIN32
pqsignal(SIGALRM, process_alarm);
-#endif
-#ifdef SIGHUP
- /* Not defined on win32 */
pqsignal(SIGHUP, signal_cleanup);
#endif
--
Tristan Partin
Neon (https://neon.tech)
From 8c912ed353240ecfa49ee3a9f1f7c65141e9d0d6 Mon Sep 17 00:00:00 2001
From: Tristan Partin <[email protected]>
Date: Thu, 6 Jul 2023 15:25:14 -0500
Subject: [PATCH v2 1/2] Use signal-safe functions in signal handler
According to signal-safety(7), exit(3) and puts(3) are not safe to call
in a signal handler.
---
src/bin/pg_test_fsync/pg_test_fsync.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/bin/pg_test_fsync/pg_test_fsync.c b/src/bin/pg_test_fsync/pg_test_fsync.c
index 435df8d808..42d0845b06 100644
--- a/src/bin/pg_test_fsync/pg_test_fsync.c
+++ b/src/bin/pg_test_fsync/pg_test_fsync.c
@@ -595,9 +595,12 @@ signal_cleanup(SIGNAL_ARGS)
/* Delete the file if it exists. Ignore errors */
if (needs_unlink)
unlink(filename);
- /* Finish incomplete line on stdout */
- puts("");
- exit(1);
+ /*
+ * Finish incomplete line on stdout. fileno(3) is not signal-safe, and
+ * STDOUT_FILENO is not portable.
+ */
+ write(1, "\n", 1);
+ _exit(1);
}
#ifdef HAVE_FSYNC_WRITETHROUGH
--
Tristan Partin
Neon (https://neon.tech)
