Re: proposal: a validator for configuration files

On Jun16, 2011, at 22:34 , Alexey Klyukin wrote:
> Attached is the v2 of the patch to show all parse errors at postgresql.conf.
> Changes (per review and suggestions from Florian):
>
> - do not stop on the first error during postmaster's start.
> - fix errors in processing files from include directives.
> - show only a single syntax error per line, i.e. fast forward to the EOL after coming across the first one.
> - additional comments/error messages, code cleanup

Looks mostly good.

I found one issue which I missed earlier. As it stands, the behaviour
of ProcessConfigFile() changes subtly when IsUnderPostmaster because of
the early abort on errors. Now, in theory the outcome should still be
the same, since we don't apply any settings if there's an error in one
of them. But still, there's a risk that this code isn't 100% waterproof,
and then we'd end up with different settings in the postmaster compared
to the backends. The benefit seems also quite small - since the backends
emit their messages at DEBUG2, you usually won't see the difference
anyway.

The elevel setting at the start of ProcessConfigFile() also seemed
needlessly complex, since we cannot have IsUnderPostmaster and
context == PGCS_POSTMASTER at the same time.

I figured it'd be harder to explain the fixes I have in mind than
simply do them and let the code speak. Attached you'll find an updated
version of your v2 patch (called v2a) as well as an incremental patch
against your v2 (called v2a_delta).

The main changes are the removal of the early aborts when
IsUnderPostmaster and the simplification of the elevel setting
logic in ProcessConfigFile().

The updated patch also adds a few comments. If you agree with my changes,
feel free to mark this patch "Ready for Committer".

best regards,
Florian Pflug