Re: proposal: a validator for configuration files

On Jun 20, 2011, at 6:22 PM, Florian Pflug wrote:

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

While I agree that making the code potentially less bug prone is a good idea,
I don't agree with the statement that since DEBUG2 output gets rarely turned
on we can make it less usable, hoping that nobody would use in production.

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

I'd say that this issue is a judgement call. Depending on a point of view, both
arguments are valid. I've marked this patch as 'Ready for Committer' w/o
removing the early abort stuff, but if there will be more people willing to remove
them - I'll do that.

Thank you,
Alexey.

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