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

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.

> 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

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.

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/