Re: Idea for improving buildfarm robustness
Re: Idea for improving buildfarm robustness
От:
Tom Lane <tgl@sss.pgh.pa.us>
Дата:
Josh Berkus writes:
> Give me source with the change, and I'll put it on a cheap, low-bandwith
> AWS instance and hammer the heck out of it. That should raise any false
> positives we can expect.
Here's a draft patch against HEAD (looks like it will work on 9.5 or
9.4 without modifications, too).
regards, tom lane
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index baa43b2..52c9acd 100644
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
*************** static void CloseServerPorts(int status,
*** 373,378 ****
--- 373,379 ----
static void unlink_external_pid_file(int status, Datum arg);
static void getInstallationPaths(const char *argv0);
static void checkDataDir(void);
+ static bool recheckDataDir(void);
static Port *ConnCreate(int serverFd);
static void ConnFree(Port *port);
static void reset_shared(int port);
*************** checkDataDir(void)
*** 1490,1495 ****
--- 1491,1539 ----
}
/*
+ * Revalidate the data directory; return TRUE if OK, FALSE if not
+ *
+ * We don't try to check everything that checkDataDir() does. Ideally, we'd
+ * return FALSE *only* if the data directory has been deleted. As a proxy
+ * for that that matches a condition that checkDataDir() checked, verify that
+ * pg_control still exists. Because the postmaster will quit if we return
+ * FALSE, do not do so if there is any doubt or possibly-transient failure.
+ * Better to wait till we're sure.
+ *
+ * Unlike checkDataDir(), we assume we've chdir'd into the data directory.
+ */
+ static bool
+ recheckDataDir(void)
+ {
+ const char *path = "global/pg_control";
+ FILE *fp;
+
+ fp = AllocateFile(path, PG_BINARY_R);
+ if (fp != NULL)
+ {
+ FreeFile(fp);
+ return true;
+ }
+
+ /*
+ * There are many foreseeable false-positive error conditions, for example
+ * EINTR or ENFILE should not cause us to fail. For safety, fail only on
+ * enumerated clearly-something-is-wrong conditions.
+ */
+ switch (errno)
+ {
+ case ENOENT:
+ case ENOTDIR:
+ elog(LOG, "could not open file \"%s\": %m", path);
+ return false;
+ default:
+ elog(LOG, "could not open file \"%s\": %m; continuing anyway",
+ path);
+ return true;
+ }
+ }
+
+ /*
* Determine how long should we let ServerLoop sleep.
*
* In normal conditions we wait at most one minute, to ensure that the other
*************** ServerLoop(void)
*** 1602,1610 ****
fd_set readmask;
int nSockets;
time_t now,
last_touch_time;
! last_touch_time = time(NULL);
nSockets = initMasks(&readmask);
--- 1646,1655 ----
fd_set readmask;
int nSockets;
time_t now,
+ last_dir_recheck_time,
last_touch_time;
! last_dir_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
/*
--- 1799,1804 ----
*************** ServerLoop(void)
*** 1793,1798 ****
--- 1825,1868 ----
/* 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 $PGDATA hasn't been removed. If it has,
+ * we want to 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.
+ */
+ if (now - last_dir_recheck_time >= 1 * SECS_PER_MINUTE)
+ {
+ if (!recheckDataDir())
+ {
+ elog(LOG, "shutting down because data directory is gone");
+ kill(MyProcPid, SIGQUIT);
+ }
+ last_dir_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;
+ }
}
}
Re: Idea for improving buildfarm robustness
От:
Tom Lane <tgl@sss.pgh.pa.us>
Дата:
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);
Re: Idea for improving buildfarm robustness
От:
Michael Paquier <michael.paquier@gmail.com>
Дата:
On Sat, Oct 3, 2015 at 1:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I wrote:
> 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.
BTW, my thought at the moment is to wait till after next week's releases
to push this in. I think it's probably solid, but it doesn't seem like
it's worth taking the risk of pushing shortly before a wrap date.
That seems a wiser approach to me. Down to which version are you planning a backpatch? As this is aimed for the buildfarm stability with TAP stuff, 9.4?
--
--
Michael
