Re: proposal: a validator for configuration files

On Jun16, 2011, at 18:46 , Alexey Klyukin wrote:
> On Jun 16, 2011, at 6:49 PM, Florian Pflug wrote:
>> 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.

Ah, yeah, you're right. PGC_S_FILE sounds fine, then. I guess this means you can
drop the check for "context == PGC_SIGHUP" though, because surely the source must
be PGC_S_DEFAULT or PGC_S_FILE if context == PGC_SIGHUP, right? So the check would
become
if (source == PGC_S_FILE || source == PGC_S_DEFAULT)
where it now says
if (context == PGC_SIGHUP || source == PGC_S_DEFAULT)

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

Ah, right, I missed that. Guess it'll have to stay a counter, then. Still, I don't think it's
worth the effort to make the count correct in case of included files, so I'd say just add
a comment explaining that the count isn't totally accurate.

best regards,
Florian Pflug