On Sat, Aug 6, 2022 at 2:54 AM Robert Haas <robertmh...@gmail.com> wrote: > On Fri, Aug 5, 2022 at 10:48 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > > Hmm ... I agree with you that the end result could be nicer code, > > but what's making it nicer is a pretty substantial amount of human > > effort for each and every call site. Is anybody stepping forward > > to put in that amount of work? > > > > My proposal is to leave the call sites alone until someone feels > > like doing that sort of detail work. > > My plan was to nerd-snipe Thomas Munro into doing it.[1]
Alright, well here's a patch for the setsid() stuff following Robert's plan, which I think is a pretty good plan. Did I understand correctly that the places that do kill(-pid) followed by kill(pid) really only need the kill(-pid)? I checked that user processes should never have pid 0 (that's a special system idle process) or 1 (because they're always even, actually it looks like they're pointers in kernel space or something like that), since those wouldn't play nice with the coding I used here. I note that Windows actually *does* have process groups (in one of the CI threads, we learned that there were at least two concepts like that). Some of our fake signals turn into messages to pipes, and others turn into process termination, and in theory we could probably also take advantage of Windows' support for its native "control C" and "control break" signals here. It's possible that someone could do something to make all but the pipe ones propagate to real Windows process groups. That might be good for things like nuking archiver subprocesses and the like when taking out the backend, or something like that, but I'm not planning to look into that myself.
From f3c224082736605f373b4c34e661b6a8d26846c0 Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.mu...@gmail.com> Date: Sun, 7 Aug 2022 09:37:34 +1200 Subject: [PATCH] Simplify conditional code for process groups. Teach our replacement kill() function for Windows to ignore process groups and send directly to a single pid instead. Now we can drop a bunch of conditional code at call sites, and the vestigial HAVE_SETSID macro. While here, rename src/port/kill.c to win32kill.c, following our convention for Windows-only fallback code. Suggested-by: Robert Haas <robertmh...@gmail.com> Discussion: https://postgr.es/m/CA%2BTgmob_5AUNCzyFGJX6quYSnQnKCHW6DGGJa1noofJqSu%2Bweg%40mail.gmail.com --- configure | 12 ++++++------ configure.ac | 2 +- src/backend/postmaster/postmaster.c | 9 +++------ src/backend/storage/ipc/procarray.c | 9 +-------- src/backend/storage/ipc/signalfuncs.c | 6 +----- src/backend/utils/init/miscinit.c | 2 +- src/backend/utils/init/postinit.c | 6 ------ src/bin/pg_ctl/pg_ctl.c | 2 +- src/include/port.h | 1 - src/port/{kill.c => win32kill.c} | 16 +++++++++------- src/tools/msvc/Mkvcbuild.pm | 3 ++- 11 files changed, 25 insertions(+), 43 deletions(-) rename src/port/{kill.c => win32kill.c} (88%) diff --git a/configure b/configure index 0e73edb9ff..6ea8051c9c 100755 --- a/configure +++ b/configure @@ -16888,12 +16888,6 @@ esac ;; esac - case " $LIBOBJS " in - *" kill.$ac_objext "* ) ;; - *) LIBOBJS="$LIBOBJS kill.$ac_objext" - ;; -esac - case " $LIBOBJS " in *" open.$ac_objext "* ) ;; *) LIBOBJS="$LIBOBJS open.$ac_objext" @@ -16930,6 +16924,12 @@ esac ;; esac + case " $LIBOBJS " in + *" win32kill.$ac_objext "* ) ;; + *) LIBOBJS="$LIBOBJS win32kill.$ac_objext" + ;; +esac + case " $LIBOBJS " in *" win32link.$ac_objext "* ) ;; *) LIBOBJS="$LIBOBJS win32link.$ac_objext" diff --git a/configure.ac b/configure.ac index efd3be91cd..127e4ce925 100644 --- a/configure.ac +++ b/configure.ac @@ -1926,13 +1926,13 @@ if test "$PORTNAME" = "win32"; then AC_CHECK_FUNCS(_configthreadlocale) AC_LIBOBJ(dirmod) AC_LIBOBJ(getrusage) - AC_LIBOBJ(kill) AC_LIBOBJ(open) AC_LIBOBJ(system) AC_LIBOBJ(win32dlopen) AC_LIBOBJ(win32env) AC_LIBOBJ(win32error) AC_LIBOBJ(win32fdatasync) + AC_LIBOBJ(win32kill) AC_LIBOBJ(win32link) AC_LIBOBJ(win32ntdll) AC_LIBOBJ(win32pread) diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 81cb585891..c50e07bf2b 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -4041,9 +4041,6 @@ PostmasterStateMachine(void) static void signal_child(pid_t pid, int signal) { - if (kill(pid, signal) < 0) - elog(DEBUG3, "kill(%ld,%d) failed: %m", (long) pid, signal); -#ifdef HAVE_SETSID switch (signal) { case SIGINT: @@ -4051,13 +4048,13 @@ signal_child(pid_t pid, int signal) case SIGQUIT: case SIGSTOP: case SIGKILL: - if (kill(-pid, signal) < 0) - elog(DEBUG3, "kill(%ld,%d) failed: %m", (long) (-pid), signal); + pid = -pid; break; default: break; } -#endif + if (kill(pid, signal) < 0) + elog(DEBUG3, "kill(%ld,%d) failed: %m", (long) pid, signal); } /* diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index 0555b02a8d..1ec4359c92 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/src/backend/storage/ipc/procarray.c @@ -3865,15 +3865,8 @@ TerminateOtherDBBackends(Oid databaseId) if (proc != NULL) { - /* - * If we have setsid(), signal the backend's whole process - * group - */ -#ifdef HAVE_SETSID + /* Signal the backend's whole process group */ (void) kill(-pid, SIGTERM); -#else - (void) kill(pid, SIGTERM); -#endif } } } diff --git a/src/backend/storage/ipc/signalfuncs.c b/src/backend/storage/ipc/signalfuncs.c index 6e310b14eb..13e691ec92 100644 --- a/src/backend/storage/ipc/signalfuncs.c +++ b/src/backend/storage/ipc/signalfuncs.c @@ -92,12 +92,8 @@ pg_signal_backend(int pid, int sig) * too unlikely to worry about. */ - /* If we have setsid(), signal the backend's whole process group */ -#ifdef HAVE_SETSID + /* Signal the backend's whole process group */ if (kill(-pid, sig)) -#else - if (kill(pid, sig)) -#endif { /* Again, just a warning to allow loops */ ereport(WARNING, diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c index bd973ba613..c65b596d1b 100644 --- a/src/backend/utils/init/miscinit.c +++ b/src/backend/utils/init/miscinit.c @@ -145,7 +145,7 @@ InitPostmasterChild(void) * children, but for consistency we make all postmaster child processes do * this. */ -#ifdef HAVE_SETSID +#ifndef WIN32 if (setsid() < 0) elog(FATAL, "setsid() failed: %m"); #endif diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c index 0d557a8684..da248b3665 100644 --- a/src/backend/utils/init/postinit.c +++ b/src/backend/utils/init/postinit.c @@ -1279,11 +1279,8 @@ StatementTimeoutHandler(void) if (ClientAuthInProgress) sig = SIGTERM; -#ifdef HAVE_SETSID /* try to signal whole process group */ kill(-MyProcPid, sig); -#endif - kill(MyProcPid, sig); } /* @@ -1292,11 +1289,8 @@ StatementTimeoutHandler(void) static void LockTimeoutHandler(void) { -#ifdef HAVE_SETSID /* try to signal whole process group */ kill(-MyProcPid, SIGINT); -#endif - kill(MyProcPid, SIGINT); } static void diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c index 2762e8590d..17c1c16b4f 100644 --- a/src/bin/pg_ctl/pg_ctl.c +++ b/src/bin/pg_ctl/pg_ctl.c @@ -478,7 +478,7 @@ start_postmaster(void) * group and make it a group leader, so that it doesn't get signaled along * with the current group that launched it. */ -#ifdef HAVE_SETSID +#ifndef WIN32 if (setsid() < 0) { write_stderr(_("%s: could not start server due to setsid() failure: %s\n"), diff --git a/src/include/port.h b/src/include/port.h index ad76384fb1..aa9bcb7966 100644 --- a/src/include/port.h +++ b/src/include/port.h @@ -502,7 +502,6 @@ extern bool wait_result_is_any_signal(int exit_status, bool include_command_not_ #define HAVE_POLL 1 #define HAVE_POLL_H 1 #define HAVE_READLINK 1 -#define HAVE_SETSID 1 #define HAVE_SHM_OPEN 1 #define HAVE_SYMLINK 1 #endif diff --git a/src/port/kill.c b/src/port/win32kill.c similarity index 88% rename from src/port/kill.c rename to src/port/win32kill.c index ff0862683c..794468405a 100644 --- a/src/port/kill.c +++ b/src/port/win32kill.c @@ -1,6 +1,6 @@ /*------------------------------------------------------------------------- * - * kill.c + * win32kill.c * kill() * * Copyright (c) 1996-2022, PostgreSQL Global Development Group @@ -9,14 +9,13 @@ * signals that the backend can recognize. * * IDENTIFICATION - * src/port/kill.c + * src/port/win32kill.c * *------------------------------------------------------------------------- */ #include "c.h" -#ifdef WIN32 /* signal sending */ int pgkill(int pid, int sig) @@ -32,12 +31,17 @@ pgkill(int pid, int sig) errno = EINVAL; return -1; } - if (pid <= 0) + if (pid == 0 || pid == -1) { - /* No support for process groups */ + /* No support for special process group values */ errno = EINVAL; return -1; } + else if (pid < -1) + { + /* No support for process groups: just send to one process instead */ + pid = -pid; + } /* special case for SIGKILL: just ask the system to terminate the target */ if (sig == SIGKILL) @@ -93,5 +97,3 @@ pgkill(int pid, int sig) return -1; } } - -#endif diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm index bacc920758..016456ca37 100644 --- a/src/tools/msvc/Mkvcbuild.pm +++ b/src/tools/msvc/Mkvcbuild.pm @@ -101,7 +101,7 @@ sub mkvcbuild our @pgportfiles = qw( chklocale.c explicit_bzero.c getpeereid.c getrusage.c inet_aton.c - getaddrinfo.c inet_net_ntop.c kill.c open.c + getaddrinfo.c inet_net_ntop.c open.c snprintf.c strlcat.c strlcpy.c dirmod.c noblock.c path.c dirent.c getopt.c getopt_long.c preadv.c pwritev.c pg_bitutils.c @@ -112,6 +112,7 @@ sub mkvcbuild win32env.c win32error.c win32fdatasync.c win32gettimeofday.c + win32kill.c win32link.c win32pread.c win32pwrite.c -- 2.37.1
