On 2013-01-24 12:02:59 -0500, Tom Lane wrote:
> Andres Freund <andres@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