Re: proposal: a validator for configuration files

On Jun 16, 2011, at 6:49 PM, Florian Pflug wrote:

> On Jun16, 2011, at 17:23 , Alexey Klyukin wrote:
>> On Jun 16, 2011, at 2:34 PM, Florian Pflug wrote:
>>> The first problem I ran into when I tried to test this is that it *only*
>>> reports multiple errors during config file reload on SIHUP, not during
>>> postmaster startup. I guess it's been done that way because we
>>> ereport(ERROR,..) not ereport(LOG,...) during postmaster startup, so it's
>>> not immediatly clear how to report multiple errors. But that proplem
>>> seems solvable. What if you ereport(LOG,..)ed the individual errors during
>>> postmaster startup, and then emitted an ereport(ERROR) at the end if
>>> errors occurred? The ERROR could either repeat the first error that was
>>> encountered, or simply say "config file contains errors".
>>
>> Makes sense. One problem I came across is that set_config_option from guc.c
>> sets the elevel by itself. I've changed it to emit LOG errors on PGC_S_FILE
>> source, apparently all the callers of this function with this source are from
>> guc-file.l, so hopefully I won't break anything with this change.
>
> Hm, wouldn't a test for "context == PGC_POSTMASTER" be more appropriate?

In such a case the errors caused by command-line arguments won't stop the postmaster.
PGC_S_FILE seems to handle this correctly. I'm not sure whether it is appropriate to use
there though.

>
>>> I see that you basically replaced "goto cleanup..." in both ParseConfigFp()
>>> and ProcessConfigFile() with "++errorcount", and arranged for ParseConfigFp()
>>> to return false, and for ProcessConfigFile() to skip the GUC updates if
>>> "errorcount > 0". The actual value of errorcount is never inspected. The value
>>> is also wrong in the case of include files containing more than error, since
>>> ParseConfigFp() simply increments errorcount by one for each failed
>>> ParseConfigFile() of an included file.
>>>
>>> I thus suggest that you replace "errorcount" with a boolean "erroroccurred".
>>
>> I can actually pass errorcount down from the ParseConfigFp() to report the total
>> number of errors (w/ the include files) at the end of ProcessConfigFile if there is
>> any interest in having the number of errors reported to a user. If not - I'll change
>> it to boolean.
>
> Nah, just use a boolean, unless you have concrete plans to actually use the errorcount
> for something other than test a la "errorcount > 0".

I just recalled a reason for counting the total number of errors. There is a condition that
checks that the total number of errors is less than 100 and bails out if it's more than that
(100 is arbitrary). The reason is to avoid bloating the logs w/ something totally unrelated
to postgresql.conf. That was suggested by Tom Lane here:
http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php

Thank you,
Alexey.

--
Command Prompt, Inc. http://www.CommandPrompt.com
PostgreSQL Replication, Consulting, Custom Development, 24x7 support