Sending SIGABRT to child processes (was Re: Strange failure on mamba) - Mailing list pgsql-hackers

Andres Freund <andres@anarazel.de> writes:
> On 2022-11-17 17:47:50 -0500, Tom Lane wrote:
>> So I'd like to have some way to make the postmaster send SIGABRT instead
>> of SIGKILL in the buildfarm environment.  The lowest-tech way would be
>> to drive that off some #define or other.  We could scale it up to a GUC
>> perhaps.  Adjacent to that, I also wonder whether SIGABRT wouldn't be
>> more useful than SIGSTOP for the existing SendStop half-a-feature ---
>> the idea that people should collect cores manually seems mighty
>> last-century.

> I suspect that having a GUC would be a good idea. I needed something similar
> recently, debugging an occasional hang in the AIO patchset. I first tried
> something like your #define approach and it did cause a problematic flood of
> core files.

Yeah, the main downside of such a thing is the risk of lots of core files
accumulating over repeated crashes.  Nonetheless, I think it'll be a
useful debugging aid.  Here's a proposed patch.  (I took the opportunity
to kill off the long-since-unimplemented Reinit switch, too.)

One thing I'm not too clear on is if we want to send SIGABRT to the child
groups (ie, SIGABRT grandchild processes too).  I made signal_child do
so here, but perhaps it's overkill.

            regards, tom lane

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index bd50ea8e48..24b1624bad 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -11500,6 +11500,62 @@ LOG:  CleanUpLock: deleting: lock(0xb7acd844) id(24688,24696,0,0,0,1)
       </listitem>
      </varlistentry>

+     <varlistentry id="guc-send-abort-for-crash" xreflabel="send_abort_for_crash">
+      <term><varname>send_abort_for_crash</varname> (<type>boolean</type>)
+      <indexterm>
+       <primary><varname>send_abort_for_crash</varname> configuration parameter</primary>
+      </indexterm>
+      </term>
+      <listitem>
+       <para>
+        By default, after a backend crash the postmaster will stop remaining
+        child processes by sending them <systemitem>SIGQUIT</systemitem>
+        signals, which permits them to exit more-or-less gracefully.  When
+        this option is set to <literal>on</literal>,
+        <systemitem>SIGABRT</systemitem> is sent instead.  That normally
+        results in production of a core dump file for each such child
+        process.
+        This can be handy for investigating the states of other processes
+        after a crash.  It can also consume lots of disk space in the event
+        of repeated crashes, so do not enable this on systems you are not
+        monitoring carefully.
+        Beware that no support exists for cleaning up the core file(s)
+        automatically.
+        This parameter can only be set in
+        the <filename>postgresql.conf</filename> file or on the server
+        command line.
+       </para>
+      </listitem>
+     </varlistentry>
+
+     <varlistentry id="guc-send-abort-for-kill" xreflabel="send_abort_for_kill">
+      <term><varname>send_abort_for_kill</varname> (<type>boolean</type>)
+      <indexterm>
+       <primary><varname>send_abort_for_kill</varname> configuration parameter</primary>
+      </indexterm>
+      </term>
+      <listitem>
+       <para>
+        By default, after attempting to stop a child process with
+        <systemitem>SIGQUIT</systemitem>, the postmaster will wait five
+        seconds and then send <systemitem>SIGKILL</systemitem> to force
+        immediate termination.  When this option is set
+        to <literal>on</literal>, <systemitem>SIGABRT</systemitem> is sent
+        instead of <systemitem>SIGKILL</systemitem>.  That normally results
+        in production of a core dump file for each such child process.
+        This can be handy for investigating the states
+        of <quote>stuck</quote> child processes.  It can also consume lots
+        of disk space in the event of repeated crashes, so do not enable
+        this on systems you are not monitoring carefully.
+        Beware that no support exists for cleaning up the core file(s)
+        automatically.
+        This parameter can only be set in
+        the <filename>postgresql.conf</filename> file or on the server
+        command line.
+       </para>
+      </listitem>
+     </varlistentry>
+
     </variablelist>
   </sect1>
   <sect1 id="runtime-config-short">
diff --git a/doc/src/sgml/ref/postgres-ref.sgml b/doc/src/sgml/ref/postgres-ref.sgml
index 55a3f6c69d..b13a16a117 100644
--- a/doc/src/sgml/ref/postgres-ref.sgml
+++ b/doc/src/sgml/ref/postgres-ref.sgml
@@ -409,24 +409,6 @@ PostgreSQL documentation
       </listitem>
      </varlistentry>

-     <varlistentry>
-      <term><option>-n</option></term>
-      <listitem>
-       <para>
-        This option is for debugging problems that cause a server
-        process to die abnormally.  The ordinary strategy in this
-        situation is to notify all other server processes that they
-        must terminate and then reinitialize the shared memory and
-        semaphores.  This is because an errant server process could
-        have corrupted some shared state before terminating.  This
-        option specifies that <command>postgres</command> will
-        not reinitialize shared data structures.  A knowledgeable
-        system programmer can then use a debugger to examine shared
-        memory and semaphore state.
-       </para>
-     </listitem>
-    </varlistentry>
-
      <varlistentry>
       <term><option>-O</option></term>
       <listitem>
@@ -466,14 +448,9 @@ PostgreSQL documentation
         This option is for debugging problems that cause a server
         process to die abnormally.  The ordinary strategy in this
         situation is to notify all other server processes that they
-        must terminate and then reinitialize the shared memory and
-        semaphores.  This is because an errant server process could
-        have corrupted some shared state before terminating.  This
-        option specifies that <command>postgres</command> will
-        stop all other server processes by sending the signal
-        <literal>SIGSTOP</literal>, but will not cause them to
-        terminate.  This permits system programmers to collect core
-        dumps from all server processes by hand.
+        must terminate, by sending them <systemitem>SIGQUIT</systemitem>
+        signals.  With this option, <systemitem>SIGABRT</systemitem>
+        will be sent instead, resulting in production of core dump files.
        </para>
       </listitem>
      </varlistentry>
diff --git a/src/backend/main/main.c b/src/backend/main/main.c
index f5da4260a1..67f556b4dd 100644
--- a/src/backend/main/main.c
+++ b/src/backend/main/main.c
@@ -352,11 +352,10 @@ help(const char *progname)

     printf(_("\nDeveloper options:\n"));
     printf(_("  -f s|i|o|b|t|n|m|h forbid use of some plan types\n"));
-    printf(_("  -n                 do not reinitialize shared memory after abnormal exit\n"));
     printf(_("  -O                 allow system table structure changes\n"));
     printf(_("  -P                 disable system indexes\n"));
     printf(_("  -t pa|pl|ex        show timings after each query\n"));
-    printf(_("  -T                 send SIGSTOP to all backend processes if one dies\n"));
+    printf(_("  -T                 send SIGABRT to all backend processes if one dies\n"));
     printf(_("  -W NUM             wait NUM seconds to allow attach from a debugger\n"));

     printf(_("\nOptions for single-user mode:\n"));
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 1da5752047..c83cc8cc6c 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -219,16 +219,6 @@ int            ReservedBackends;
 #define MAXLISTEN    64
 static pgsocket ListenSocket[MAXLISTEN];

-/*
- * These globals control the behavior of the postmaster in case some
- * backend dumps core.  Normally, it kills all peers of the dead backend
- * and reinitializes shared memory.  By specifying -s or -n, we can have
- * the postmaster stop (rather than kill) peers and not reinitialize
- * shared data structures.  (Reinit is currently dead code, though.)
- */
-static bool Reinit = true;
-static int    SendStop = false;
-
 /* still more option variables */
 bool        EnableSSL = false;

@@ -243,6 +233,8 @@ bool        enable_bonjour = false;
 char       *bonjour_name;
 bool        restart_after_crash = true;
 bool        remove_temp_files_after_crash = true;
+bool        send_abort_for_crash = false;
+bool        send_abort_for_kill = false;

 /* PIDs of special child processes; 0 when not running */
 static pid_t StartupPID = 0,
@@ -414,6 +406,7 @@ static void report_fork_failure_to_client(Port *port, int errnum);
 static CAC_state canAcceptConnections(int backend_type);
 static bool RandomCancelKey(int32 *cancel_key);
 static void signal_child(pid_t pid, int signal);
+static void sigquit_child(pid_t pid);
 static bool SignalSomeChildren(int signal, int target);
 static void TerminateChildren(int signal);

@@ -697,7 +690,7 @@ PostmasterMain(int argc, char *argv[])
      * tcop/postgres.c (the option sets should not conflict) and with the
      * common help() function in main/main.c.
      */
-    while ((opt = getopt(argc, argv, "B:bc:C:D:d:EeFf:h:ijk:lN:nOPp:r:S:sTt:W:-:")) != -1)
+    while ((opt = getopt(argc, argv, "B:bc:C:D:d:EeFf:h:ijk:lN:OPp:r:S:sTt:W:-:")) != -1)
     {
         switch (opt)
         {
@@ -767,11 +760,6 @@ PostmasterMain(int argc, char *argv[])
                 SetConfigOption("max_connections", optarg, PGC_POSTMASTER, PGC_S_ARGV);
                 break;

-            case 'n':
-                /* Don't reinit shared mem after abnormal exit */
-                Reinit = false;
-                break;
-
             case 'O':
                 SetConfigOption("allow_system_table_mods", "true", PGC_POSTMASTER, PGC_S_ARGV);
                 break;
@@ -799,11 +787,10 @@ PostmasterMain(int argc, char *argv[])
             case 'T':

                 /*
-                 * In the event that some backend dumps core, send SIGSTOP,
-                 * rather than SIGQUIT, to all its peers.  This lets the wily
-                 * post_hacker collect core dumps from everyone.
+                 * This option used to be defined as sending SIGSTOP after a
+                 * backend crash, but sending SIGABRT seems more useful.
                  */
-                SendStop = true;
+                SetConfigOption("send_abort_for_crash", "true", PGC_POSTMASTER, PGC_S_ARGV);
                 break;

             case 't':
@@ -1896,20 +1883,23 @@ ServerLoop(void)

         /*
          * If we already sent SIGQUIT to children and they are slow to shut
-         * down, it's time to send them SIGKILL.  This doesn't happen
-         * normally, but under certain conditions backends can get stuck while
-         * shutting down.  This is a last measure to get them unwedged.
+         * down, it's time to send them SIGKILL (or SIGABRT if requested).
+         * This doesn't happen normally, but under certain conditions backends
+         * can get stuck while shutting down.  This is a last measure to get
+         * them unwedged.
          *
          * Note we also do this during recovery from a process crash.
          */
-        if ((Shutdown >= ImmediateShutdown || (FatalError && !SendStop)) &&
+        if ((Shutdown >= ImmediateShutdown || FatalError) &&
             AbortStartTime != 0 &&
             (now - AbortStartTime) >= SIGKILL_CHILDREN_AFTER_SECS)
         {
             /* We were gentle with them before. Not anymore */
             ereport(LOG,
-                    (errmsg("issuing SIGKILL to recalcitrant children")));
-            TerminateChildren(SIGKILL);
+            /* translator: %s is SIGKILL or SIGABRT */
+                    (errmsg("issuing %s to recalcitrant children",
+                            send_abort_for_kill ? "SIGABRT" : "SIGKILL")));
+            TerminateChildren(send_abort_for_kill ? SIGABRT : SIGKILL);
             /* reset flag so we don't SIGKILL again */
             AbortStartTime = 0;
         }
@@ -2903,6 +2893,7 @@ pmdie(SIGNAL_ARGS)
 #endif

             /* tell children to shut down ASAP */
+            /* (note we don't apply send_abort_for_crash here) */
             SetQuitSignalReason(PMQUIT_FOR_STOP);
             TerminateChildren(SIGQUIT);
             pmState = PM_WAIT_BACKENDS;
@@ -3470,20 +3461,9 @@ HandleChildCrash(int pid, int exitstatus, const char *procname)
             /*
              * This worker is still alive.  Unless we did so already, tell it
              * to commit hara-kiri.
-             *
-             * SIGQUIT is the special signal that says exit without proc_exit
-             * and let the user know what's going on. But if SendStop is set
-             * (-T on command line), then we send SIGSTOP instead, so that we
-             * can get core dumps from all backends by hand.
              */
             if (take_action)
-            {
-                ereport(DEBUG2,
-                        (errmsg_internal("sending %s to process %d",
-                                         (SendStop ? "SIGSTOP" : "SIGQUIT"),
-                                         (int) rw->rw_pid)));
-                signal_child(rw->rw_pid, (SendStop ? SIGSTOP : SIGQUIT));
-            }
+                sigquit_child(rw->rw_pid);
         }
     }

@@ -3514,13 +3494,8 @@ HandleChildCrash(int pid, int exitstatus, const char *procname)
              * This backend is still alive.  Unless we did so already, tell it
              * to commit hara-kiri.
              *
-             * SIGQUIT is the special signal that says exit without proc_exit
-             * and let the user know what's going on. But if SendStop is set
-             * (-T on command line), then we send SIGSTOP instead, so that we
-             * can get core dumps from all backends by hand.
-             *
-             * We could exclude dead_end children here, but at least in the
-             * SIGSTOP case it seems better to include them.
+             * We could exclude dead_end children here, but at least when
+             * sending SIGABRT it seems better to include them.
              *
              * Background workers were already processed above; ignore them
              * here.
@@ -3529,13 +3504,7 @@ HandleChildCrash(int pid, int exitstatus, const char *procname)
                 continue;

             if (take_action)
-            {
-                ereport(DEBUG2,
-                        (errmsg_internal("sending %s to process %d",
-                                         (SendStop ? "SIGSTOP" : "SIGQUIT"),
-                                         (int) bp->pid)));
-                signal_child(bp->pid, (SendStop ? SIGSTOP : SIGQUIT));
-            }
+                sigquit_child(bp->pid);
         }
     }

@@ -3547,11 +3516,7 @@ HandleChildCrash(int pid, int exitstatus, const char *procname)
     }
     else if (StartupPID != 0 && take_action)
     {
-        ereport(DEBUG2,
-                (errmsg_internal("sending %s to process %d",
-                                 (SendStop ? "SIGSTOP" : "SIGQUIT"),
-                                 (int) StartupPID)));
-        signal_child(StartupPID, (SendStop ? SIGSTOP : SIGQUIT));
+        sigquit_child(StartupPID);
         StartupStatus = STARTUP_SIGNALED;
     }

@@ -3559,73 +3524,37 @@ HandleChildCrash(int pid, int exitstatus, const char *procname)
     if (pid == BgWriterPID)
         BgWriterPID = 0;
     else if (BgWriterPID != 0 && take_action)
-    {
-        ereport(DEBUG2,
-                (errmsg_internal("sending %s to process %d",
-                                 (SendStop ? "SIGSTOP" : "SIGQUIT"),
-                                 (int) BgWriterPID)));
-        signal_child(BgWriterPID, (SendStop ? SIGSTOP : SIGQUIT));
-    }
+        sigquit_child(BgWriterPID);

     /* Take care of the checkpointer too */
     if (pid == CheckpointerPID)
         CheckpointerPID = 0;
     else if (CheckpointerPID != 0 && take_action)
-    {
-        ereport(DEBUG2,
-                (errmsg_internal("sending %s to process %d",
-                                 (SendStop ? "SIGSTOP" : "SIGQUIT"),
-                                 (int) CheckpointerPID)));
-        signal_child(CheckpointerPID, (SendStop ? SIGSTOP : SIGQUIT));
-    }
+        sigquit_child(CheckpointerPID);

     /* Take care of the walwriter too */
     if (pid == WalWriterPID)
         WalWriterPID = 0;
     else if (WalWriterPID != 0 && take_action)
-    {
-        ereport(DEBUG2,
-                (errmsg_internal("sending %s to process %d",
-                                 (SendStop ? "SIGSTOP" : "SIGQUIT"),
-                                 (int) WalWriterPID)));
-        signal_child(WalWriterPID, (SendStop ? SIGSTOP : SIGQUIT));
-    }
+        sigquit_child(WalWriterPID);

     /* Take care of the walreceiver too */
     if (pid == WalReceiverPID)
         WalReceiverPID = 0;
     else if (WalReceiverPID != 0 && take_action)
-    {
-        ereport(DEBUG2,
-                (errmsg_internal("sending %s to process %d",
-                                 (SendStop ? "SIGSTOP" : "SIGQUIT"),
-                                 (int) WalReceiverPID)));
-        signal_child(WalReceiverPID, (SendStop ? SIGSTOP : SIGQUIT));
-    }
+        sigquit_child(WalReceiverPID);

     /* Take care of the autovacuum launcher too */
     if (pid == AutoVacPID)
         AutoVacPID = 0;
     else if (AutoVacPID != 0 && take_action)
-    {
-        ereport(DEBUG2,
-                (errmsg_internal("sending %s to process %d",
-                                 (SendStop ? "SIGSTOP" : "SIGQUIT"),
-                                 (int) AutoVacPID)));
-        signal_child(AutoVacPID, (SendStop ? SIGSTOP : SIGQUIT));
-    }
+        sigquit_child(AutoVacPID);

     /* Take care of the archiver too */
     if (pid == PgArchPID)
         PgArchPID = 0;
     else if (PgArchPID != 0 && take_action)
-    {
-        ereport(DEBUG2,
-                (errmsg_internal("sending %s to process %d",
-                                 (SendStop ? "SIGSTOP" : "SIGQUIT"),
-                                 (int) PgArchPID)));
-        signal_child(PgArchPID, (SendStop ? SIGSTOP : SIGQUIT));
-    }
+        sigquit_child(PgArchPID);

     /* We do NOT restart the syslogger */

@@ -3834,6 +3763,10 @@ PostmasterStateMachine(void)
                      * Any required cleanup will happen at next restart. We
                      * set FatalError so that an "abnormal shutdown" message
                      * gets logged when we exit.
+                     *
+                     * We don't consult send_abort_for_crash here, as it's
+                     * unlikely that dumping cores would illuminate the reason
+                     * for checkpointer fork failure.
                      */
                     FatalError = true;
                     pmState = PM_WAIT_DEAD_END;
@@ -4003,8 +3936,8 @@ signal_child(pid_t pid, int signal)
         case SIGINT:
         case SIGTERM:
         case SIGQUIT:
-        case SIGSTOP:
         case SIGKILL:
+        case SIGABRT:
             if (kill(-pid, signal) < 0)
                 elog(DEBUG3, "kill(%ld,%d) failed: %m", (long) (-pid), signal);
             break;
@@ -4014,6 +3947,24 @@ signal_child(pid_t pid, int signal)
 #endif
 }

+/*
+ * Convenience function for killing a child process after a crash of some
+ * other child process.  We log the action at a higher level than we would
+ * otherwise do, and we apply send_abort_for_crash to decide which signal
+ * to send.  Normally it's SIGQUIT -- and most other comments in this file
+ * are written on the assumption that it is -- but developers might prefer
+ * to use SIGABRT to collect per-child core dumps.
+ */
+static void
+sigquit_child(pid_t pid)
+{
+    ereport(DEBUG2,
+            (errmsg_internal("sending %s to process %d",
+                             (send_abort_for_crash ? "SIGABRT" : "SIGQUIT"),
+                             (int) pid)));
+    signal_child(pid, (send_abort_for_crash ? SIGABRT : SIGQUIT));
+}
+
 /*
  * Send a signal to the targeted children (but NOT special children;
  * dead_end children are never signaled, either).
@@ -4069,7 +4020,7 @@ TerminateChildren(int signal)
     if (StartupPID != 0)
     {
         signal_child(StartupPID, signal);
-        if (signal == SIGQUIT || signal == SIGKILL)
+        if (signal == SIGQUIT || signal == SIGKILL || signal == SIGABRT)
             StartupStatus = STARTUP_SIGNALED;
     }
     if (BgWriterPID != 0)
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 836b49484a..349dd6a537 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -1227,6 +1227,26 @@ struct config_bool ConfigureNamesBool[] =
         true,
         NULL, NULL, NULL
     },
+    {
+        {"send_abort_for_crash", PGC_SIGHUP, DEVELOPER_OPTIONS,
+            gettext_noop("Send SIGABRT not SIGQUIT to child processes after backend crash."),
+            NULL,
+            GUC_NOT_IN_SAMPLE
+        },
+        &send_abort_for_crash,
+        false,
+        NULL, NULL, NULL
+    },
+    {
+        {"send_abort_for_kill", PGC_SIGHUP, DEVELOPER_OPTIONS,
+            gettext_noop("Send SIGABRT not SIGKILL to stuck child processes."),
+            NULL,
+            GUC_NOT_IN_SAMPLE
+        },
+        &send_abort_for_kill,
+        false,
+        NULL, NULL, NULL
+    },

     {
         {"log_duration", PGC_SUSET, LOGGING_WHAT,
diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h
index 90e333ccd2..f078321df2 100644
--- a/src/include/postmaster/postmaster.h
+++ b/src/include/postmaster/postmaster.h
@@ -30,6 +30,8 @@ extern PGDLLIMPORT bool enable_bonjour;
 extern PGDLLIMPORT char *bonjour_name;
 extern PGDLLIMPORT bool restart_after_crash;
 extern PGDLLIMPORT bool remove_temp_files_after_crash;
+extern PGDLLIMPORT bool send_abort_for_crash;
+extern PGDLLIMPORT bool send_abort_for_kill;

 #ifdef WIN32
 extern PGDLLIMPORT HANDLE PostmasterHandle;