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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
