Attached a new patch, rebased against the current head. Errors in
pg_hba.conf and pg_ident.conf are now also noticed.
I checked the documentation for pg_ctl reload, and the only place where
it's explained seems to be runtime.sgml and that description is so
high-level that adding this new bit of functionality wouldn't make much
sense.
On Sat, Apr 25, 2015 at 10:03 AM, Jan de Visser <j...@de-visser.net> wrote:
> On April 22, 2015 06:04:42 PM Payal Singh wrote:
> > The following review has been posted through the commitfest application:
> > make installcheck-world: not tested
> > Implements feature: tested, failed
> > Spec compliant: not tested
> > Documentation: tested, failed
> >
> > Error in postgresql.conf gives the expected result on pg_ctl reload,
> > although errors in pg_hba.conf file don't. Like before, reload completes
> > fine without any information that pg_hba failed to load and only
> > information is present in the log file. I'm assuming pg_ctl reload should
> > prompt user if file fails to load irrespective of which file it is -
> > postgresql.conf or pg_hba.conf.
>
> Will fix. Not hard, just move the code that writes the .pid file to after
> the
> pg_hba reload.
>
> >
> > There is no documentation change so far, but I guess that's not yet
> > necessary.
>
> I will update the page for pg_ctl. At least the behaviour of -w/-W in
> relation
> to the reload command needs explained.
>
> >
> > gmake check passed all tests.
> >
> > The new status of this patch is: Waiting on Author
>
> jan
>
>
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index df8037b..909a078 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -1235,6 +1235,15 @@ PostmasterMain(int argc, char *argv[])
#endif
/*
+ * Update postmaster.pid with startup time as the last reload time:
+ */
+ {
+ char last_reload_info[32];
+ snprintf(last_reload_info, 32, "%ld %d", (long) MyStartTime, 1);
+ AddToDataDirLockFile(LOCK_FILE_LINE_LAST_RELOAD, last_reload_info);
+ }
+
+ /*
* Remember postmaster startup time
*/
PgStartTime = GetCurrentTimestamp();
@@ -2349,6 +2358,8 @@ static void
SIGHUP_handler(SIGNAL_ARGS)
{
int save_errno = errno;
+ bool reload_success;
+ char last_reload_info[32];
PG_SETMASK(&BlockSig);
@@ -2356,7 +2367,8 @@ SIGHUP_handler(SIGNAL_ARGS)
{
ereport(LOG,
(errmsg("received SIGHUP, reloading configuration files")));
- ProcessConfigFile(PGC_SIGHUP);
+ reload_success = ProcessConfigFile(PGC_SIGHUP);
+
SignalChildren(SIGHUP);
SignalUnconnectedWorkers(SIGHUP);
if (StartupPID != 0)
@@ -2379,13 +2391,25 @@ SIGHUP_handler(SIGNAL_ARGS)
signal_child(PgStatPID, SIGHUP);
/* Reload authentication config files too */
- if (!load_hba())
+ if (!load_hba()) {
+ reload_success = 0;
ereport(WARNING,
(errmsg("pg_hba.conf not reloaded")));
+ }
- if (!load_ident())
+ if (!load_ident()) {
+ reload_success = 0;
ereport(WARNING,
(errmsg("pg_ident.conf not reloaded")));
+ }
+
+ /*
+ * Write the current time and the result of the reload to the
+ * postmaster.pid file.
+ */
+ snprintf(last_reload_info, 32, "%ld %d",
+ (long) time(NULL), reload_success);
+ AddToDataDirLockFile(LOCK_FILE_LINE_LAST_RELOAD, last_reload_info);
#ifdef EXEC_BACKEND
/* Update the starting-point file for future children */
diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l
index 5b5846c..2f5537d 100644
--- a/src/backend/utils/misc/guc-file.l
+++ b/src/backend/utils/misc/guc-file.l
@@ -117,12 +117,13 @@ STRING \'([^'\\\n]|\\.|\'\')*\'
* If a hard error occurs, no values will be changed. (There can also be
* errors that prevent just one value from being changed.)
*/
-void
+bool
ProcessConfigFile(GucContext context)
{
int elevel;
MemoryContext config_cxt;
MemoryContext caller_cxt;
+ bool ok;
/*
* Config files are processed on startup (by the postmaster only) and on
@@ -153,16 +154,19 @@ ProcessConfigFile(GucContext context)
/*
* Read and apply the config file. We don't need to examine the result.
*/
- (void) ProcessConfigFileInternal(context, true, elevel);
+ ok = ProcessConfigFileInternal(context, true, elevel) != NULL;
/* Clean up */
MemoryContextSwitchTo(caller_cxt);
MemoryContextDelete(config_cxt);
+ return ok;
}
/*
* This function handles both actual config file (re)loads and execution of
- * show_all_file_settings() (i.e., the pg_file_settings view). In the latter
+ * show_all_file_settings() (i.e., the pg_file_settings view). In the former
+ * case, the settings are applied and this function returns the ConfigVariable
+ * list when this is successful, or NULL when it is not. In the latter
* case we don't apply any of the settings, but we make all the usual validity
* checks, and we return the ConfigVariable list so that it can be printed out
* by show_all_file_settings().
@@ -505,9 +509,13 @@ bail_out:
(errcode(ERRCODE_CONFIG_FILE_ERROR),
errmsg("configuration file \"%s\" contains errors; no changes were applied",
ConfFileWithError)));
+ head = NULL;
}
- /* Successful or otherwise, return the collected data list */
+ /*
+ * Return the collected data list or NULL when we attempted to apply the
+ * settings and failed.
+ */
return head;
}
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 74764fa..7fcadcf 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -73,6 +73,20 @@ typedef enum
RUN_AS_SERVICE_COMMAND
} CtlCommand;
+typedef struct
+{
+ pgpid_t pid;
+ char *datadir;
+ time_t startup_ts;
+ int port;
+ char *socketdir;
+ char *listenaddr;
+ unsigned long shmkey;
+ int shmid;
+ time_t reload_ts;
+ bool reload_ok;
+} PIDFileContents;
+
#define DEFAULT_WAIT 60
static bool do_wait = false;
@@ -153,6 +167,8 @@ static int CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo,
static pgpid_t get_pgpid(bool is_status_request);
static char **readfile(const char *path);
static void free_readfile(char **optlines);
+static PIDFileContents * get_pidfile_contents(const char *path);
+static void free_pidfile_contents(PIDFileContents *contents);
static int start_postmaster(void);
static void read_post_opts(void);
@@ -416,6 +432,78 @@ free_readfile(char **optlines)
}
/*
+ * Read and parse the contents of a postmaster.pid file. These contents are
+ * placed in a PIDFileContents struct, a pointer to which is returned. If the
+ * file could not be read NULL is returned.
+ */
+static PIDFileContents *
+get_pidfile_contents(const char *path)
+{
+ char **optlines = readfile(path);
+ PIDFileContents *result = NULL;
+ int lines;
+
+ if (optlines && optlines[0]) {
+ /*
+ * Count number of lines returned:
+ */
+ for (lines = 0; optlines[lines]; lines++);
+
+ result = (PIDFileContents *) pg_malloc0(sizeof(PIDFileContents));
+ result -> pid = atol(optlines[LOCK_FILE_LINE_PID - 1]);
+ result->datadir = (lines >= LOCK_FILE_LINE_DATA_DIR)
+ ? pg_strdup(optlines[LOCK_FILE_LINE_DATA_DIR - 1])
+ : NULL;
+ result->startup_ts = (lines >= LOCK_FILE_LINE_START_TIME)
+ ? atol(optlines[LOCK_FILE_LINE_START_TIME - 1])
+ : 0L;
+ result->port = (lines >= LOCK_FILE_LINE_PORT)
+ ? atoi(optlines[LOCK_FILE_LINE_PORT - 1])
+ : 0;
+ result->socketdir = (lines >= LOCK_FILE_LINE_SOCKET_DIR)
+ ? pg_strdup(optlines[LOCK_FILE_LINE_SOCKET_DIR - 1])
+ : NULL;
+ result->listenaddr = (lines >= LOCK_FILE_LINE_LISTEN_ADDR)
+ ? pg_strdup(optlines[LOCK_FILE_LINE_LISTEN_ADDR - 1])
+ : NULL;
+ if (lines >= LOCK_FILE_LINE_SHMEM_KEY) {
+ sscanf(optlines[LOCK_FILE_LINE_SHMEM_KEY - 1], "%9lu %9d",
+ &result->shmkey, &result->shmid);
+ } else {
+ result->shmkey = 0;
+ result->shmid = 0;
+ }
+ if (lines >= LOCK_FILE_LINE_LAST_RELOAD) {
+ char *ptr = strchr(optlines[LOCK_FILE_LINE_LAST_RELOAD - 1], ' ');
+ *ptr = 0;
+ result->reload_ts = atol(optlines[LOCK_FILE_LINE_LAST_RELOAD - 1]);
+ result->reload_ok = (bool) atoi(ptr+1);
+ *ptr = ' ';
+ } else {
+ result->reload_ts = 0;
+ result->reload_ok = 0;
+ }
+ free_readfile(optlines);
+ }
+ return result;
+}
+
+/*
+ * Free the memory allocated by get_pidfile_contents.
+ */
+static void
+free_pidfile_contents(PIDFileContents *contents)
+{
+ if (contents) {
+ pg_free(contents->datadir);
+ pg_free(contents->socketdir);
+ pg_free(contents->listenaddr);
+ free(contents);
+ }
+}
+
+
+/*
* start/test/stop routines
*/
@@ -1094,7 +1182,10 @@ do_restart(void)
static void
do_reload(void)
{
- pgpid_t pid;
+ pgpid_t pid;
+ PIDFileContents *current_contents;
+ PIDFileContents *new_contents = NULL;
+ int retries;
pid = get_pgpid(false);
if (pid == 0) /* no pid file */
@@ -1113,14 +1204,101 @@ do_reload(void)
exit(1);
}
+ /*
+ * Load the current contents of the postmaster.pid, so that after the
+ * SIGHUP we can compare the reload timestamp with the one currently in
+ * the file.
+ */
+ current_contents = get_pidfile_contents(pid_file);
+ if (!current_contents) {
+ write_stderr(_("%s: PID file \"%s\" can not be read\n"), progname, pid_file);
+ write_stderr(_("Is server running?\n"));
+ exit(1);
+ }
+
if (kill((pid_t) pid, sig) != 0)
{
write_stderr(_("%s: could not send reload signal (PID: %ld): %s\n"),
progname, pid, strerror(errno));
exit(1);
}
-
print_msg(_("server signaled\n"));
+
+ /*
+ * - Pre-9.6 servers don't write the last-reload timestamp, so we can
+ * only assume everything was OK.
+ * - If -W was specified on the command line, the user doesn't care
+ * about the result and we leave.
+ */
+ if ((current_contents->reload_ts == 0) || !do_wait) {
+ return;
+ }
+
+ /*
+ * Load the current contents of the postmaster.pid file, and check
+ * if the reload timestamp is newer than the one in the current
+ * contents loaded before the SIGHUP. If the file cannot be read, or
+ * the timestamp is not newer, the reload hasn't finished yet. In that
+ * case sleep and try again, for a maximum of 5 times.
+ */
+ for (retries = 0; retries < 5; retries++) {
+
+ /*
+ * Wait 1 second - the first time around to give the postmaster the
+ * opportunity to actually rewrite postmaster.pid.
+ */
+ pg_usleep(1000000); /* 1 sec */
+
+ /*
+ * free_pidfile_contents knows how to deal with NULL pointers
+ * and therefore this is safe the first time around:
+ */
+ free_pidfile_contents(new_contents);
+ new_contents = get_pidfile_contents(pid_file);
+ if (new_contents) {
+
+ /*
+ * We were able to read the postmaster.pid file:
+ */
+ if (new_contents->reload_ts > current_contents->reload_ts) {
+
+ /*
+ * The reload timestamp is newer that the "old" timestamp:
+ */
+ if (new_contents->reload_ok) {
+
+ /*
+ * The reload was successful:
+ */
+ break;
+ } else {
+
+ /*
+ * The reload failed, probably because of errors in the
+ * config file. The server will continue to run (with the
+ * old config!) but will fail to start the next time
+ * it is restarted.
+ */
+ write_stderr(_("%s: Reload of server with PID %ld FAILED\n"),
+ progname, pid);
+ write_stderr(_("Consult the server log for details.\n"));
+ break;
+ }
+ }
+ }
+ }
+ free_pidfile_contents(new_contents);
+ free_pidfile_contents(current_contents);
+
+ if (retries >= 5) {
+
+ /*
+ * We couldn't read a new postmaster.pid after 5 retries.
+ */
+ write_stderr(_("%s: Server with PID %ld non-responsive after reload request\n"),
+ progname, pid);
+ write_stderr(_("Consult the server log for details.\n"));
+ }
}
@@ -2361,6 +2539,7 @@ main(int argc, char **argv)
do_wait = false;
break;
case STOP_COMMAND:
+ case RELOAD_COMMAND:
do_wait = true;
break;
default:
@@ -2371,7 +2550,6 @@ main(int argc, char **argv)
if (ctl_command == RELOAD_COMMAND)
{
sig = SIGHUP;
- do_wait = false;
}
if (pg_data)
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index b539167..176f715 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -445,6 +445,7 @@ extern char *local_preload_libraries_string;
#define LOCK_FILE_LINE_SOCKET_DIR 5
#define LOCK_FILE_LINE_LISTEN_ADDR 6
#define LOCK_FILE_LINE_SHMEM_KEY 7
+#define LOCK_FILE_LINE_LAST_RELOAD 8
extern void CreateDataDirLockFile(bool amPostmaster);
extern void CreateSocketLockFile(const char *socketfile, bool amPostmaster,
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index dc167f9..7e7cc02 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -348,7 +348,7 @@ extern void EmitWarningsOnPlaceholders(const char *className);
extern const char *GetConfigOption(const char *name, bool missing_ok,
bool restrict_superuser);
extern const char *GetConfigOptionResetString(const char *name);
-extern void ProcessConfigFile(GucContext context);
+extern bool ProcessConfigFile(GucContext context);
extern void InitializeGUCOptions(void);
extern bool SelectConfigFiles(const char *userDoption, const char *progname);
extern void ResetAllOptions(void);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
