If SQLite commit() fails, transaction status gets incorrectly cleared

[Richard Boulton]

If a commit() call in SQLIte fails, for example, because another connection is keeping a lock on the database, subsequent calls to the connection fail with "OperationalError: cannot start a transaction within a transaction". This appears to be because the SQLiteConnection._in_transaction member is cleared before the commit() call is made, rather than only being cleared after it (if it succeeds).

Also, the commit() call doesn't respect the timeout setting - it fails immediately.

The sqlite-locked-recovery branch includes a testcase for this problem, and also includes a fix for it.

[Richard Boulton (richardboulton)]

After looking into the state in which the database is left after a commit() fails more closely, I believe that the connection is only left in a state in which COMMIT can be retried if the failure was due to the database being locked. In all other cases, the transaction is rolled back, and the connection is no longer in the middle of a transaction. My proposed fix assumed that any failure left the transaction open.

According to http://www.sqlite.org/lang_transaction.html the "database locked" case can be detected by the SQLITE_BUSY return code. pysqlite2 converts this to the "database is locked" OperationalError, and after reading through the code, I believe no other case will have this message. Therefore, we can detect this case, and set the SQLiteConnection._in_transaction member to False in all other cases.

I've pushed changes to implement this to the sqlite-locked-recovery branch.

[Christopher Armstrong (radix)]

[1] While you're refactoring that code into _retry_until_timeout, can you also please add a comment that explains how this handles either the case where sqlite3 does the timeout itself or when we need to do it?

[2] I like how you documented the test, but it's way too big - can you break it up into smaller more "unity" tests?

[3] Is there any chance we can do something other than parsing exception strings? Maybe we should submit a bug request to the PySQLite author to include the sqlite error code in his exception objects.

[Christopher Armstrong (radix)]

I just filed a PySQLite bug. http://www.initd.org/tracker/pysqlite/ticket/206

[Richard Boulton (richardboulton)]

I've pushed changes to the sqlite-locked-recovery branch to address issue 1 and 2 which you raised: the test is only split into two parts, but I can't see how to make it smaller than it now is.

I removed the RetryableOperationalError idea, too - discussion on IRC concluded that that wasn't a useful approach.

For the record, I don't think there is anything we can currently do to avoid parsing exception strings, other than wait for pysqlite to be enhanced.

[Gustavo Niemeyer (niemeyer)]

Richard, I've made a small modification to raw_execute() which solves the problem,
and avoids the need for the additional _retry_until_timeout() method.

I've integrated your tests unchanged, and also integrated your nice comments
into the existing raw_execute() method.

I'm merging this with [r=radix,niemeyer].

Thanks a lot for this!