Re: [HACKERS] Idea: closing the loop for pg_ctl reload
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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
Jan de Visser j...@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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
On August 25, 2015 08:36:52 PM Tom Lane wrote: Jan de Visser j...@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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
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 j...@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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
* Tom Lane (t...@sss.pgh.pa.us) wrote: Jan de Visser j...@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 signature.asc Description: Digital signature
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
* Jan de Visser (j...@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 signature.asc Description: Digital signature
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
Jan de Visser j...@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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
On July 3, 2015 06:21:09 PM Tom Lane wrote: Jan de Visser j...@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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
Attached a new patch, rebased against the current head. Errors in
pg_hba.conf and pg_ident.conf are now also noticed.
I checked the documentation for pg_ctl reload, and the only place where
it's explained seems to be runtime.sgml and that description is so
high-level that adding this new bit of functionality wouldn't make much
sense.
On Sat, Apr 25, 2015 at 10:03 AM, Jan de Visser j...@de-visser.net wrote:
On April 22, 2015 06:04:42 PM Payal Singh wrote:
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: tested, failed
Spec compliant: not tested
Documentation:tested, failed
Error in postgresql.conf gives the expected result on pg_ctl reload,
although errors in pg_hba.conf file don't. Like before, reload completes
fine without any information that pg_hba failed to load and only
information is present in the log file. I'm assuming pg_ctl reload should
prompt user if file fails to load irrespective of which file it is -
postgresql.conf or pg_hba.conf.
Will fix. Not hard, just move the code that writes the .pid file to after
the
pg_hba reload.
There is no documentation change so far, but I guess that's not yet
necessary.
I will update the page for pg_ctl. At least the behaviour of -w/-W in
relation
to the reload command needs explained.
gmake check passed all tests.
The new status of this patch is: Waiting on Author
jan
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index df8037b..909a078 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -1235,6 +1235,15 @@ PostmasterMain(int argc, char *argv[])
#endif
/*
+ * Update postmaster.pid with startup time as the last reload time:
+ */
+ {
+ char last_reload_info[32];
+ snprintf(last_reload_info, 32, %ld %d, (long) MyStartTime, 1);
+ AddToDataDirLockFile(LOCK_FILE_LINE_LAST_RELOAD, last_reload_info);
+ }
+
+ /*
* Remember postmaster startup time
*/
PgStartTime = GetCurrentTimestamp();
@@ -2349,6 +2358,8 @@ static void
SIGHUP_handler(SIGNAL_ARGS)
{
int save_errno = errno;
+ boolreload_success;
+ charlast_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:
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
On April 22, 2015 06:04:42 PM Payal Singh wrote: The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: tested, failed Spec compliant: not tested Documentation:tested, failed Error in postgresql.conf gives the expected result on pg_ctl reload, although errors in pg_hba.conf file don't. Like before, reload completes fine without any information that pg_hba failed to load and only information is present in the log file. I'm assuming pg_ctl reload should prompt user if file fails to load irrespective of which file it is - postgresql.conf or pg_hba.conf. Will fix. Not hard, just move the code that writes the .pid file to after the pg_hba reload. There is no documentation change so far, but I guess that's not yet necessary. I will update the page for pg_ctl. At least the behaviour of -w/-W in relation to the reload command needs explained. gmake check passed all tests. The new status of this patch is: Waiting on Author jan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
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 j...@de-visser.net wrote: 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$
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
I'm trying to review this patch and applied http://www.postgresql.org/message-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.na...@bluetreble.com wrote: 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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
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 Herrerahttp://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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
On 2015-03-03 15:21:24 +, 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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
Andres Freund and...@2ndquadrant.com writes: On 2015-03-03 15:21:24 +, 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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
On Fri, Feb 20, 2015 at 1:26 AM, Tom Lane t...@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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
On 3/3/15 9:26 AM, Andres Freund wrote: On 2015-03-03 15:21:24 +, 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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
On March 3, 2015 10:29:43 AM Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2015-03-03 15:21:24 +, 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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
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 +, 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)? 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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
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 +, 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)? 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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
Jim Nasby jim.na...@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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
On 3/3/15 5:13 PM, Tom Lane wrote: Jim Nasby jim.na...@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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
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 +, 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)? 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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
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 +, 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)? 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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
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.*** 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;
+ boolreload_success;
+ charlast_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_tpid;
+ char *datadir;
+ time_t startup_ts;
+ intport;
+ char *socketdir;
+ char *listenaddr;
+ unsigned long shmkey;
+ intshmid;
+ 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);
+
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
Jan de Visser j...@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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
Jan de Visser j...@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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
Jan de Visser j...@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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
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
*** 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;
+ boolreload_success;
+ charlast_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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
On Mon, Mar 2, 2015 at 3:01 PM, Jan de Visser j...@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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
Jan de Visser j...@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
[HACKERS] 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
Re: [HACKERS] Idea: closing the loop for pg_ctl reload
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
