Re: REVIEW proposal: a validator for configuration files

Hello,

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

> Andy Colson <andy(at)squeakycode(dot)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 Klyukin http://www.commandprompt.com
The PostgreSQL Company – Command Prompt, Inc.