On Sun, Aug 7, 2022 at 10:42 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > Thomas Munro <thomas.mu...@gmail.com> writes: > > Did I understand correctly that the places that do kill(-pid) followed > > by kill(pid) really only need the kill(-pid)? > > Uh ... did you read the comment right above signal_child? > > * There is a race condition for recently-forked children: they might not > * have executed setsid() yet. So we signal the child directly as well as > * the group. We assume such a child will handle the signal before trying > * to spawn any grandchild processes. We also assume that signaling the > * child twice will not cause any problems.
Oof. Fixed.
From 7ec1aa38f30e32734eb77a6dc6448d332179dfa3 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 v2] 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> Reviewed-by: Tom Lane <t...@sss.pgh.pa.us> Discussion: https://postgr.es/m/CA%2BTgmob_5AUNCzyFGJX6quYSnQnKCHW6DGGJa1noofJqSu%2Bweg%40mail.gmail.com --- configure | 12 ++++++------ configure.ac | 2 +- src/backend/postmaster/postmaster.c | 2 -- 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 | 4 ---- src/bin/pg_ctl/pg_ctl.c | 2 +- src/include/port.h | 1 - src/include/port/win32_port.h | 2 +- src/port/{kill.c => win32kill.c} | 16 +++++++++------- src/tools/msvc/Mkvcbuild.pm | 3 ++- 12 files changed, 23 insertions(+), 38 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..f8bd6285b8 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -4043,7 +4043,6 @@ 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: @@ -4057,7 +4056,6 @@ signal_child(pid_t pid, int signal) default: break; } -#endif } /* diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index 0555b02a8d..950ecc764b 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..5a8d5f9c38 100644 --- a/src/backend/utils/init/postinit.c +++ b/src/backend/utils/init/postinit.c @@ -1279,10 +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,10 +1290,8 @@ StatementTimeoutHandler(void) static void LockTimeoutHandler(void) { -#ifdef HAVE_SETSID /* try to signal whole process group */ kill(-MyProcPid, SIGINT); -#endif kill(MyProcPid, SIGINT); } 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/include/port/win32_port.h b/src/include/port/win32_port.h index 79451a00f9..ea94bb3dac 100644 --- a/src/include/port/win32_port.h +++ b/src/include/port/win32_port.h @@ -470,7 +470,7 @@ extern HANDLE pgwin32_create_signal_listener(pid_t pid); extern void pgwin32_dispatch_queued_signals(void); extern void pg_queue_signal(int signum); -/* In src/port/kill.c */ +/* In src/port/win32kill.c */ #define kill(pid,sig) pgkill(pid,sig) extern int pgkill(int pid, int sig); 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
