Re: Idea: closing the loop for "pg_ctl reload"
Re: Idea: closing the loop for "pg_ctl reload"
От:
Jan de Visser <jan@de-visser.net>
Дата:
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: Idea: closing the loop for "pg_ctl reload"
От:
Jan de Visser <jan@de-visser.net>
Дата:
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
Re: Idea: closing the loop for "pg_ctl reload"
От:
Jan de Visser <jan@de-visser.net>
Дата:
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.
Re: Idea: closing the loop for "pg_ctl reload"
От:
Jan de Visser <jan@de-visser.net>
Дата:
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.
Re: Idea: closing the loop for "pg_ctl reload"
От:
Jan de Visser <jan@de-visser.net>
Дата:
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:
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.jan
>
> gmake check passed all tests.
>
> The new status of this patch is: Waiting on Author
Re: Idea: closing the loop for "pg_ctl reload"
От:
Payal Singh <payal@omniti.com>
Дата:
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:
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: Idea: closing the loop for "pg_ctl reload"
От:
Payal Singh <payal@omniti.com>
Дата:
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
On Thu, Mar 5, 2015 at 4:06 PM, Jim Nasby <Jim.Nasby@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
