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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
