Re: proposal: a validator for configuration files

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".

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".

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.

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.

best regards,
Florian Pflug