Re: [HACKERS] REVIEW proposal: a validator for configuration files
Alexey Klyukin al...@commandprompt.com writes: Attached is v5. It should fix both problems you've experienced with v4. I've applied this patch after some additional hacking. 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). I thought that the most reasonable solution here was to extend set_config_option to provide a three-valued result: applied, error, or not-applied-for-some-non-error-reason. So I did that, and then ripped out the first set of calls to set_config_option. That should make things a bit faster, as well as eliminating a set of corner cases where the checking call succeeds but then the real apply step fails anyway. I also got rid of the changes to ParseConfigFile's API. I thought those were messy and unnecessary, because there's no good reason not to define the parse-error limit as being so many errors per file. It's really only there to prevent the config file contains War and Peace syndrome anyway. 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] REVIEW proposal: a validator for configuration files
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-46FE1B447447(at)commandprompt(dot)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 Klyukinhttp://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 Alexey, I've included my review procedure outline and output below. Your patch behaves as you described in the email containing 'v5' and judging by Andy's earlier success, I think this is ready to go. I will mark this 'Ready for Committer'. Alexander --- TESTING METHODOLOGY === * Start postgres with stock, valid postgresql.conf * Record output * Stop postgres * Add invalid configuration option to postgresql.conf * Start postgres * Record output * Stop postgres * Add configuration option with bad syntax to postgresql.conf * Start postgres * Record output SETUP - ./configure --prefix=~/builds/pgsql/orig; make install ./configure --prefix=~/builds/pgsql/patched/guc-param-validator; make install cd ~/builds/pgsql/orig; bin/initdb -D data; bin/postgres -D data cd ~/builds/pgsql/patched/guc-param-validator; bin/initdb -D data; bin/postgres -D data OUTPUT -- orig: LOG: database system was shut down at 2011-09-28 20:26:17 PDT LOG: database system is ready to accept connections LOG: autovacuum launcher started # alter postgresql.conf, add bogus param # unknown_config_param = on $ kill -9 `pgrep postgres | head -n1`; bin/postgres -D data FATAL: unrecognized configuration parameter unknown_config_param # alter postgresql.conf, add param with bad syntax # unknown_config_param @@= on $ kill -9 `pgrep postgres | head -n1`; bin/postgres -D data FATAL: syntax error in file /home/av/builds/pgsql/orig/data/postgresql.conf line 156, near token @ patched: LOG: database system was shut down at 2011-09-28 20:12:39 PDT LOG: database system is ready to accept connections LOG: autovacuum launcher started # alter postgresql.conf, add bogus param # unknown_config_param = on $ kill -9 `pgrep postgres | head -n1`; bin/postgres -D data LOG: unrecognized configuration parameter unknown_config_param LOG: unrecognized configuration parameter another_unknown_config_param FATAL: errors detected while parsing configuration files DETAIL: changes were not applied.
Re: [HACKERS] REVIEW proposal: a validator for configuration files
On ons, 2011-09-07 at 10:00 -0400, Tom Lane wrote: There has however been some debate about the exact extent of ignoring bad values during reload --- currently the theory is ignore the whole file if anything is wrong, but there's some support for applying all non-bad values as long as the overall file syntax is okay. That could be a problem if you have some values that depend on each other, and then you change both of them, but because of an error only one gets applied. I think ACID(-like) changes is a feature, also on this level. -- 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] REVIEW proposal: a validator for configuration files
On Sep 12, 2011, at 10:24 PM, Peter Eisentraut wrote: On ons, 2011-09-07 at 10:00 -0400, Tom Lane wrote: There has however been some debate about the exact extent of ignoring bad values during reload --- currently the theory is ignore the whole file if anything is wrong, but there's some support for applying all non-bad values as long as the overall file syntax is okay. That could be a problem if you have some values that depend on each other, and then you change both of them, but because of an error only one gets applied. I think ACID(-like) changes is a feature, also on this level. I think exactly this argument has already been discussed earlier in this thread: http://archives.postgresql.org/message-id/21310d95-eb8d-4b15-a8bc-0f05505c6...@phlo.org -- Alexey Klyukinhttp://www.commandprompt.com The PostgreSQL Company – Command Prompt, Inc. -- 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] REVIEW proposal: a validator for configuration files
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 to 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 Klyukinhttp://www.commandprompt.com The PostgreSQL Company – Command Prompt, Inc. pg_parser_continue_on_error_v5.diff Description: Binary data -- 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] REVIEW proposal: a validator for configuration files
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 Klyukinhttp://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
Re: [HACKERS] REVIEW proposal: a validator for configuration files
Hello,
On Sep 7, 2011, at 5:00 PM, Tom Lane wrote:
Andy Colson a...@squeakycode.net writes:
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?
The historical behavior is that a configuration file error detected
during postmaster startup should prevent the server from starting, but
an error detected during reload should only result in a LOG message and
the reload not occurring. I don't believe anyone will accept a patch
that causes the server to quit on a failed reload. There has however
been some debate about the exact extent of ignoring bad values during
reload --- currently the theory is ignore the whole file if anything is
wrong, but there's some support for applying all non-bad values as long
as the overall file syntax is okay.
This patch actually aims to do the latter, applying all good values and
reporting the bad ones. It removes the calls to set_config_option with
changeVal = false from ProcessConfigFile, trying to apply every option at once
and incrementing the warnings counter if set_config_option returns false. After
some investigation I've discovered that set_config_option returns false even if
a variable value is unchanged, but is applied in the wrong GucContext. The
particular case is trying to reload the configuration file variables during
SIGHUP: if, say, shared_buffers are commented out, the call to
set_config_option with changeVal = true will return false due to this code:
if (prohibitValueChange)
{
if (*conf-variable != newval)
ereport(elevel,
(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
errmsg(parameter \%s\ cannot be changed without restarting the server,
name)));
return false;
}
Due to the above, set_config_option returns false for unchanged PGC_POSTMASTER
variables during SIGHUP, so it's impossible to distinguish between valid and
non valid values and report the latter ones with a single call of this function
per var. What do you think about changing the code above to return true if the
variable is actually unchanged?
This explains the WARNINGs emitted during reload even for a pristine
configuration file right after the installation. I'm looking into why the
server gets killed during reload if there is a parse error, it might be as well
related to the problem above.
--
Alexey Klyukinhttp://www.commandprompt.com
The PostgreSQL Company – Command Prompt, Inc.
--
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] REVIEW proposal: a validator for configuration files
Andy Colson a...@squeakycode.net writes: 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? The historical behavior is that a configuration file error detected during postmaster startup should prevent the server from starting, but an error detected during reload should only result in a LOG message and the reload not occurring. I don't believe anyone will accept a patch that causes the server to quit on a failed reload. There has however been some debate about the exact extent of ignoring bad values during reload --- currently the theory is ignore the whole file if anything is wrong, but there's some support for applying all non-bad values as long as the overall file syntax is okay. 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] REVIEW proposal: a validator for configuration files
Hi Alexey, I was taking a quick look at this patch, and have a question for ya. I have a default config from initdb, there is a new setting at the end but its commented out. root@storm: /db/pg92 # /etc/rc.d/postgresql start Starting PostgreSQL: root@storm: /db/pg92 # more serverlog LOG: database system was shut down at 2011-09-06 22:30:17 CDT LOG: database system is ready to accept connections LOG: autovacuum launcher started root@storm: /db/pg92 # /etc/rc.d/postgresql reload Reload PostgreSQL: No directory, logging in with HOME=/ root@storm: /db/pg92 # more serverlog LOG: database system was shut down at 2011-09-06 22:30:17 CDT LOG: database system is ready to accept connections LOG: autovacuum launcher started LOG: received SIGHUP, reloading configuration files WARNING: errors detected while parsing configuration files WARNING: errors detected while parsing configuration files WARNING: errors detected while parsing configuration files WARNING: errors detected while parsing configuration files WARNING: errors detected while parsing configuration files I didn't edit the config, it was fine at startup, so why does reload upset it so? Also, what line is the warning for? If I edit postgresql.conf and just add bob at the last line, then reload: LOG: received SIGHUP, reloading configuration files LOG: syntax error in file /db/pg92/postgresql.conf line 570, near end of line FATAL: errors detected while parsing configuration files 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? -Andy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
