[Ur] Code review request

[Adam Chlipala]

 From the department of better-late-than-never (sorry!), here's my 
response to this thread.

On 04/06/2014 04:45 PM, Sergey Mironov wrote:
> - Sometimes the Demo.exe starts successfully (~ 5% chance with 10
> callback threads). In this case feeding it with tasks results in
> random 'Serialization failure's reported from the callback threads.

"Serialization failure" is not a fatal error message.  In fact, you 
should expect to see it regularly in applications with high concurrency 
and many SQL writes.  It just means that transactions are being 
restarted to maintain the ACID semantics.

> I've run a stress testing and it shows that some database transactions
> just disappear . Investigation shows an interesting point in uw_commit
> (see the code below). The code says that it is possible to return from
> the [uw_commit] function after calling all the
> transactionals[i].commit handlers (return 1 after db_commit). I think
> it is clearly a bug, since the user applications (like urweb-callback)
> think that everything is OK at this point. I've done some testing and
> it shows that this returns really happen from time to time under high
> load.

I agree.  It was a bug to forget to clean up transactionals (and some 
other state) on that code path, which corresponds to a serialization 
failure from the database engine.

> So there are two questions:
>
>   1. What should application do if database commit fails? Probably, the
> answer is - report the error to client.

But not for a serialization failure, in which case the appropriate 
action is restarting the transaction.  The (non-FFI) programmer 
shouldn't even have to think about such things going on under the hood, 
so it wouldn't be appropriate to set an error message.

>   2. How to notify the ctx->transactionals about successful db_commit?
> Looks like we can't do that at the moment.  Here I suggest rewriting
> the code: call NULL-rolllbacked commits_after_  the db_commit point
> (and forbid raising errors for such handlers). This way we can really
> handle the non-transactional actions.

I've implemented your suggestion, in a patch of my own with a few 
differences from yours.