Re: proposal: a validator for configuration files

On Jun20, 2011, at 17:02 , Alexey Klyukin wrote:
> 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.

Hm, I think you miss-understood what I was trying to say, probably because I
explained it badly. Let me try again.

I fully agree that there *shouldn't* be any difference in behaviour, because
it *shouldn't* matter whether we abort early or not - we won't have applied
any of the settings anway.

But.

The code the actually implements the "check settings first, apply later" logic
isn't easy to read. Now, assume that this code has a bug. Then, with your
patch applied, we might end up with the postmaster applying a setting (because
it didn't abort early) but the backend ignoring it (because they did abort early).
This is obviously bad. Depending on the setting, the consequences may range
from slightly confusing behaviour to outright crashes I guess...

Now, the risk of that happening might be very small. But on the other hand,
the benefit is pretty small also - you get a little less output for log level
DEBUG2, that's it. A level that people probably don't use for the production
databases anyway. This convinced me that the risk/benefit ratio isn't high enough
to warrant the early abort.

Another benefit of removing the check is that it reduces code complexity. Maybe
not when measured in line counts, but it's one less outside factor that changes
ProcessConfigFiles()'s behaviour and thus one thing less you need to think when
you modify that part again in the future. Again, it's a small benefit, but IMHO
it still outweights the benefit.

Having said that, this is my personal opinion and whoever will eventually
commit this may very will assess the cost/benefit ratio differently. So, if
after this more detailed explanations of my reasoning, you still feel that
it makes sense to keep the early abort, then feel free to mark the
patch "Ready for Committer" nevertheless.

best regards,
Florian Pflug