On 09/10/2011 11:39 AM, Alexey Klyukin wrote:
Hi Andy,
On Sep 7, 2011, at 6:40 AM, Andy Colson wrote:
Hi Alexey, I was taking a quick look at this patch, and have a question for ya.
...
Where did the other warnings go? Its right though, line 570 is bad. It also
seems to have killed the server. I have not gotten through the history of
messages regarding this patch, but is it supposed to kill the server if there
is a syntax error in the config file?
Thank you for looking at this patch. v4 was more a "what if" concept that took
a lot of time for somebody to look at. There were a lot of problems with it, but I think
I've nailed down most of them.
Attached is v5. It should fix both problems you've experienced with v4. As with
the current code, the startup process gets interrupted on any error detected in
the configuration file. Unlike the current code, the patch tries to report all
of them before bailing out. The behavior during configuration reload has
changed significantly. Instead of ignoring all changes after the first error,
the code reports the problematic value and continues. It only skips applying
new values completely on syntax errors and invalid configuration option names.
In no cases should it bring the server down during reload.
One problem I'm not sure how to address is the fact that we require 2 calls of
set_config_option for each option, one to verify the new value and another to
actually apply it. Normally, this function returns true for a valid value and
false if it is unable to set the new value for whatever reason (either the
value is invalid, or the value cannot be changed in the context of the caller).
However, calling it once per value in the 'apply' mode during reload produces
false for every unchanged value that can only be changed during startup (i.e.
shared_buffers, or max_connections). If we ignore its return value, we'll lose
the ability to detect whether invalid values were encountered during the reload
and report this fact at the end of the function. I think the function should be
changed, as described in my previous email
(http://archives.postgresql.org/message-id/97a66029-9d3e-43af-95aa-46fe1b447...@commandprompt.com)
and I'd like to hear other opinions on that. Meanwhile, due t
o 2 calls to set_config_option, it currently reports all invalid values twice.
If others will be opposed to changing the set_config_option, I'll fix this by
removing the first, verification call and final 'errors were detected' warning
to avoid 'false positives' on that (i.e. the WARNING you saw with the previous
version for the valid .conf).
I'd appreciate your further comments and suggestions.
Thank you.
--
Alexey Klyukin http://www.commandprompt.com
The PostgreSQL Company – Command Prompt, Inc.
After a quick two minute test, this patch seems to work well. It does just
what I think it should. I'll add it to the commitfest page for ya.
-Andy
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
