Re: REVIEW proposal: a validator for configuration files - Mailing list pgsql-hackers

From Alexey Klyukin
Subject Re: REVIEW proposal: a validator for configuration files
Date
Msg-id 97A66029-9D3E-43AF-95AA-46FE1B447447@commandprompt.com
Whole thread Raw
In response to Re: REVIEW proposal: a validator for configuration files  (Tom Lane <tgl@sss.pgh.pa.us>)
List pgsql-hackers
Hello,

On Sep 7, 2011, at 5:00 PM, Tom Lane wrote:

> Andy Colson <andy@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
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? 
>
> 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_optionwith changeVal = false from ProcessConfigFile, trying to apply every option at once and incrementing
thewarnings counter if set_config_option returns false. After some investigation I've discovered that set_config_option
returnsfalse even if a variable value is unchanged, but is applied in the wrong GucContext. The particular case is
tryingto reload the configuration file variables during SIGHUP: if, say, shared_buffers are commented out, the call to
set_config_optionwith 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
impossibleto distinguish between valid and non valid values and report the latter ones with a single call of this
functionper 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'mlooking into why the server gets killed during reload if there is a parse error, it might be as well related to the
problemabove. 

--
Alexey Klyukin        http://www.commandprompt.com
The PostgreSQL Company – Command Prompt, Inc.






pgsql-hackers by date:

Previous
From: Tom Lane
Date:
Subject: Re: Patch to improve reliability of postgresql on linux nfs
Next
From: Simon Riggs
Date:
Subject: Re: unite recovery.conf and postgresql.conf