Re: proposal: a validator for configuration files - Mailing list pgsql-hackers

Alvaro Herrera <alvherre@commandprompt.com> writes:
> Excerpts from Alexey Kluykin's message of jue jul 14 09:18:15 -0400 2011:
>> This is happening because a check for total number of errors so far
>> is happening only after coming across at least one non-recognized
>> configuration option.  What about adding one more check right after
>> ParseConfigFile, so we can bail out early when overwhelmed with
>> syntax errors? This would save a line in translation :).

> Actually I think it would make sense to do it completely the other way
> around: reset the number of errors to zero before starting to apply the
> values.  The rationale is that all the settings that made it past the
> tokenisation are completely different lines for which the errors were
> reported earlier.

I'd like to re-open the discussion from here
http://archives.postgresql.org/pgsql-hackers/2011-06/msg01741.php
http://archives.postgresql.org/pgsql-hackers/2011-04/msg00330.php
specifically about this concern:

>>> Does that mean that some backends might currently choose to ignore an
>>> updated config file wholesale on SIGUP (because some settings are invalid)
>>> while others happily apply it? Meaning that they'll afterwards disagree
>>> even on modified settings which *would* be valid for both backends?

I complained about exactly that point back in April, but at the time
people (quite reasonably) didn't want to touch the behavior.  Now that
we are entering a new development cycle, it's time to reconsider.
Particularly so if we're considering a patch that touches the behavior
already.

I think that it might be sensible to have the following behavior:

1. Parse the file, where "parse" means collect all the name = value
pairs.  Bail out if we find any syntax errors at that level of detail.
(With this patch, we could report some or all of the syntax errors
first.)

2. Tentatively apply the new custom_variable_classes setting if any.

3. Check to see whether all the "name"s are valid.  If not, report
the ones that aren't and bail out.

4. Apply each "value".  If some of them aren't valid, report that,
but continue, and apply all the ones that are valid.

We can expect that the postmaster and all backends will agree on the
results of steps 1 through 3.  They might differ as to the validity
of individual values in step 4 (as per my example of a setting that
depends on database_encoding), but we should never end up with a
situation where a globally correct value is not globally applied.

The original argument for the current behavior was to avoid applying
settings from a thoroughly munged config file, but I think that the
checks involved in steps 1-3 would be sufficient to reject files that
had major problems.  It's possible that step 1 is really sufficient to
cover the issue, in which case we could drop the separate step-3 pass
and just treat invalid GUC names as a reason to ignore the particular
line rather than the whole file.  That would make things simpler and
faster, and maybe less surprising too.

Comments?
        regards, tom lane