Idea: closing the loop for "pg_ctl reload"
Bug #12788 reminded me of a problem I think we've discussed before:
if you use "pg_ctl reload" to trigger reload of the postmaster's
config files, and there's something wrong with those files, there's
no warning to you of that. The postmaster just bleats to its log and
keeps running. If you don't think to look in the log to confirm
successful reload, you're left with a time bomb: the next attempt
to restart the postmaster will fail altogether because of the bad
config file.
I commented in the bug thread that there wasn't much that pg_ctl
could do about this, but on reflection it seems like something we
could fix, because pg_ctl must be able to read the postmaster.pid
file in order to issue a reload signal. Consider a design like this:
1. Extend the definition of the postmaster.pid file to add another
line, which will contain the time of the last postmaster configuration
load attempt (might as well be a numeric Unix-style timestamp) and
a boolean indication of whether that attempt succeeded or not.
2. Change pg_ctl so that after sending a reload signal, it sleeps
for a second and checks for a change in the config load timestamp
(repeat as necessary till timeout). Once it sees the timestamp
change, it's in a position to report success or failure using the
boolean. I think this should become the default behavior, though
you could define -W to mean that there should be no wait or feedback.
It's tempting to think of storing a whole error message rather than
just a boolean, but I think that would complicate the pidfile definition
undesirably. A warning to look in the postmaster log ought to be
sufficient here.
For extra credit, the pg_reload_conf() function could be made to behave
similarly.
I don't have the time to pursue this idea myself, but perhaps someone
looking for a not-too-complicated project could take it on.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On February 19, 2015 08:26:45 PM Tom Lane wrote:
I don't have the time to pursue this idea myself, but perhaps someone
looking for a not-too-complicated project could take it on.
I can have a crack at this. What's the process? Do I add it to a CF once I
have a patch, or do I do that beforehand?
jan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Jan de Visser <jan@de-visser.net> writes:
I can have a crack at this. What's the process? Do I add it to a CF once I
have a patch, or do I do that beforehand?
The CF process is for reviewing things, so until you have either a patch
or a design sketch you want feedback on, there's no need for a CF entry.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On February 19, 2015 08:26:45 PM Tom Lane wrote:
Bug #12788 reminded me of a problem I think we've discussed before:
if you use "pg_ctl reload" to trigger reload of the postmaster's
config files, and there's something wrong with those files, there's
no warning to you of that. The postmaster just bleats to its log and
keeps running. If you don't think to look in the log to confirm
successful reload, you're left with a time bomb: the next attempt
to restart the postmaster will fail altogether because of the bad
config file.I commented in the bug thread that there wasn't much that pg_ctl
could do about this, but on reflection it seems like something we
could fix, because pg_ctl must be able to read the postmaster.pid
file in order to issue a reload signal. Consider a design like this:1. Extend the definition of the postmaster.pid file to add another
line, which will contain the time of the last postmaster configuration
load attempt (might as well be a numeric Unix-style timestamp) and
a boolean indication of whether that attempt succeeded or not.2. Change pg_ctl so that after sending a reload signal, it sleeps
for a second and checks for a change in the config load timestamp
(repeat as necessary till timeout). Once it sees the timestamp
change, it's in a position to report success or failure using the
boolean. I think this should become the default behavior, though
you could define -W to mean that there should be no wait or feedback.It's tempting to think of storing a whole error message rather than
just a boolean, but I think that would complicate the pidfile definition
undesirably. A warning to look in the postmaster log ought to be
sufficient here.For extra credit, the pg_reload_conf() function could be made to behave
similarly.I don't have the time to pursue this idea myself, but perhaps someone
looking for a not-too-complicated project could take it on.regards, tom lane
Here's my first crack at this. Notes:
1/ I haven't done the '-W' flag Tom mentions yet.
2/ Likewise haven't touched pg_reload_conf()
3/ Design details: I introduced a new struct in pg_ctl containing the parsed-
out data from postmaster.pid, with functions to retrieve the data and to
dispose memory allocated for it. This made the change in do_reload fairly
straightforward. I think this struct can be used in other places in pg_ctl as
well, and potentially in other files as well.
4/ Question: Can I assume pg_malloc allocated memory is set to zero? If not,
is it OK to do a memset(..., 0, ...)? I don't have much experience on any of
the esoteric platforms pgsql supports...
jan
Attachments:
Let_pg_ctl_check_the_result_of_a_postmaster_config_reload.patchtext/x-patch; charset=UTF-8; name=Let_pg_ctl_check_the_result_of_a_postmaster_config_reload.patchDownload
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
***************
*** 2341,2346 **** static void
--- 2341,2348 ----
SIGHUP_handler(SIGNAL_ARGS)
{
int save_errno = errno;
+ bool reload_success;
+ char last_reload_info[32];
PG_SETMASK(&BlockSig);
***************
*** 2348,2354 **** SIGHUP_handler(SIGNAL_ARGS)
{
ereport(LOG,
(errmsg("received SIGHUP, reloading configuration files")));
! ProcessConfigFile(PGC_SIGHUP);
SignalChildren(SIGHUP);
SignalUnconnectedWorkers(SIGHUP);
if (StartupPID != 0)
--- 2350,2365 ----
{
ereport(LOG,
(errmsg("received SIGHUP, reloading configuration files")));
! reload_success = ProcessConfigFile(PGC_SIGHUP);
!
! /*
! * 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);
!
SignalChildren(SIGHUP);
SignalUnconnectedWorkers(SIGHUP);
if (StartupPID != 0)
*** a/src/backend/utils/misc/guc-file.l
--- b/src/backend/utils/misc/guc-file.l
***************
*** 112,118 **** STRING \'([^'\\\n]|\\.|\'\')*\'
* All options mentioned in the configuration file are set to new values.
* If an error occurs, no values will be changed.
*/
! void
ProcessConfigFile(GucContext context)
{
bool error = false;
--- 112,118 ----
* All options mentioned in the configuration file are set to new values.
* If an error occurs, no values will be changed.
*/
! bool
ProcessConfigFile(GucContext context)
{
bool error = false;
***************
*** 205,211 **** ProcessConfigFile(GucContext context)
* the config file.
*/
if (head == NULL)
! return;
}
/*
--- 205,211 ----
* the config file.
*/
if (head == NULL)
! return false;
}
/*
***************
*** 433,438 **** ProcessConfigFile(GucContext context)
--- 433,439 ----
* freed here.
*/
FreeConfigVariables(head);
+ return !error;
}
/*
*** a/src/bin/pg_ctl/pg_ctl.c
--- b/src/bin/pg_ctl/pg_ctl.c
***************
*** 73,78 **** typedef enum
--- 73,92 ----
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;
***************
*** 157,162 **** static int CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo,
--- 171,178 ----
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);
***************
*** 419,424 **** free_readfile(char **optlines)
--- 435,512 ----
}
/*
+ * 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_malloc(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
*/
***************
*** 1097,1103 **** do_restart(void)
static void
do_reload(void)
{
! pgpid_t pid;
pid = get_pgpid(false);
if (pid == 0) /* no pid file */
--- 1185,1194 ----
static void
do_reload(void)
{
! pgpid_t pid;
! PIDFileContents *current_contents;
! PIDFileContents *new_contents = NULL;
! int retries;
pid = get_pgpid(false);
if (pid == 0) /* no pid file */
***************
*** 1116,1129 **** do_reload(void)
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"));
}
--- 1207,1294 ----
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"));
+
+ /*
+ * 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++) {
+ 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;
+ }
+ } else if (new_contents->reload_ts == 0) {
+
+ /*
+ * The server is pre-9.5 and doesn't write a reload
+ * timestamp. Just assume everything is OK.
+ */
+ break;
+ }
+ }
+ pg_usleep(1000000); /* 1 sec */
+ }
+ 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"));
+ }
}
*** a/src/include/miscadmin.h
--- b/src/include/miscadmin.h
***************
*** 444,449 **** extern char *local_preload_libraries_string;
--- 444,450 ----
#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,
*** a/src/include/utils/guc.h
--- b/src/include/utils/guc.h
***************
*** 334,340 **** 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 void InitializeGUCOptions(void);
extern bool SelectConfigFiles(const char *userDoption, const char *progname);
extern void ResetAllOptions(void);
--- 334,340 ----
extern const char *GetConfigOption(const char *name, bool missing_ok,
bool restrict_superuser);
extern const char *GetConfigOptionResetString(const char *name);
! extern bool ProcessConfigFile(GucContext context);
extern void InitializeGUCOptions(void);
extern bool SelectConfigFiles(const char *userDoption, const char *progname);
extern void ResetAllOptions(void);
On March 2, 2015 12:56:23 AM Jan de Visser wrote:
... stuff ...
I seem to have mis-clicked something in the CF app - I created two entries
somehow. I think one got created when I entered the msgid of Tom's original
message with the enclosing '<...>'. If that's the case, then that may be a
bug.
jan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Mar 2, 2015 at 3:01 PM, Jan de Visser <jan@de-visser.net> wrote:
On March 2, 2015 12:56:23 AM Jan de Visser wrote:
... stuff ...I seem to have mis-clicked something in the CF app - I created two entries
somehow. I think one got created when I entered the msgid of Tom's original
message with the enclosing '<...>'. If that's the case, then that may be a
bug.
Don't worry for that. I just removed the duplicated entry.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Jan de Visser <jan@de-visser.net> writes:
4/ Question: Can I assume pg_malloc allocated memory is set to zero? If not,
is it OK to do a memset(..., 0, ...)? I don't have much experience on any of
the esoteric platforms pgsql supports...
No, you need the memset. You might accidentally get already-zeroed memory
from malloc, but it's not something you can rely on.
However, you could and should use pg_malloc0, which takes care of that
for you...
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On March 2, 2015 09:50:49 AM Tom Lane wrote:
However, you could and should use pg_malloc0, which takes care of that
for you...
I am (using pg_malloc, that is). So, just to be sure: pg_malloc memsets the
block to 0, right?
My question was more along the lines if memsetting to 0 to ensure that pointer
fields are NULL and int/long fields are 0. I know they are on Linux, but don't
know if that applies to other platforms as well, or if I need to set fields
explicitly to those 'zero'/'uninitialized' values.
jan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Jan de Visser <jan@de-visser.net> writes:
On March 2, 2015 09:50:49 AM Tom Lane wrote:
However, you could and should use pg_malloc0, which takes care of that
for you...
I am (using pg_malloc, that is). So, just to be sure: pg_malloc memsets the
block to 0, right?
No, it doesn't, but pg_malloc0 does. Consult the code if you're confused:
src/common/fe_memutils.c
My question was more along the lines if memsetting to 0 to ensure that pointer
fields are NULL and int/long fields are 0.
Yes, we do assume that widely, and so does a heck of a lot of other code.
In principle the C standard doesn't require that a NULL pointer be
all-zero-bits, only that casting "0" to a pointer yield a NULL pointer.
But certainly there are no modern implementations that don't represent
NULL as 0. Anybody who tried to do it differently would soon find that
hardly any real-world C code would run on their platform.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On March 2, 2015 12:44:40 PM Tom Lane wrote:
No, it doesn't, but pg_malloc0 does. Consult the code if you're confused:
src/common/fe_memutils.c
Doh! I read pg_malloc( ), not pg_malloc0.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Jan de Visser <jan@de-visser.net> wrote:
On March 2, 2015 09:50:49 AM Tom Lane wrote:
However, you could and should use pg_malloc0, which takes care
of that for you...I am (using pg_malloc, that is). So, just to be sure: pg_malloc
memsets the block to 0, right?
I think you may have misread a zero character as an empty pair of
parentheses. Tom pointed out that the pg_malloc() function gives
you uninitialized memory -- you cannot count on the contents. He
further pointed out that if you need it to be initialized to '0'
bytes you should call the pg_malloc0() function rather than calling
the pg_malloc() function and running memset separately.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On March 2, 2015 12:56:23 AM Jan de Visser wrote:
Here's my first crack at this. Notes:
1/ I haven't done the '-W' flag Tom mentions yet.
2/ Likewise haven't touched pg_reload_conf()
3/ Design details: I introduced a new struct in pg_ctl containing the
parsed- out data from postmaster.pid, with functions to retrieve the data
and to dispose memory allocated for it. This made the change in do_reload
fairly straightforward. I think this struct can be used in other places in
pg_ctl as well, and potentially in other files as well.
4/ Question: Can I assume pg_malloc allocated memory is set to zero? If not,
is it OK to do a memset(..., 0, ...)? I don't have much experience on any
of the esoteric platforms pgsql supports...
Slight tweaks to better deal with pre-9.5 (6?) servers that don't write the
reload timestamp. I tested with a 9.4 server and it seemed to work...
Also implemented -W/-w. Haven't done pg_reload_conf() yet.
Attachments:
Let_pg_ctl_check_the_result_of_a_postmaster_config_reload.patchtext/x-patch; charset=UTF-8; name=Let_pg_ctl_check_the_result_of_a_postmaster_config_reload.patchDownload
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
***************
*** 883,888 **** PostmasterMain(int argc, char *argv[])
--- 883,899 ----
CreateDataDirLockFile(true);
/*
+ * 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);
+ }
+
+
+ /*
* Initialize SSL library, if specified.
*/
#ifdef USE_SSL
***************
*** 2341,2346 **** static void
--- 2352,2359 ----
SIGHUP_handler(SIGNAL_ARGS)
{
int save_errno = errno;
+ bool reload_success;
+ char last_reload_info[32];
PG_SETMASK(&BlockSig);
***************
*** 2348,2354 **** SIGHUP_handler(SIGNAL_ARGS)
{
ereport(LOG,
(errmsg("received SIGHUP, reloading configuration files")));
! ProcessConfigFile(PGC_SIGHUP);
SignalChildren(SIGHUP);
SignalUnconnectedWorkers(SIGHUP);
if (StartupPID != 0)
--- 2361,2376 ----
{
ereport(LOG,
(errmsg("received SIGHUP, reloading configuration files")));
! reload_success = ProcessConfigFile(PGC_SIGHUP);
!
! /*
! * 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);
!
SignalChildren(SIGHUP);
SignalUnconnectedWorkers(SIGHUP);
if (StartupPID != 0)
*** a/src/backend/utils/misc/guc-file.l
--- b/src/backend/utils/misc/guc-file.l
***************
*** 112,118 **** STRING \'([^'\\\n]|\\.|\'\')*\'
* All options mentioned in the configuration file are set to new values.
* If an error occurs, no values will be changed.
*/
! void
ProcessConfigFile(GucContext context)
{
bool error = false;
--- 112,118 ----
* All options mentioned in the configuration file are set to new values.
* If an error occurs, no values will be changed.
*/
! bool
ProcessConfigFile(GucContext context)
{
bool error = false;
***************
*** 205,211 **** ProcessConfigFile(GucContext context)
* the config file.
*/
if (head == NULL)
! return;
}
/*
--- 205,211 ----
* the config file.
*/
if (head == NULL)
! return false;
}
/*
***************
*** 433,438 **** ProcessConfigFile(GucContext context)
--- 433,439 ----
* freed here.
*/
FreeConfigVariables(head);
+ return !error;
}
/*
*** a/src/bin/pg_ctl/pg_ctl.c
--- b/src/bin/pg_ctl/pg_ctl.c
***************
*** 73,78 **** typedef enum
--- 73,92 ----
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;
***************
*** 157,162 **** static int CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo,
--- 171,178 ----
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);
***************
*** 419,424 **** free_readfile(char **optlines)
--- 435,512 ----
}
/*
+ * 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
*/
***************
*** 1097,1103 **** do_restart(void)
static void
do_reload(void)
{
! pgpid_t pid;
pid = get_pgpid(false);
if (pid == 0) /* no pid file */
--- 1185,1194 ----
static void
do_reload(void)
{
! pgpid_t pid;
! PIDFileContents *current_contents;
! PIDFileContents *new_contents = NULL;
! int retries;
pid = get_pgpid(false);
if (pid == 0) /* no pid file */
***************
*** 1116,1129 **** do_reload(void)
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"));
}
--- 1207,1307 ----
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.5 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"));
+ }
}
***************
*** 2352,2359 **** main(int argc, char **argv)
do_wait = false;
break;
case STOP_COMMAND:
do_wait = true;
! break;
default:
break;
}
--- 2530,2538 ----
do_wait = false;
break;
case STOP_COMMAND:
+ case RELOAD_COMMAND:
do_wait = true;
! break;
default:
break;
}
***************
*** 2362,2368 **** main(int argc, char **argv)
if (ctl_command == RELOAD_COMMAND)
{
sig = SIGHUP;
- do_wait = false;
}
if (pg_data)
--- 2541,2546 ----
*** a/src/include/miscadmin.h
--- b/src/include/miscadmin.h
***************
*** 444,449 **** extern char *local_preload_libraries_string;
--- 444,450 ----
#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,
*** a/src/include/utils/guc.h
--- b/src/include/utils/guc.h
***************
*** 334,340 **** 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 void InitializeGUCOptions(void);
extern bool SelectConfigFiles(const char *userDoption, const char *progname);
extern void ResetAllOptions(void);
--- 334,340 ----
extern const char *GetConfigOption(const char *name, bool missing_ok,
bool restrict_superuser);
extern const char *GetConfigOptionResetString(const char *name);
! extern bool ProcessConfigFile(GucContext context);
extern void InitializeGUCOptions(void);
extern bool SelectConfigFiles(const char *userDoption, const char *progname);
extern void ResetAllOptions(void);
On Fri, Feb 20, 2015 at 1:26 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
1. Extend the definition of the postmaster.pid file to add another
line, which will contain the time of the last postmaster configuration
load attempt (might as well be a numeric Unix-style timestamp) and
a boolean indication of whether that attempt succeeded or not.
Fwiw this concerns me slightly. I'm sure a lot of people are doing
things like "kill -HUP `cat .../postmaster.pid`" or the equivalent.
We do have the external_pid_file GUC so perhaps this just means we
should warn people in the release notes or somewhere and point them at
that GUC.
--
greg
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-03-03 15:21:24 +0000, Greg Stark wrote:
Fwiw this concerns me slightly. I'm sure a lot of people are doing
things like "kill -HUP `cat .../postmaster.pid`" or the equivalent.
postmaster.pid already contains considerably more than just the pid. e.g.
4071
/srv/dev/pgdev-master
1425396089
5440
/tmp
localhost
5440001 82345992
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@2ndquadrant.com> writes:
On 2015-03-03 15:21:24 +0000, Greg Stark wrote:
Fwiw this concerns me slightly. I'm sure a lot of people are doing
things like "kill -HUP `cat .../postmaster.pid`" or the equivalent.
postmaster.pid already contains considerably more than just the pid. e.g.
Yeah, that ship sailed long ago. I'm sure anyone who's doing this is
using "head -1" not just "cat", and what I suggest wouldn't break that.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 3/3/15 9:26 AM, Andres Freund wrote:
On 2015-03-03 15:21:24 +0000, Greg Stark wrote:
Fwiw this concerns me slightly. I'm sure a lot of people are doing
things like "kill -HUP `cat .../postmaster.pid`" or the equivalent.postmaster.pid already contains considerably more than just the pid. e.g.
4071
/srv/dev/pgdev-master
1425396089
5440
/tmp
localhost
5440001 82345992
If we already have all this extra stuff, why not include an actual error
message then, or at least the first line of an error (or maybe just swap
any newlines with spaces)?
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On March 3, 2015 10:29:43 AM Tom Lane wrote:
Andres Freund <andres@2ndquadrant.com> writes:
On 2015-03-03 15:21:24 +0000, Greg Stark wrote:
Fwiw this concerns me slightly. I'm sure a lot of people are doing
things like "kill -HUP `cat .../postmaster.pid`" or the equivalent.postmaster.pid already contains considerably more than just the pid. e.g.
Yeah, that ship sailed long ago. I'm sure anyone who's doing this is
using "head -1" not just "cat", and what I suggest wouldn't break that.regards, tom lane
And what I've implemented doesn't either. The extra line is added to the back
of the file.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On March 3, 2015 11:09:29 AM Jim Nasby wrote:
On 3/3/15 9:26 AM, Andres Freund wrote:
On 2015-03-03 15:21:24 +0000, Greg Stark wrote:
Fwiw this concerns me slightly. I'm sure a lot of people are doing
things like "kill -HUP `cat .../postmaster.pid`" or the equivalent.postmaster.pid already contains considerably more than just the pid. e.g.
4071
/srv/dev/pgdev-master
1425396089
5440
/tmp
localhost5440001 82345992
If we already have all this extra stuff, why not include an actual error
message then, or at least the first line of an error (or maybe just swap
any newlines with spaces)?
Not impossible. I can play around with that and see if it's as straightforward
as I think it is.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 3/3/15 11:15 AM, Jan de Visser wrote:
On March 3, 2015 11:09:29 AM Jim Nasby wrote:
On 3/3/15 9:26 AM, Andres Freund wrote:
On 2015-03-03 15:21:24 +0000, Greg Stark wrote:
Fwiw this concerns me slightly. I'm sure a lot of people are doing
things like "kill -HUP `cat .../postmaster.pid`" or the equivalent.postmaster.pid already contains considerably more than just the pid. e.g.
4071
/srv/dev/pgdev-master
1425396089
5440
/tmp
localhost5440001 82345992
If we already have all this extra stuff, why not include an actual error
message then, or at least the first line of an error (or maybe just swap
any newlines with spaces)?Not impossible. I can play around with that and see if it's as straightforward
as I think it is.
I'm sure the code side of this is trivial; it's a question of why Tom
was objecting. It would probably be better for us to come to a
conclusion before working on this.
On a related note... something else we could do here would be to keep a
last-known-good copy of the config files around. That way if you flubbed
something at least the server would still start. I do think that any
admin worth anything would notice an error from pg_ctl, but maybe others
have a different opinion.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-03-03 11:09:29 -0600, Jim Nasby wrote:
On 3/3/15 9:26 AM, Andres Freund wrote:
On 2015-03-03 15:21:24 +0000, Greg Stark wrote:
Fwiw this concerns me slightly. I'm sure a lot of people are doing
things like "kill -HUP `cat .../postmaster.pid`" or the equivalent.postmaster.pid already contains considerably more than just the pid. e.g.
4071
/srv/dev/pgdev-master
1425396089
5440
/tmp
localhost
5440001 82345992If we already have all this extra stuff, why not include an actual error
message then, or at least the first line of an error (or maybe just swap any
newlines with spaces)?
It's often several errors and such. I think knowing that it failed and
that you should look into the error log is already quite helpful
already.
Generally we obviously need some status to indicate that the config file
has been reloaded, but that could be easily combined with storing the
error message.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 3/3/15 11:33 AM, Andres Freund wrote:
On 2015-03-03 11:09:29 -0600, Jim Nasby wrote:
On 3/3/15 9:26 AM, Andres Freund wrote:
On 2015-03-03 15:21:24 +0000, Greg Stark wrote:
Fwiw this concerns me slightly. I'm sure a lot of people are doing
things like "kill -HUP `cat .../postmaster.pid`" or the equivalent.postmaster.pid already contains considerably more than just the pid. e.g.
4071
/srv/dev/pgdev-master
1425396089
5440
/tmp
localhost
5440001 82345992If we already have all this extra stuff, why not include an actual error
message then, or at least the first line of an error (or maybe just swap any
newlines with spaces)?It's often several errors and such. I think knowing that it failed and
that you should look into the error log is already quite helpful
already.
It's certainly better than now, but why put DBAs through an extra step
for no reason? Though, in the case of multiple errors perhaps it would
be best to just report a count and point them at the log.
Generally we obviously need some status to indicate that the config file
has been reloaded, but that could be easily combined with storing the
error message.
Not sure I'm following... are you saying we should include the error
message in postmaster.pid?
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-03-03 11:43:46 -0600, Jim Nasby wrote:
It's certainly better than now, but why put DBAs through an extra step for
no reason?
Because it makes it more complicated than it already is? It's nontrivial
to capture the output, escape it to somehow fit into a delimited field,
et al. I'd rather have a committed improvement, than talks about a
bigger one.
Though, in the case of multiple errors perhaps it would be best
to just report a count and point them at the log.
It'll be confusing to have different interfaces in one/multiple error cases.
Generally we obviously need some status to indicate that the config file
has been reloaded, but that could be easily combined with storing the
error message.Not sure I'm following... are you saying we should include the error message
in postmaster.pid?
I'm saying that you'll need a way to notice that a reload was processed
or not. And that can't really be the message itself, there has to be
some other field; like the timestamp Tom proposes.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 3/3/15 11:48 AM, Andres Freund wrote:
On 2015-03-03 11:43:46 -0600, Jim Nasby wrote:
It's certainly better than now, but why put DBAs through an extra step for
no reason?Because it makes it more complicated than it already is? It's nontrivial
to capture the output, escape it to somehow fit into a delimited field,
et al. I'd rather have a committed improvement, than talks about a
bigger one.
I don't see how this would be significantly more complex, but of course
that's subjective.
Though, in the case of multiple errors perhaps it would be best
to just report a count and point them at the log.It'll be confusing to have different interfaces in one/multiple error cases.
If we simply don't want the code complexity then fine, but I just don't
buy this argument. How could it possibly be confusing? Either you'll get
the actual error message or a message that says "Didn't work, check the
log." And the error will always be in the log no matter what. I really
can't imagine that anyone would be unhappy that we just up-front told
them what the error was if we could. Why make them jump through needless
hoops?
I'm saying that you'll need a way to notice that a reload was processed
or not. And that can't really be the message itself, there has to be
some other field; like the timestamp Tom proposes.
Ahh, right. We should make sure we don't go brain-dead if the system
clock moves backwards. I assume we couldn't just fstat the file...
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
On 3/3/15 11:48 AM, Andres Freund wrote:
It'll be confusing to have different interfaces in one/multiple error cases.
If we simply don't want the code complexity then fine, but I just don't
buy this argument. How could it possibly be confusing?
What I'm concerned about is confusing the code. There is a lot of stuff
that looks at pidfiles and a lot of it is not very bright (note upthread
argument about "cat" vs "head -1"). I don't want possibly localized
(non-ASCII) text in there, especially when there's not going to be any
sane way to know which encoding it's in. And I definitely don't want
multiline error messages in there.
It's possible we could dumb things down enough to meet these restrictions,
but I'd really rather not go there at all.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On March 3, 2015 04:57:58 PM Jim Nasby wrote:
On 3/3/15 11:48 AM, Andres Freund wrote:
I'm saying that you'll need a way to notice that a reload was processed
or not. And that can't really be the message itself, there has to be
some other field; like the timestamp Tom proposes.Ahh, right. We should make sure we don't go brain-dead if the system
clock moves backwards. I assume we couldn't just fstat the file...
The timestamp field is already there (in my patch). It gets populated when the
server starts and repopulated by SIGHUP_handler. It's the only timestamp
pg_ctl pays attention to.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 3/3/15 5:13 PM, Tom Lane wrote:
Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
On 3/3/15 11:48 AM, Andres Freund wrote:
It'll be confusing to have different interfaces in one/multiple error cases.
If we simply don't want the code complexity then fine, but I just don't
buy this argument. How could it possibly be confusing?What I'm concerned about is confusing the code. There is a lot of stuff
that looks at pidfiles and a lot of it is not very bright (note upthread
argument about "cat" vs "head -1"). I don't want possibly localized
(non-ASCII) text in there, especially when there's not going to be any
sane way to know which encoding it's in. And I definitely don't want
multiline error messages in there.
Definitely no multi-line. If we keep that restriction, couldn't we just
dedicate one entire line to the error message? ISTM that would be safe.
It's possible we could dumb things down enough to meet these restrictions,
but I'd really rather not go there at all.
IMHO the added DBA convenience would be worth it (assuming we can make
it safe). I know I'd certainly appreciate it...
On 3/3/15 5:24 PM, Jan de Visser wrote:> On March 3, 2015 04:57:58 PM
Jim Nasby wrote:
On 3/3/15 11:48 AM, Andres Freund wrote:
I'm saying that you'll need a way to notice that a reload was
processed
or not. And that can't really be the message itself, there has to be
some other field; like the timestamp Tom proposes.Ahh, right. We should make sure we don't go brain-dead if the system
clock moves backwards. I assume we couldn't just fstat the file...The timestamp field is already there (in my patch). It gets populated
when the
server starts and repopulated by SIGHUP_handler. It's the only timestamp
pg_ctl pays attention to.
What happens if someone does a reload sometime in the future (perhaps
because they've messed with the system clock for test purposes). Do we
still detect a new reload?
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On March 3, 2015 06:34:33 PM Jim Nasby wrote:
On 3/3/15 5:24 PM, Jan de Visser wrote:> On March 3, 2015 04:57:58 PM
Jim Nasby wrote:On 3/3/15 11:48 AM, Andres Freund wrote:
I'm saying that you'll need a way to notice that a reload was
processed
or not. And that can't really be the message itself, there has to be
some other field; like the timestamp Tom proposes.Ahh, right. We should make sure we don't go brain-dead if the system
clock moves backwards. I assume we couldn't just fstat the file...The timestamp field is already there (in my patch). It gets populated
when the
server starts and repopulated by SIGHUP_handler. It's the only timestamp
pg_ctl pays attention to.What happens if someone does a reload sometime in the future (perhaps
because they've messed with the system clock for test purposes). Do we
still detect a new reload?
Good point, and I had that scenario in the back of my head. What I do right
now is read the 'last reload' timestamp from postmaster.pid (which can be the
same as the startup time, since I make postmaster write an initial timestamp
when it starts), send the SIGHUP, and wait until I read a later one or until
timeout. What I could do is wait until I read a *different* one, and not just
a later one. In that case you're only SOL if you manage to time it just so
that your new server time is *exactly* the same as your old server time when
you did your last reload (or startup). But even in that case it would time out
and recommend you check the log.
That whole error message thing has one gotcha BTW; it's not hard, it's just
that I have to find a way to make it bubble up from guc_file.l. Triggering an
error was just changing the return value from void to bool, but returning the
error string would mean returning a char buffer which would then need to be
freed by other callers of ProcessConfig etc etc.
* /me scratches head
But I could just rename the current ProcessConfig, make it return a buffer,
and have a *new*, void, ProcessConfig which calls the renamed function and
just discards the result.
As an aside: I always found it interesting how feature threads "explode"
around here. But it figures if even something as "simple" as this gets such
detailed input. Definitely something to get used to...
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 3/3/15 7:34 PM, Jim Nasby wrote:
Definitely no multi-line. If we keep that restriction, couldn't we just
dedicate one entire line to the error message? ISTM that would be safe.
But we have multiline error messages. If we put only the first line in
the pid file, then all the tools that build on this will effectively
show users truncated information, which won't be a good experience. If
we don't put any error message in there, then users or tools will be
more likely to look up the full message somewhere else.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Peter Eisentraut wrote:
On 3/3/15 7:34 PM, Jim Nasby wrote:
Definitely no multi-line. If we keep that restriction, couldn't we just
dedicate one entire line to the error message? ISTM that would be safe.But we have multiline error messages. If we put only the first line in
the pid file, then all the tools that build on this will effectively
show users truncated information, which won't be a good experience. If
we don't put any error message in there, then users or tools will be
more likely to look up the full message somewhere else.
Would it work to have it be multiline using here-doc syntax? It could
be printed similarly to what psql does to error messages, with a prefix
in front of each component (ERROR, STATEMENT, etc) and a leading tab for
continuation lines. The last line is a terminator that matches
something specified in the first error line. This becomes pretty
complicated for a PID file, mind you.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-03-04 19:04:02 -0300, Alvaro Herrera wrote:
This becomes pretty complicated for a PID file, mind you.
Yes.
Let's get the basic feature (notification of failed reloads) done
first. That will be required with or without including the error
message. Then we can get more fancy later, if somebody really wants to
invest the time.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On March 4, 2015 11:08:09 PM Andres Freund wrote:
Let's get the basic feature (notification of failed reloads) done
first. That will be required with or without including the error
message. Then we can get more fancy later, if somebody really wants to
invest the time.
Except for also fixing pg_reload_conf() to pay attention to what happens with
postmaster.pid, the patch I submitted does exactly what you want I think.
And I don't mind spending time on stuff like this. I'm not smart enough to deal
with actual, you know, database technology.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 3/4/15 7:13 PM, Jan de Visser wrote:
On March 4, 2015 11:08:09 PM Andres Freund wrote:
Let's get the basic feature (notification of failed reloads) done
first. That will be required with or without including the error
message. Then we can get more fancy later, if somebody really wants to
invest the time.Except for also fixing pg_reload_conf() to pay attention to what happens with
postmaster.pid, the patch I submitted does exactly what you want I think.And I don't mind spending time on stuff like this. I'm not smart enough to deal
with actual, you know, database technology.
Yeah, lets at least get this wrapped and we can see about improving it.
I like the idea of doing a here-doc or similar in the .pid, though I
think it'd be sufficient to just prefix all the continuation lines with
a tab. An uglier option would be just striping the newlines out.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I'm trying to review this patch and applied
/messages/by-id/attachment/37123/Let_pg_ctl_check_the_result_of_a_postmaster_config_reload.patch
to postgres. gmake check passed but while starting postgres I see:
[postgres@vagrant-centos65 data]$ LOG: incomplete data in
"postmaster.pid": found only 5 newlines while trying to add line 8
LOG: redirecting log output to logging collector process
HINT: Future log output will appear in directory "pg_log".
Also, a simple syntax error test gave no warning at all on doing a reload,
but just as before there was an error message in the logs:
[postgres@vagrant-centos65 data]$ /usr/local/pgsql/bin/pg_ctl -D
/usr/local/pgsql/data reload
server signaled
[postgres@vagrant-centos65 data]$ cd pg_log
[postgres@vagrant-centos65 pg_log]$ ls
postgresql-2015-04-21_232328.log postgresql-2015-04-21_232858.log
[postgres@vagrant-centos65 pg_log]$ grep error
postgresql-2015-04-21_232858.log
LOG: syntax error in file "/usr/local/pgsql/data/postgresql.conf" line
211, near token "/"
LOG: configuration file "/usr/local/pgsql/data/postgresql.conf" contains
errors; no changes were applied
I'm guessing since this is a patch submitted to the commitfest after the
current one, am I too early to start reviewing it?
Payal
Payal Singh,
Database Administrator,
OmniTI Computer Consulting Inc.
Phone: 240.646.0770 x 253
On Thu, Mar 5, 2015 at 4:06 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
Show quoted text
On 3/4/15 7:13 PM, Jan de Visser wrote:
On March 4, 2015 11:08:09 PM Andres Freund wrote:
Let's get the basic feature (notification of failed reloads) done
first. That will be required with or without including the error
message. Then we can get more fancy later, if somebody really wants to
invest the time.Except for also fixing pg_reload_conf() to pay attention to what happens
with
postmaster.pid, the patch I submitted does exactly what you want I think.And I don't mind spending time on stuff like this. I'm not smart enough
to deal
with actual, you know, database technology.Yeah, lets at least get this wrapped and we can see about improving it.
I like the idea of doing a here-doc or similar in the .pid, though I think
it'd be sufficient to just prefix all the continuation lines with a tab. An
uglier option would be just striping the newlines out.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
(Please don't top post)
On April 21, 2015 07:32:05 PM Payal Singh wrote:
I'm trying to review this patch and applied
/messages/by-id/attachment/37123/Let_pg_ctl_check_the_r
esult_of_a_postmaster_config_reload.patch to postgres. gmake check passed
but while starting postgres I see:[postgres@vagrant-centos65 data]$ LOG: incomplete data in
"postmaster.pid": found only 5 newlines while trying to add line 8
LOG: redirecting log output to logging collector process
HINT: Future log output will appear in directory "pg_log".Also, a simple syntax error test gave no warning at all on doing a reload,
but just as before there was an error message in the logs:[postgres@vagrant-centos65 data]$ /usr/local/pgsql/bin/pg_ctl -D
/usr/local/pgsql/data reload
server signaled
[postgres@vagrant-centos65 data]$ cd pg_log
[postgres@vagrant-centos65 pg_log]$ ls
postgresql-2015-04-21_232328.log postgresql-2015-04-21_232858.log
[postgres@vagrant-centos65 pg_log]$ grep error
postgresql-2015-04-21_232858.log
LOG: syntax error in file "/usr/local/pgsql/data/postgresql.conf" line
211, near token "/"
LOG: configuration file "/usr/local/pgsql/data/postgresql.conf" contains
errors; no changes were appliedI'm guessing since this is a patch submitted to the commitfest after the
current one, am I too early to start reviewing it?Payal
But, but, but... it worked for me... :-)
I'll have a look. I'll apply my patch to a clean tree and see if any bits have
rotted in the last month or so.
One thing to note is that you won't get the actual error; just a message that
reloading failed.
jan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On April 21, 2015 09:01:14 PM Jan de Visser wrote:
On April 21, 2015 07:32:05 PM Payal Singh wrote:
I'm trying to review this patch and applied
/messages/by-id/attachment/37123/Let_pg_ctl_check_the
_r esult_of_a_postmaster_config_reload.patch to postgres. gmake check
passed but while starting postgres I see:[postgres@vagrant-centos65 data]$ LOG: incomplete data in
"postmaster.pid": found only 5 newlines while trying to add line 8
LOG: redirecting log output to logging collector process
HINT: Future log output will appear in directory "pg_log".Also, a simple syntax error test gave no warning at all on doing a reload,
but just as before there was an error message in the logs:[postgres@vagrant-centos65 data]$ /usr/local/pgsql/bin/pg_ctl -D
/usr/local/pgsql/data reload
server signaled
[postgres@vagrant-centos65 data]$ cd pg_log
[postgres@vagrant-centos65 pg_log]$ ls
postgresql-2015-04-21_232328.log postgresql-2015-04-21_232858.log
[postgres@vagrant-centos65 pg_log]$ grep error
postgresql-2015-04-21_232858.log
LOG: syntax error in file "/usr/local/pgsql/data/postgresql.conf" line
211, near token "/"
LOG: configuration file "/usr/local/pgsql/data/postgresql.conf" contains
errors; no changes were appliedI'm guessing since this is a patch submitted to the commitfest after the
current one, am I too early to start reviewing it?Payal
But, but, but... it worked for me... :-)
I'll have a look. I'll apply my patch to a clean tree and see if any bits
have rotted in the last month or so.One thing to note is that you won't get the actual error; just a message
that reloading failed.jan
Urgh. It appears you are right. Will fix.
jan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On April 21, 2015 09:34:51 PM Jan de Visser wrote:
On April 21, 2015 09:01:14 PM Jan de Visser wrote:
On April 21, 2015 07:32:05 PM Payal Singh wrote:
... snip ...
Urgh. It appears you are right. Will fix.
jan
Attached a new attempt. This was one from the category 'I have no idea how
that ever worked", but whatever. For reference, this is how it looks for me
(magic man-behind-the-curtain postgresql.conf editing omitted):
jan@wolverine:~/Projects/postgresql$ initdb -D data
... Bla bla bla ...
jan@wolverine:~/Projects/postgresql$ pg_ctl -D data -l logfile start
server starting
jan@wolverine:~/Projects/postgresql$ tail -5 logfile
LOG: database system was shut down at 2015-04-21 22:03:33 EDT
LOG: database system is ready to accept connections
LOG: autovacuum launcher started
jan@wolverine:~/Projects/postgresql$ pg_ctl -D data reload
server signaled
jan@wolverine:~/Projects/postgresql$ tail -5 logfile
LOG: database system was shut down at 2015-04-21 22:03:33 EDT
LOG: database system is ready to accept connections
LOG: autovacuum launcher started
LOG: received SIGHUP, reloading configuration files
jan@wolverine:~/Projects/postgresql$ pg_ctl -D data reload
server signaled
pg_ctl: Reload of server with PID 14656 FAILED
Consult the server log for details.
jan@wolverine:~/Projects/postgresql$ tail -5 logfile
LOG: autovacuum launcher started
LOG: received SIGHUP, reloading configuration files
LOG: received SIGHUP, reloading configuration files
LOG: syntax error in file "/home/jan/Projects/postgresql/data/postgresql.conf"
line 1, near end of line
LOG: configuration file "/home/jan/Projects/postgresql/data/postgresql.conf"
contains errors; no changes were applied
jan@wolverine:~/Projects/postgresql$
Attachments:
Let_pg_ctl_check_the_result_of_a_postmaster_config_reload.patchtext/x-patch; charset=UTF-8; name=Let_pg_ctl_check_the_result_of_a_postmaster_config_reload.patchDownload
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index a9f20ac..a7819d2 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -1222,6 +1222,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();
@@ -2341,6 +2350,8 @@ static void
SIGHUP_handler(SIGNAL_ARGS)
{
int save_errno = errno;
+ bool reload_success;
+ char last_reload_info[32];
PG_SETMASK(&BlockSig);
@@ -2348,7 +2359,16 @@ SIGHUP_handler(SIGNAL_ARGS)
{
ereport(LOG,
(errmsg("received SIGHUP, reloading configuration files")));
- ProcessConfigFile(PGC_SIGHUP);
+ reload_success = ProcessConfigFile(PGC_SIGHUP);
+
+ /*
+ * 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);
+
SignalChildren(SIGHUP);
SignalUnconnectedWorkers(SIGHUP);
if (StartupPID != 0)
diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l
index c5e0fac..3162cd5 100644
--- a/src/backend/utils/misc/guc-file.l
+++ b/src/backend/utils/misc/guc-file.l
@@ -109,7 +109,7 @@ STRING \'([^'\\\n]|\\.|\'\')*\'
* All options mentioned in the configuration file are set to new values.
* If an error occurs, no values will be changed.
*/
-void
+bool
ProcessConfigFile(GucContext context)
{
bool error = false;
@@ -202,7 +202,7 @@ ProcessConfigFile(GucContext context)
* the config file.
*/
if (head == NULL)
- return;
+ return false;
}
/*
@@ -430,6 +430,7 @@ ProcessConfigFile(GucContext context)
* freed here.
*/
FreeConfigVariables(head);
+ return !error;
}
/*
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 80d7bc7..0ffe97b 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);
@@ -415,6 +431,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
*/
@@ -1093,7 +1181,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 */
@@ -1112,14 +1203,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"));
+ }
}
@@ -2348,6 +2526,7 @@ main(int argc, char **argv)
do_wait = false;
break;
case STOP_COMMAND:
+ case RELOAD_COMMAND:
do_wait = true;
break;
default:
@@ -2358,7 +2537,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 eacfccb..991405f 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -444,6 +444,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 ff78b70..6db7bd7 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -338,7 +338,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);
Ah sorry, didn't realize I top posted. I'll test this new one.
Payal.
On Apr 21, 2015 10:23 PM, "Jan de Visser" <jan@de-visser.net> wrote:
Show quoted text
On April 21, 2015 09:34:51 PM Jan de Visser wrote:
On April 21, 2015 09:01:14 PM Jan de Visser wrote:
On April 21, 2015 07:32:05 PM Payal Singh wrote:
... snip ...
Urgh. It appears you are right. Will fix.
jan
Attached a new attempt. This was one from the category 'I have no idea how
that ever worked", but whatever. For reference, this is how it looks for me
(magic man-behind-the-curtain postgresql.conf editing omitted):jan@wolverine:~/Projects/postgresql$ initdb -D data
... Bla bla bla ...
jan@wolverine:~/Projects/postgresql$ pg_ctl -D data -l logfile start
server starting
jan@wolverine:~/Projects/postgresql$ tail -5 logfile
LOG: database system was shut down at 2015-04-21 22:03:33 EDT
LOG: database system is ready to accept connections
LOG: autovacuum launcher started
jan@wolverine:~/Projects/postgresql$ pg_ctl -D data reload
server signaled
jan@wolverine:~/Projects/postgresql$ tail -5 logfile
LOG: database system was shut down at 2015-04-21 22:03:33 EDT
LOG: database system is ready to accept connections
LOG: autovacuum launcher started
LOG: received SIGHUP, reloading configuration files
jan@wolverine:~/Projects/postgresql$ pg_ctl -D data reload
server signaled
pg_ctl: Reload of server with PID 14656 FAILED
Consult the server log for details.
jan@wolverine:~/Projects/postgresql$ tail -5 logfile
LOG: autovacuum launcher started
LOG: received SIGHUP, reloading configuration files
LOG: received SIGHUP, reloading configuration files
LOG: syntax error in file
"/home/jan/Projects/postgresql/data/postgresql.conf"
line 1, near end of line
LOG: configuration file
"/home/jan/Projects/postgresql/data/postgresql.conf"
contains errors; no changes were applied
jan@wolverine:~/Projects/postgresql$
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.
There is no documentation change so far, but I guess that's not yet necessary.
gmake check passed all tests.
The new status of this patch is: Waiting on Author
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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, failedError 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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 <jan@de-visser.net> wrote:
Show quoted text
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, failedError 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
Attachments:
Let_pg_ctl_check_the_result_of_a_postmaster_config_reload_v3.patchtext/x-patch; charset=US-ASCII; name=Let_pg_ctl_check_the_result_of_a_postmaster_config_reload_v3.patchDownload
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);
On Fri, Jul 3, 2015 at 4:47 PM, I wrote:
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.
Um. Make that config.sgml.
Jan de Visser <jan@de-visser.net> writes:
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.
BTW, it's probably worth pointing out that the recent work on the
pg_file_settings view has taken away a large part of the use-case for
this, in that you can find out more with less risk by inspecting
pg_file_settings before issuing SIGHUP, instead of SIGHUP'ing and
hoping you didn't break anything too nastily. Also, you can use
pg_file_settings remotely, unlike pg_ctl (though admittedly you
still need contrib/adminpack or something to allow uploading a
new config file if you're doing remote admin).
I wonder whether we should consider inventing similar views for
pg_hba.conf and pg_ident.conf.
While that's not necessarily a reason not to adopt this patch anyway,
it does mean that we should think twice about whether we need to add
a couple of hundred lines for a facility that's less useful than
what we already have.
BTW, this version of this patch neglects to update the comments in
miscadmin.h, and it makes the return convention for
ProcessConfigFileInternal completely unintelligible IMO; the inaccuracy
and inconsistency in the comments is a symptom of that. I didn't read it
in enough detail to say whether there are other problems.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On July 3, 2015 06:21:09 PM Tom Lane wrote:
Jan de Visser <jan@de-visser.net> writes:
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.BTW, it's probably worth pointing out that the recent work on the
pg_file_settings view has taken away a large part of the use-case for
this, in that you can find out more with less risk by inspecting
pg_file_settings before issuing SIGHUP, instead of SIGHUP'ing and
hoping you didn't break anything too nastily. Also, you can use
pg_file_settings remotely, unlike pg_ctl (though admittedly you
still need contrib/adminpack or something to allow uploading a
new config file if you're doing remote admin).I wonder whether we should consider inventing similar views for
pg_hba.conf and pg_ident.conf.While that's not necessarily a reason not to adopt this patch anyway,
it does mean that we should think twice about whether we need to add
a couple of hundred lines for a facility that's less useful than
what we already have.
Since you were the one proposing the feature, I'm going to leave it to you
whether or not I should continue with this. I have no use for this feature;
for me it just seemed like a nice bite-sized feature to get myself acquainted
with the code base and the development process. As far as I'm concerned that
goal has already been achieved, even though finalizing a patch towards commit
(and having my name in the release notes ha ha) would be the icing on the
cake.
BTW, this version of this patch neglects to update the comments in
miscadmin.h, and it makes the return convention for
ProcessConfigFileInternal completely unintelligible IMO; the inaccuracy
and inconsistency in the comments is a symptom of that. I didn't read it
in enough detail to say whether there are other problems.
OK, miscadmin.h. I'll go and look what that's all about. And would it make
sense to find a better solution for the ProcessConfigFileInternal return value
(which is convoluted, I agree - I went for the solution with the least impact
on existing code), or should I improve documentation?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On July 3, 2015 09:24:36 PM Jan de Visser wrote:
On July 3, 2015 06:21:09 PM Tom Lane wrote:
BTW, this version of this patch neglects to update the comments in
miscadmin.h, and it makes the return convention for
ProcessConfigFileInternal completely unintelligible IMO; the inaccuracy
and inconsistency in the comments is a symptom of that. I didn't read it
in enough detail to say whether there are other problems.OK, miscadmin.h. I'll go and look what that's all about. And would it make
sense to find a better solution for the ProcessConfigFileInternal return
value (which is convoluted, I agree - I went for the solution with the
least impact on existing code), or should I improve documentation?
Heh. I actually touched that file. I completely missed those comments (or saw
them, thought that I should update them, and then forgot about them - just as
likely). I'll obviously fix this if we carry this to completion.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On July 3, 2015 06:21:09 PM Tom Lane wrote:
I wonder whether we should consider inventing similar views for
pg_hba.conf and pg_ident.conf.
(Apologies for the flurry of emails).
Was there not an attempt at a view for pg_hba.conf which ended in a lot of
bikeshedding and no decisions?
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
Jan de Visser <jan@de-visser.net> writes:
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.BTW, it's probably worth pointing out that the recent work on the
pg_file_settings view has taken away a large part of the use-case for
this, in that you can find out more with less risk by inspecting
pg_file_settings before issuing SIGHUP, instead of SIGHUP'ing and
hoping you didn't break anything too nastily. Also, you can use
pg_file_settings remotely, unlike pg_ctl (though admittedly you
still need contrib/adminpack or something to allow uploading a
new config file if you're doing remote admin).I wonder whether we should consider inventing similar views for
pg_hba.conf and pg_ident.conf.
Yes. That's definitely something that I'd been hoping someone would
work on.
Also, thanks for the work on the pg_file_settings code; I agree with all
you did there.
Thanks again!
Stephen
On July 6, 2015 09:23:12 AM Stephen Frost wrote:
I wonder whether we should consider inventing similar views for
pg_hba.conf and pg_ident.conf.Yes. That's definitely something that I'd been hoping someone would
work on.
There actually is a patch in the current CF that provides a view for pg_hba. I
could look at one for pg_ident, which seems much simpler.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Jan de Visser (jan@de-visser.net) wrote:
On July 6, 2015 09:23:12 AM Stephen Frost wrote:
I wonder whether we should consider inventing similar views for
pg_hba.conf and pg_ident.conf.Yes. That's definitely something that I'd been hoping someone would
work on.There actually is a patch in the current CF that provides a view for pg_hba. I
could look at one for pg_ident, which seems much simpler.
That would be good, as would a review of the patch for pg_hba with
particular consideration of what's been done with the pg_file_settings
view.
Thanks!
Stephen
On 07/04/2015 04:24 AM, Jan de Visser wrote:
On July 3, 2015 06:21:09 PM Tom Lane wrote:
Jan de Visser <jan@de-visser.net> writes:
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.BTW, it's probably worth pointing out that the recent work on the
pg_file_settings view has taken away a large part of the use-case for
this, in that you can find out more with less risk by inspecting
pg_file_settings before issuing SIGHUP, instead of SIGHUP'ing and
hoping you didn't break anything too nastily. Also, you can use
pg_file_settings remotely, unlike pg_ctl (though admittedly you
still need contrib/adminpack or something to allow uploading a
new config file if you're doing remote admin).I wonder whether we should consider inventing similar views for
pg_hba.conf and pg_ident.conf.While that's not necessarily a reason not to adopt this patch anyway,
it does mean that we should think twice about whether we need to add
a couple of hundred lines for a facility that's less useful than
what we already have.Since you were the one proposing the feature, I'm going to leave it to you
whether or not I should continue with this.
It's handy that you can wait for the reload to complete, e.g. "pg_ctl
reload -w; psql ...", without having to put a "sleep 1" in there and
hope for the best. The view is useful too, but it's not the same thing.
This isn't the most exciting feature ever, but seems worthwhile to me.
BTW, this version of this patch neglects to update the comments in
miscadmin.h, and it makes the return convention for
ProcessConfigFileInternal completely unintelligible IMO; the inaccuracy
and inconsistency in the comments is a symptom of that. I didn't read it
in enough detail to say whether there are other problems.OK, miscadmin.h. I'll go and look what that's all about. And would it make
sense to find a better solution for the ProcessConfigFileInternal return value
(which is convoluted, I agree - I went for the solution with the least impact
on existing code), or should I improve documentation?
Both ;-). I'd suggest adding a "bool *success" output parameter to that
function, and explaining it in the comments.
Other comments:
* A timestamp with one second granularity doesn't work if you reload the
config file twice within the same second. Or if the clock moves
backwards. Perhaps use a simple counter instead.
* Parsing the whole file into a struct in get_pidfile_contents() seems
like overkill, when you're only interested in the reload timestamp.
Granted, it might become useful in the future, but let's cross that
bridge when we get there, and keep this patch minimal.
* When "pg_ctl reload -w" returns, indicating that the configuration has
been reloaded, it only means that the postmaster has reloaded. Other
processes might not have. That's OK, but needs to be documented.
* There is no documentation.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jul 23, 2015 at 5:06 PM, Heikki Linnakangas wrote:
Other comments:
[...]
This patch had feedback, but there has been no update in the last
month, so I am now marking it as returned with feedback.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On August 25, 2015 09:31:35 PM Michael Paquier wrote:
On Thu, Jul 23, 2015 at 5:06 PM, Heikki Linnakangas wrote:
Other comments:
[...]This patch had feedback, but there has been no update in the last
month, so I am now marking it as returned with feedback.
It was suggested that this mechanism became superfluous with the inclusion of
the view postgresql.conf (pg_settings?) in 9.5. I left it to Tom (being the
one that suggested the feature) to indicate if he still thinks it's useful.
jan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Jan de Visser <jan@de-visser.net> writes:
On August 25, 2015 09:31:35 PM Michael Paquier wrote:
This patch had feedback, but there has been no update in the last
month, so I am now marking it as returned with feedback.
It was suggested that this mechanism became superfluous with the inclusion of
the view postgresql.conf (pg_settings?) in 9.5. I left it to Tom (being the
one that suggested the feature) to indicate if he still thinks it's useful.
I think there's still a fair argument for "pg_ctl reload" being able to
return a simple yes-or-no result. We had talked about trying to shoehorn
textual error messages into the protocol, and I'm now feeling that that
complication isn't needed, but a bare-bones feature would still be worth
the trouble IMO.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On August 25, 2015 08:36:52 PM Tom Lane wrote:
Jan de Visser <jan@de-visser.net> writes:
On August 25, 2015 09:31:35 PM Michael Paquier wrote:
This patch had feedback, but there has been no update in the last
month, so I am now marking it as returned with feedback.It was suggested that this mechanism became superfluous with the inclusion
of the view postgresql.conf (pg_settings?) in 9.5. I left it to Tom
(being the one that suggested the feature) to indicate if he still thinks
it's useful.I think there's still a fair argument for "pg_ctl reload" being able to
return a simple yes-or-no result. We had talked about trying to shoehorn
textual error messages into the protocol, and I'm now feeling that that
complication isn't needed, but a bare-bones feature would still be worth
the trouble IMO.
OK, I'll have a look at the review comments and submit something updated soon.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers