On Sun, Jan 06, 2013 at 10:41:19PM +0000, John Rouillard wrote:
> 
> In message <1357499750.81.0.443026588007.issue2550731@psf.upfronthosting.co.za>
>  <1357499750.81.0.443026588007.issue2550731@psf.upfronthosting.co.za>,
> Bernhard Reiter writes:
> >Bernhard Reiter added the comment:
> >Yes,I believe the observed race condition is to blame for at least a part
> >of the problem. If this hypothesis holds, then if would mean this is
> >independent from your patch, though. We'e probably should bring it
> >up in a different issue and find a way to demonstrate it with a
> >regular version of roundup.
> 
> Agreed. I think interleving two commits that change the same fiend in
> the same issue with a mysql/postgresql back would do it.

I don't think this is true, there are different strategies for
RDBMSes how they handle concurrency:
- Use locking and don't allow concurrent updates
  (done by mysql and sqlite)
- Allow concurrent updates and roll back one of the transactions
  if updates collide (done by PostgreSQL)

Now if we would decide to do locking in userspace this would defeat the
whole database implementation.

Concurrent updates *can* be a problem if we are using side-effects (e.g.
sending email) before we know that a transaction will succeed.

AFAIK roundup currently runs the code of the auditors in the same
transaction as the reactors and if a reactor fails the whole thing
is rolled back. This can be a problem because side-effects are not
rolled back. I'm open for doing a commit after the auditors and run the
reactors in a separate transaction. This would still not solve
side-effect issues, though.

> Actually it looks like back_mysql deals only with the case where a new
> object/node is created. An example race is:
> 
>   tx 1 checks to see if an issue with the new issue name/handle is present
>     in the db. The handle is available.
>   tx 2 checks to see if an issue with the new issue name/handle is present
>     in the db. The handle is available.
>   tx 1 create a new issue using the name/handle. Success.
>   tx 2 creates a new issue using the handle, fails because the handle
>     was created by tx1.

If you mean by "handle" the id of the new issue: No this is not a problem.
Postgres and MySQL support sequences, each of the transactions above
would obtain a separate sequence number which is used as the new id. The
database code guarantees that these are unique.

You can have problems if you do some checks on issue creation in
userspace (e.g. you want to make sure that a certain issue property is
not in the database): In that case the scenario above will come true
because in concurrent updates you see only the database status at the
start of the current transaction. In that case both checks on uniqueness
in userspace will return "ok" and both transactions will go through.
Thats why databases use uniqueness constraints.

> I think this case will result in a traceback in the postgres case, but
> that depends on the underlying sql that is produced. If the generated
> does an insert rather than an insert and on error update, I hope it
> will boow up in psycop2. Creating an issue to verify that this case is
> handled "correctly" by failing and then making sure a clean
> non-traceback result is returned to the user would be good.

See above: I'm sure that normal issue creation is not subject to
concurrency problems. Doing two concurrent updates on the same properties
of the same issue *will* produce one failing transaction in postgres,
though.

> >It would mean that some decisions based on auditors or reactors may be wrong
> >in a concurrent edit situation for postgresql.

See above: Yes if these have side-effects or you have "hand-coded"
uniqueness constraints checked in userspace.

> Consider:
> 
>  database has prop 1 set to A 
>  tx 1: sets prop 1 to B
>  tx 2: doesn't change prop 1 keeping it at A
> 
>  what happens after tx 2 completes?
> 
>  if tx1 commits before tx2 hits this code, it looks like tx 2
>    changed prop 1 from B to A. The end state is prop 1 = A.
> 
>  if tx2 passes through this code before tx1 hits this code we end up
>    with prop 1 changed to B on tx 1. The end state is prop 1 = B
> 
> I am not sure what should happen there exactly.

The database-behavior of the example above depends on what tx1 and tx2
*read* and *write*: If tx2 doesn't read prop1, *and* (as you implied)
doesn't *write* prop1, tx1 will still "win" and change prop-1 to B. The
two transactions are independent in this case and will both go through.

Note that roundup will always read *all* properties of an issue,
even if you do a p = db.issue.get (id, 'property') this will do
a db.issue.getnode (id) behind the scenes. So my scenario above will
never come true in roundup if both transactions read the same issue.

If tx2 *reads* prop-1 and is selected by the database to win the race,
tx1 is rolled back (because other properties written by tx2 could be
based on the value prop-1). See my example session below (you can try
this yourself).

Roundups implementation makes sure (since some time ago I fixed this)
that no properties are cached beyond transactions: The database needs to
see the writes *and* the reads to the database to decide if transactions
collide.

Example concurrent sessions:

Session 1                               Session 2
>>> import os                           >>> import os
>>> from roundup import instance        >>> from roundup import instance
>>> dir     = os.getcwd ()              >>> dir     = os.getcwd ()
>>> tracker = instance.open (dir)
                                        >>> tracker = instance.open (dir)
>>> db      = tracker.open ('admin')
                                        >>> db      = tracker.open ('admin')
>>> i = db.issue.getnode ('1')          
                                        >>> i = db.issue.getnode ('1')
>>> db.issue.set ('1', title = "HUHU")
                                        >>> db.issue.set ('1', title = "HUHU")
                                        [hangs]
>>> db.commit ()
                                        Traceback (most recent call last):
                                        ...
                                        psycopg2.extensions.TransactionRollbackError: could not serialize access due to concurrent update

Note that I'm seeing these errors about once per month in a very
busy tracker.

Far more often transactions are rolled back because roundup caches the
session cookies and updates them in the same transaction as the actual
data modifications... I think we still have an open issue on that.

It is implemented for quite some time now that roundup will display an
error if it detects that since the browser loaded an issue someone else
updated it and now a change based on an earlier version is submitted. We
probably should display the same error in case of real concurrency errors
from the database in that case.

Ralf
-- 
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   http://www.runtux.com
Reichergasse 131, A-3411 Weidling       email: office@runtux.com
osAlliance member                       email: rsc@osalliance.com