Re: 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
havenot gotten through the history of messages regarding this patch, but is it supposed to kill the server if there is
asyntax 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.
Therewere 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
getsinterrupted on any error detected in the configuration file. Unlike the current code, the patch tries to report all
ofthem before bailing out. The behavior during configuration reload has changed significantly. Instead of ignoring all
changesafter the first error, the code  reports the problematic value and continues. It only skips applying new values
completelyon 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
toverify the new value and another to actually apply it. Normally, this function returns true for a valid value and
falseif it is unable to set the new value for whatever reason (either the value is invalid, or the value cannot be
changedin the context of the caller). However, calling it once per value in the 'apply' mode during reload produces
falsefor every unchanged value that can only be changed during startup (i.e. shared_buffers, or max_connections). If we
ignoreits return value, we'll lose the ability to detect whether invalid values were encountered during the reload and
reportthis 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
tohear 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
theset_config_option, I'll fix this by removing the first, verification call and final 'errors were detected' warning
toavoid '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
thecommitfest 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.

# alter postgresql.conf, add param with bad syntax
# unknown_config_param @@= on
$ kill -9 `pgrep postgres | head -n1`; bin/postgres -D data

LOG:  syntax error in file
"/home/av/builds/pgsql/patched/data/postgresql.conf" line 156, near
token "@"
LOG:  syntax error in file
"/home/av/builds/pgsql/patched/data/postgresql.conf" line 575, near
token "@"
FATAL:  errors detected while parsing configuration files
DETAIL:  changes were not applied.