Re: Idea for improving buildfarm robustness

I wrote:
> It strikes me that a different approach that might be of value would
> be to re-read postmaster.pid and make sure that (a) it's still there
> and (b) it still contains the current postmaster's PID.  This would
> be morally equivalent to what Jim suggests above, and it would dodge
> the checkpointer-destroys-the-evidence problem, and it would have the
> additional advantage that we'd notice when a brain-dead DBA decides
> to manually remove postmaster.pid so he can start a new postmaster.
> (It's probably far too late to avoid data corruption at that point,
> but better late than never.)

> This is still not bulletproof against all overwrite-with-a-backup
> scenarios, but it seems like a noticeable improvement over what we
> discussed yesterday.

Here's a rewritten patch that looks at postmaster.pid instead of
pg_control.  It should be effectively the same as the prior patch in terms
of response to directory-removal cases, and it should also catch many
overwrite cases.

            regards, tom lane

diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index baa43b2..498ebfa 100644
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
*************** ServerLoop(void)
*** 1602,1610 ****
      fd_set        readmask;
      int            nSockets;
      time_t        now,
                  last_touch_time;

!     last_touch_time = time(NULL);

      nSockets = initMasks(&readmask);

--- 1602,1611 ----
      fd_set        readmask;
      int            nSockets;
      time_t        now,
+                 last_lockfile_recheck_time,
                  last_touch_time;

!     last_lockfile_recheck_time = last_touch_time = time(NULL);

      nSockets = initMasks(&readmask);

*************** ServerLoop(void)
*** 1754,1772 ****
          if (StartWorkerNeeded || HaveCrashedWorker)
              maybe_start_bgworker();

-         /*
-          * Touch Unix socket and lock files every 58 minutes, to ensure that
-          * they are not removed by overzealous /tmp-cleaning tasks.  We assume
-          * no one runs cleaners with cutoff times of less than an hour ...
-          */
-         now = time(NULL);
-         if (now - last_touch_time >= 58 * SECS_PER_MINUTE)
-         {
-             TouchSocketFiles();
-             TouchSocketLockFiles();
-             last_touch_time = now;
-         }
-
  #ifdef HAVE_PTHREAD_IS_THREADED_NP

          /*
--- 1755,1760 ----
*************** ServerLoop(void)
*** 1793,1798 ****
--- 1781,1828 ----
              /* reset flag so we don't SIGKILL again */
              AbortStartTime = 0;
          }
+
+         /*
+          * Lastly, check to see if it's time to do some things that we don't
+          * want to do every single time through the loop, because they're a
+          * bit expensive.  Note that there's up to a minute of slop in when
+          * these tasks will be performed, since DetermineSleepTime() will let
+          * us sleep at most that long.
+          */
+         now = time(NULL);
+
+         /*
+          * Once a minute, verify that postmaster.pid hasn't been removed or
+          * overwritten.  If it has, we commit hara-kiri.  This avoids having
+          * postmasters and child processes hanging around after their database
+          * is gone, and maybe causing problems if a new database cluster is
+          * created in the same place.  It also provides some protection
+          * against a DBA foolishly removing postmaster.pid and manually
+          * starting a new postmaster.  Data corruption is likely to ensue from
+          * that anyway, but we can minimize the damage by aborting ASAP.
+          */
+         if (now - last_lockfile_recheck_time >= 1 * SECS_PER_MINUTE)
+         {
+             if (!RecheckDataDirLockFile())
+             {
+                 ereport(LOG,
+                         (errmsg("performing immediate shutdown because data directory lock file is invalid")));
+                 kill(MyProcPid, SIGQUIT);
+             }
+             last_lockfile_recheck_time = now;
+         }
+
+         /*
+          * Touch Unix socket and lock files every 58 minutes, to ensure that
+          * they are not removed by overzealous /tmp-cleaning tasks.  We assume
+          * no one runs cleaners with cutoff times of less than an hour ...
+          */
+         if (now - last_touch_time >= 58 * SECS_PER_MINUTE)
+         {
+             TouchSocketFiles();
+             TouchSocketLockFiles();
+             last_touch_time = now;
+         }
      }
  }

diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index f0099d3..b6270e1 100644
*** a/src/backend/utils/init/miscinit.c
--- b/src/backend/utils/init/miscinit.c
*************** AddToDataDirLockFile(int target_line, co
*** 1202,1207 ****
--- 1202,1277 ----
  }


+ /*
+  * Recheck that the data directory lock file still exists with expected
+  * content.  Return TRUE if the lock file appears OK, FALSE if it isn't.
+  *
+  * We call this periodically in the postmaster.  The idea is that if the
+  * lock file has been removed or replaced by another postmaster, we should
+  * do a panic database shutdown.  Therefore, we should return TRUE if there
+  * is any doubt: we do not want to cause a panic shutdown unnecessarily.
+  * Transient failures like EINTR or ENFILE should not cause us to fail.
+  * (If there really is something wrong, we'll detect it on a future recheck.)
+  */
+ bool
+ RecheckDataDirLockFile(void)
+ {
+     int            fd;
+     int            len;
+     long        file_pid;
+     char        buffer[BLCKSZ];
+
+     fd = open(DIRECTORY_LOCK_FILE, O_RDWR | PG_BINARY, 0);
+     if (fd < 0)
+     {
+         /*
+          * There are many foreseeable false-positive error conditions.  For
+          * safety, fail only on enumerated clearly-something-is-wrong
+          * conditions.
+          */
+         switch (errno)
+         {
+             case ENOENT:
+             case ENOTDIR:
+                 /* disaster */
+                 ereport(LOG,
+                         (errcode_for_file_access(),
+                          errmsg("could not open file \"%s\": %m",
+                                 DIRECTORY_LOCK_FILE)));
+                 return false;
+             default:
+                 /* non-fatal, at least for now */
+                 ereport(LOG,
+                         (errcode_for_file_access(),
+                   errmsg("could not open file \"%s\": %m; continuing anyway",
+                          DIRECTORY_LOCK_FILE)));
+                 return true;
+         }
+     }
+     len = read(fd, buffer, sizeof(buffer) - 1);
+     if (len < 0)
+     {
+         ereport(LOG,
+                 (errcode_for_file_access(),
+                  errmsg("could not read from file \"%s\": %m",
+                         DIRECTORY_LOCK_FILE)));
+         close(fd);
+         return true;            /* treat read failure as nonfatal */
+     }
+     buffer[len] = '\0';
+     close(fd);
+     file_pid = atol(buffer);
+     if (file_pid == getpid())
+         return true;            /* all is well */
+
+     /* Trouble: someone's overwritten the lock file */
+     ereport(LOG,
+             (errmsg("lock file \"%s\" contains wrong PID: %ld instead of %ld",
+                     DIRECTORY_LOCK_FILE, file_pid, (long) getpid())));
+     return false;
+ }
+
+
  /*-------------------------------------------------------------------------
   *                Version checking support
   *-------------------------------------------------------------------------
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 80ac732..87133bd 100644
*** a/src/include/miscadmin.h
--- b/src/include/miscadmin.h
*************** extern void CreateSocketLockFile(const c
*** 450,455 ****
--- 450,456 ----
                       const char *socketDir);
  extern void TouchSocketLockFiles(void);
  extern void AddToDataDirLockFile(int target_line, const char *str);
+ extern bool RecheckDataDirLockFile(void);
  extern void ValidatePgVersion(const char *path);
  extern void process_shared_preload_libraries(void);
  extern void process_session_preload_libraries(void);