On 2013-01-24 12:02:59 -0500, Tom Lane wrote: > 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.
Well, the patch acquires it as-is, so I made the judgement based on that. But I think that lock isn't neccesarily a bad idea. While the replacement of the values clearly is atomic due to the rename I think there's another confusing behaviour lurking: Backend A: starts Backend B: starts Backend A: reads the config Backend B: reads the config Backend A: does SET PERSISTENT foobar =..; Backend B: does SET PERSISTENT foobar =..; Now B overwrites the config change A has made as they are all stored in the same file. So the only safe way to do this seems to be: LWLockAcquire(SetPersistentLock, LW_EXCLUSIVE); ProcessConfigFile(); validate_option(); write_rename(); LWLockRelease(); We can decide not to care about the above case but the window isn't that small if no reload is implied and so this seems to be overly grotty. > > Any opinion whether its acceptable to allow SET PERSISTENT in functions? > > It seems absurd to me to allow it, but Amit seems to be of another > > opinion. > > Well, it's really a definitional question I think: do you expect that > subsequent failure of the transaction should cause such a SET to roll > back? > > I think it would be entirely self-consistent to define SET PERSISTENT as > a nontransactional operation. Then the implementation would just be to > write the file immediately when the command is executed, and there's no > particular reason why it can't be allowed inside a transaction block. Thats how its implemented atm except for not allowing it in transactions. I think the reason I am weary of allowing it inside transaction is that I think the config file needs to be reloaded before writing the new one and it seems dangerous to me to reload the config in all the possible situations a function can be called. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
