Re: [bug fix] "pg_ctl stop" times out when it should respond quickly

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Feb 17, 2014 at 11:29 AM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
>> The pg_regress part is ugly.  However, pg_regress is doing something
>> unusual when starting postmaster itself, so the ugly coding to stop it
>> seems to match.  If we wanted to avoid the ugliness here, the right fix
>> would be to use pg_ctl to start postmaster as well as to stop it.

> I wonder if this would change the behavior in cases where we hit ^C
> during the regression tests.  Right now I think that kills the
> postmaster as well as pg_regress, but if we used pg_ctl, it might not,
> because pg_regress uses fork()+exec(), but pg_ctl uses system() to
> launch a shell which is in turn instructed to background the
> postmaster.

After re-reading this thread, I am convinced that this patch is failing
to fix the real problem, which is that the postmaster isn't defending
against early arrival of a signal; see
http://www.postgresql.org/message-id/30805.1386110129@sss.pgh.pa.us

Hacking pg_ctl is a band-aid solution, and given these concerns,
not a particularly safe band-aid.

I experimented with simply moving postmaster.c's initialization of signals
further up, and that seems to fix the complained-of problem just fine.
It could be an issue if anything in postmaster initialization is expecting
to run with signals unblocked, but I don't know what that would be.
So I suggest we just do the attached.

            regards, tom lane

diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 7a2c45a..bb771a6 100644
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
*************** PostmasterMain(int argc, char *argv[])
*** 562,567 ****
--- 562,597 ----
      getInstallationPaths(argv[0]);

      /*
+      * Set up signal handlers for the postmaster process.
+      *
+      * CAUTION: when changing this list, check for side-effects on the signal
+      * handling setup of child processes.  See tcop/postgres.c,
+      * bootstrap/bootstrap.c, postmaster/bgwriter.c, postmaster/walwriter.c,
+      * postmaster/autovacuum.c, postmaster/pgarch.c, postmaster/pgstat.c,
+      * postmaster/syslogger.c, postmaster/bgworker.c and
+      * postmaster/checkpointer.c.
+      */
+     pqinitmask();
+     PG_SETMASK(&BlockSig);
+
+     pqsignal(SIGHUP, SIGHUP_handler);    /* reread config file and have
+                                          * children do same */
+     pqsignal(SIGINT, pmdie);    /* send SIGTERM and shut down */
+     pqsignal(SIGQUIT, pmdie);    /* send SIGQUIT and die */
+     pqsignal(SIGTERM, pmdie);    /* wait for children and shut down */
+     pqsignal(SIGALRM, SIG_IGN); /* ignored */
+     pqsignal(SIGPIPE, SIG_IGN); /* ignored */
+     pqsignal(SIGUSR1, sigusr1_handler); /* message from child process */
+     pqsignal(SIGUSR2, dummy_handler);    /* unused, reserve for children */
+     pqsignal(SIGCHLD, reaper);    /* handle child termination */
+     pqsignal(SIGTTIN, SIG_IGN); /* ignored */
+     pqsignal(SIGTTOU, SIG_IGN); /* ignored */
+     /* ignore SIGXFSZ, so that ulimit violations work like disk full */
+ #ifdef SIGXFSZ
+     pqsignal(SIGXFSZ, SIG_IGN); /* ignored */
+ #endif
+
+     /*
       * Options setup
       */
      InitializeGUCOptions();
*************** PostmasterMain(int argc, char *argv[])
*** 1109,1144 ****
      }

      /*
-      * Set up signal handlers for the postmaster process.
-      *
-      * CAUTION: when changing this list, check for side-effects on the signal
-      * handling setup of child processes.  See tcop/postgres.c,
-      * bootstrap/bootstrap.c, postmaster/bgwriter.c, postmaster/walwriter.c,
-      * postmaster/autovacuum.c, postmaster/pgarch.c, postmaster/pgstat.c,
-      * postmaster/syslogger.c, postmaster/bgworker.c and
-      * postmaster/checkpointer.c.
-      */
-     pqinitmask();
-     PG_SETMASK(&BlockSig);
-
-     pqsignal(SIGHUP, SIGHUP_handler);    /* reread config file and have
-                                          * children do same */
-     pqsignal(SIGINT, pmdie);    /* send SIGTERM and shut down */
-     pqsignal(SIGQUIT, pmdie);    /* send SIGQUIT and die */
-     pqsignal(SIGTERM, pmdie);    /* wait for children and shut down */
-     pqsignal(SIGALRM, SIG_IGN); /* ignored */
-     pqsignal(SIGPIPE, SIG_IGN); /* ignored */
-     pqsignal(SIGUSR1, sigusr1_handler); /* message from child process */
-     pqsignal(SIGUSR2, dummy_handler);    /* unused, reserve for children */
-     pqsignal(SIGCHLD, reaper);    /* handle child termination */
-     pqsignal(SIGTTIN, SIG_IGN); /* ignored */
-     pqsignal(SIGTTOU, SIG_IGN); /* ignored */
-     /* ignore SIGXFSZ, so that ulimit violations work like disk full */
- #ifdef SIGXFSZ
-     pqsignal(SIGXFSZ, SIG_IGN); /* ignored */
- #endif
-
-     /*
       * If enabled, start up syslogger collection subprocess
       */
      SysLoggerPID = SysLogger_Start();
--- 1139,1144 ----