Re: Brittleness in regression test setup

Peter Eisentraut wrote:
> ============== removing existing temp installation    ==============
> ============== creating temporary installation        ==============
> ============== initializing database system           ==============
> ============== starting postmaster                    ==============
> running on port 65432 with pid 94178
> ============== creating database "regression"         ==============
> ERROR:  database "regression" already exists
>
> It evidently failed to realize that there is another postmaster already
> running at that port and just ran its test setup routines against that
> other instance.

On this matter, I noticed that pg_regress doesn't do anything to clean
up its child processes.  I see zombies lying around on Linux and Mac OS
when the postmaster dies.  (And the zombie is exactly the pid 94178 it
announced in the example above.)

I played around a little with signal handling to collect the dying
postmaster and report and error; see attached rough patch.  I'm not
exactly understanding how this works though.  I would expect lots of
psql zombies for example, because those go through the same
spawn_process() call, but I'm not seeing any.  Also, the sleep() call in
my patch is necessary to get some effect.  Anyone else go a clue on what
to do here?

Index: src/test/regress/GNUmakefile
===================================================================
RCS file: /cvsroot/pgsql/src/test/regress/GNUmakefile,v
retrieving revision 1.75
diff -u -3 -p -r1.75 GNUmakefile
--- src/test/regress/GNUmakefile    1 Oct 2008 22:38:57 -0000    1.75
+++ src/test/regress/GNUmakefile    25 Nov 2008 13:44:05 -0000
@@ -47,6 +47,8 @@ EXTRADEFS = '-DHOST_TUPLE="$(host_tuple)
     '-DSHELLPROG="$(SHELL)"' \
     '-DDLSUFFIX="$(DLSUFFIX)"'

+override CPPFLAGS := -I$(libpq_srcdir) $(CPPFLAGS)
+
 ##
 ## Prepare for tests
 ##
@@ -55,7 +57,7 @@ EXTRADEFS = '-DHOST_TUPLE="$(host_tuple)

 all: submake-libpgport pg_regress$(X)

-pg_regress$(X): pg_regress.o pg_regress_main.o
+pg_regress$(X): pg_regress.o pg_regress_main.o pqsignal.o
     $(CC) $(CFLAGS) $^ $(LDFLAGS) $(LIBS) -o $@

 # dependencies ensure that path changes propagate
@@ -65,6 +67,10 @@ pg_regress.o: pg_regress.c $(top_builddi
 $(top_builddir)/src/port/pg_config_paths.h: $(top_builddir)/src/Makefile.global
     $(MAKE) -C $(top_builddir)/src/port pg_config_paths.h

+pqsignal.c: % : $(top_srcdir)/src/interfaces/libpq/%
+    rm -f $@ && $(LN_S) $< .
+
+
 install: all installdirs
     $(INSTALL_PROGRAM) pg_regress$(X) '$(DESTDIR)$(pgxsdir)/$(subdir)/pg_regress$(X)'

@@ -172,7 +178,7 @@ bigcheck: all

 clean distclean maintainer-clean: clean-lib
 # things built by `all' target
-    rm -f $(OBJS) refint$(DLSUFFIX) autoinc$(DLSUFFIX) pg_regress_main.o pg_regress.o pg_regress$(X)
+    rm -f $(OBJS) refint$(DLSUFFIX) autoinc$(DLSUFFIX) pg_regress_main.o pg_regress.o pg_regress$(X) pqsignal.c
 # things created by various check targets
     rm -f $(output_files) $(input_files)
     rm -rf testtablespace
Index: src/test/regress/pg_regress.c
===================================================================
RCS file: /cvsroot/pgsql/src/test/regress/pg_regress.c,v
retrieving revision 1.51
diff -u -3 -p -r1.51 pg_regress.c
--- src/test/regress/pg_regress.c    25 Nov 2008 11:49:35 -0000    1.51
+++ src/test/regress/pg_regress.c    25 Nov 2008 13:44:05 -0000
@@ -31,6 +31,7 @@

 #include "getopt_long.h"
 #include "pg_config_paths.h"
+#include "libpq/pqsignal.h"

 /* for resultmap we need a list of pairs of strings */
 typedef struct _resultmap
@@ -277,6 +278,8 @@ stop_postmaster(void)
         fflush(stdout);
         fflush(stderr);

+        postmaster_pid = INVALID_PID;
+
         snprintf(buf, sizeof(buf),
                  SYSTEMQUOTE "\"%s/pg_ctl\" stop -D \"%s/data\" -s -m fast" SYSTEMQUOTE,
                  bindir, temp_install);
@@ -1803,6 +1806,29 @@ make_absolute_path(const char *in)
 }

 static void
+sigchld_handler(int signum)
+{
+    pid_t pid;
+    int status;
+    int save_errno;
+
+    save_errno = errno;
+    while (1)
+    {
+        pid = waitpid(postmaster_pid, &status, WNOHANG);
+        if (pid <= 0)
+            break;
+
+        if (pid == postmaster_pid)
+        {
+            fprintf(stderr, "postmaster died\n");
+            exit(2);
+        }
+    }
+    errno = save_errno;
+}
+
+static void
 help(void)
 {
     printf(_("PostgreSQL regression test driver\n"));
@@ -2100,6 +2126,7 @@ regression_main(int argc, char *argv[],
                  debug ? " -d 5" : "",
                  hostname ? hostname : "",
                  outputdir);
+        pqsignal(SIGCHLD, sigchld_handler);
         postmaster_pid = spawn_process(buf);
         if (postmaster_pid == INVALID_PID)
         {
@@ -2116,6 +2143,7 @@ regression_main(int argc, char *argv[],
         snprintf(buf, sizeof(buf),
                  SYSTEMQUOTE "\"%s/psql\" -X postgres <%s 2>%s" SYSTEMQUOTE,
                  bindir, DEVNULL, DEVNULL);
+        sleep(5);
         for (i = 0; i < 60; i++)
         {
             /* Done if psql succeeds */