Re: proposal: a validator for configuration files

Florian,

On Jun 18, 2011, at 5:40 PM, Florian Pflug wrote:

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

I don't think it has changed at all. Previously, we did goto cleanup_list (or
cleanup_exit in ParseConfigFp) right after the first error, no matter whether
that was a postmaster or its child. What I did in my patch is removing the
goto for the postmaster's case. It was my intention to exit after the initial
error for the postmaster's child, to avoid complaining about all errors both
in the postmaster and in the normal backend (imagine seeing 100 errors from
the postmaster and the same 100 from each of the backends if your log level is
DEBUG2). I think the postmaster's child case won't cause any problems, since
we do exactly what we used to do before.

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

Agreed.

>
> 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().

Attached is the new patch and the delta. It includes some of the changes from
your patch, while leaving the early abort stuff for postmaster's children.

Thank you,
Alexey.

--
Command Prompt, Inc. http://www.CommandPrompt.com
PostgreSQL Replication, Consulting, Custom Development, 24x7 support