Re: Re: Proposal for Allow postgresql.conf values to be changed via SQL [review]

On Tuesday, February 12, 2013 12:54 AM Andres Freund wrote:
> On 2013-02-11 11:17:16 -0800, Josh Berkus wrote:
> > On 02/11/2013 06:33 AM, Boszormenyi Zoltan wrote:
> > > 2013-02-11 15:25 keltezéssel, Andres Freund írta:
> > >> On 2013-02-11 15:21:13 +0100, Boszormenyi Zoltan wrote:
> > >>> 2013-01-24 18:02 keltezéssel, Tom Lane írta:
> > >>>> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > >>>>> On 2013-01-24 11:22:52 -0500, Tom Lane wrote:
> > >>>>>> Say again? Surely the temp file is being written by whichever
> > >>>>>> backend
> > >>>>>> is executing SET PERSISTENT, and there could be more than one.
> > >>>>> Sure, but the patch acquires SetPersistentLock exlusively
> beforehand
> > >>>>> which seems fine to me.
> > >>>> Why should we have such a lock? Seems like that will probably
> > >>>> introduce
> > >>>> as many problems as it fixes. Deadlock risk, blockages, etc.
> It is
> > >>>> not
> > >>>> necessary for atomicity, since rename() would be atomic already.
> > >>> There is a problem when running SET PERSISTENT for different GUCs
> > >>> in parallel. All happen to read the same original file, and only
> one
> > >>> setting ends up in the result if you rely only on the rename()
> being
> > >>> atomic.
> > >>> The LWLock provides the serialization for that problem.
> > >> Tom was voting for one-setting-per-file, in that case the problem
> > >> doesn't exist.
> > >
> > > I voted for the one-file approach and was arguing from the POV
> > > of the current implementation.
> >
> > I thought we discussed this ad naseum, and decided to go with the
> > one-single-file approach for the first round, since we already had an
> > implementation for that. I still think that's the right approach to
> take
> > with this patch; if it doesn't work out, we can go do
> > one-file-per-setting in 9.4.
>
> Well, several people (at least Tom, I, and I think Zoltan as well)
> think
> that the one-file approach is considerably more complex.

Zoltan has reviewed this patch very thoroughly, I have never seen a comment
from him that the current
Patch (one-file-all-settings) approach is not as good as having
one-file-per-setting approach.
Zoltan, please correct me, If I am wrong.

What I could understand from mails is Tom has initially suggested to have
one-file-per-setting but for the current patch
(one-file-all-settings) he was telling that if we wanted to go for single
file approach, then there
is no need for separate directory, but for that CRAIG has given a scenario
where separate directory is useful.

Also Robert has suggested from the beginning that (one-file-all-settings) is
better approach.

It took months of discussion to reach the consensus of this level, if we
again want to change approach,
then I think it will be tough to touch this feature again.

I think it would be better if we could try to see if there are any problems
in existing patch which cannot be handled because of it's current design,
then it will make more sense to conclude on changing approach.

> Check
> http://www.postgresql.org/message-
> id/20130126162728(dot)GA5482(at)awork2(dot)anarazel(dot)de
> and related messages for some of the problems. Most of which are
> unhandled in the current patch, i.e. currently you *will* loose changes
> made in concurrent sessions.

How will somebody loose changes in concurrent sessions?
Each session currently waits with LWLock if some other session is operating
on file. Also after having lock they don't acquire any other lock, so there
should be no chances of any other problem.

With Regards,
Amit Kapila.