Re: proposal: a validator for configuration files

Florian,

On Jun 16, 2011, at 2:34 PM, Florian Pflug wrote:

> Hi
>
> On May14, 2011, at 00:49 , Alexey Klyukin wrote:
>> The patch forces the parser to report all errors (max 100) from the
>> ProcessConfigFile/ParseConfigFp. Currently, only the first parse error or an
>> invalid directive is reported. Reporting all of them is crucial to automatic
>> validation of postgres config files.
>>
>> This patch is based on the one submitted earlier by Selena Deckelmann:
>> http://archives.postgresql.org/pgsql-hackers/2009-03/msg00345.php
>>
>> It incorporates suggestions by Tom Lane for avoiding excessive bloat in logs
>> in case there is a junk instead of postgresql.conf.
>> http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php
>
> Here's my review of your patch.
>
> The patch is in context diff format and applies cleanly to HEAD. It doesn't
> contain superfluous whitespace changes any more is and quite readable.
>
> First, the behaviour.
>
> 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.

>
> Now to the code.
>
> 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.

>
> You've also introduced a bug in ParseConfigFp(). Previously, if an included
> file contained an error, it did "goto cleanup_exit()" and thus didn't
> ereport() on it's own. With your patch applied it ereport()s a parse error
> at the location of the "include" directive, which seems wrong.

Right, I noticed that I skipped switching the buffers and restoring the Lineno
as well. I'll fix it in the next revision.

>
> Finally, I believe that ParseConfigFp() should make at least some effort to
> resync after hitting a parser error. I suggest that you simply fast-forward
> to the next GUC_EOL token after reporting a parser error.

Makes sense.

Thank you for the review.

I'll post an updated patch, addressing you concerns, shortly.

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