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.
> 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.
If you want it to be transactional, then the point of disallowing it in
transaction blocks (or functions) would be to not have a very large
window between writing the file and committing. But it's still possible
that the transaction would fail somewhere in there, leading to the
inconsistent outcome that the transaction reports failing but we applied
the SET anyway. I do agree that it would be nonsensical to allow SET
PERSISTENT in functions but not transaction blocks.
Another approach is to remember the requested setting until somewhere in
the pre-commit sequence, and then try to do the file write at that time.
I'm not terribly thrilled with that approach, though, because (a) it
only narrows the window for an inconsistent outcome, it doesn't remove
it entirely; (b) there are already too many things that want to be the
"last thing done before commit"; and (c) it adds complexity and overhead
that I'd just as soon this patch not add. But if you want S.P. to be
transactional and allowed inside functions, I think this would be the
only acceptable implementation.
regards, tom lane