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