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

On Tuesday, February 12, 2013 11:24 AM Boszormenyi Zoltan wrote:
> 2013-02-12 04:54 keltezéssel, Amit Kapila írta:
> > 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.
>
> I cannot recall arguing for the one-file-per-GUC way.
> But I haven't re-read my mails in this thread, either.

Thanks for confirmation. I also don't think if ever have argument over this
approach.

> > 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.
>
> This mail lists this order for the single file approach:
>
> > 1) exlusive lock
> > 2) reload config file (to update in memory structures)
> > 3) check new variable
> > 4) write config file (needs to be done atomically)
> > 5) optionally reload config file
> > 6) reload lock
>
> The patch does it this way:
> 1. check new variable. No problem with that, validation for proper GUC
> name,
> type of the value, etc. can be done outside the lock.
> 2. grab lock
> 3. parse the config file
> 4. write the new config file
> 5. release lock

Before step-3, we open the auto conf file and in step-3 parse will update
in-memory structures.
So I think Andres's Step-2 is same as the missing step and Step-3 mentioned
by you.

> Reloading the config file is intentionally not done, it's even
> documented.
> You can do SELECT pg_reload_conf() after SET PERSISTENT if you need it.
>
> > 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.
>
> Specifically, LWLockAcquire() is called first, then ParseConfigFp()
> in a PG_TRY() block, so reading the original postgresql.auto.conf
> is serialized. No chance to lose changes done in parallel.

True.

With Regards,
Amit Kapila.