classification
Title: [sqlite3] Remove special rollback handling
Type: Stage: patch review
Components: Extension Modules Versions: Python 3.11
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: berker.peksag, erlendaasland, lemburg, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2021-05-09 21:36 by erlendaasland, last changed 2021-05-25 22:36 by erlendaasland.

Pull Requests
URL Status Linked Edit
PR 26026 open erlendaasland, 2021-05-10 22:38
Messages (5)
msg393338 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-05-09 21:36
Ref. bpo-33376 and bpo-10513.

Quoting from the SQLite 3.7.11 changelog[1]:
"Pending statements no longer block ROLLBACK. Instead, the pending statement will return SQLITE_ABORT upon next access after the ROLLBACK."

Quoting from the SQLite 3.8.7.2 changelog[2]:
"Enhance the ROLLBACK command so that pending queries are allowed to continue as long as the schema is unchanged. Formerly, a ROLLBACK would cause all pending queries to fail with an SQLITE_ABORT or SQLITE_ABORT_ROLLBACK error. That error is still returned if the ROLLBACK modifies the schema."

Quoting from the SQLite docs[3]:
"In more recent versions of SQLite, the ROLLBACK will proceed and pending statements will often be aborted, causing them to return an SQLITE_ABORT or SQLITE_ABORT_ROLLBACK error. In SQLite version 3.8.8 (2015-01-16) and later, a pending read will continue functioning after the ROLLBACK as long as the ROLLBACK does not modify the database schema."

I've done some tests with SQLite versions 3.35.4 and 3.7.15 where I've removed the call to pysqlite_do_all_statements() (introduced by [4]) in pysqlite_connection_rollback_impl(), and I've also removed the pysqlite_Cursor.reset member and all of the related code. The test suite passes fine (except for, as expected, the two tests that check for InterfaceError in case of fetch across rollback).

Do we really need to special case rollbacks anymore? I've tried to come up with tests that prove this approach wrong, but I haven't found any yet.


[1] https://sqlite.org/changes.html#version_3_7_11
[2] https://sqlite.org/changes.html#version_3_8_7_2
[3] https://www.sqlite.org/lang_transaction.html
[4] https://github.com/ghaering/pysqlite/commit/95f0956d9a78750ac8b5ca54f028b5f8d8db0abb
msg393411 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-05-10 15:47
Quoting pysqlite commit 5a009ed message (https://github.com/ghaering/pysqlite/commit/5a009ed6fb2e90b952438f5786f93cd1e8ac8722):
"Implemented a function that resets all statements in the connection's
  statement cache. After calling this function it is always possible to
  rollback a transaction or close a connection."

The commit is from 2005-12-09. SQLite 3.7.11 wasn't released until 2012, so in 2005 any pending statements would block a rollback. I'm guessing commit 5a009ed addressed that issue.
msg393944 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-05-19 10:25
The effect of PR 26026 is that InterfaceError is no longer raised for fetch across rollback; instead it is up to SQLite how to handle this:

- for some cases, SQLITE_ABORT or SQLITE_ABORT_ROLLBACK may be returned, which will result in an OperationalError (accompanied by a nice error message provided by SQLite)
- for other cases, no error is returned; the operation is allowed and succeeds as expected
- for yet other cases, no error is returned, and the operation was rolled back

A NEWS entry should mention the change in behaviour, but I can't see how it would break existing projects; the current code disallows fetch across rollback (InterfaceError), so any problematic code would have been found, handled and fixed during debugging.
msg393945 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-05-19 10:42
I've crafted a number of rollback tests, but it occurred to me that they are simply just testing SQLite behaviour; not sqlite3 behaviour. I had to adjust the tests according to which version of SQLite was used (for example 3.8.7.2 introduced new behaviour). Such tests are bound to break as SQLite evolves. I'm not sure we want such tests in our test suite; it can make the CI fail for completely unrelated PRs. Suggesting to leave detailed rollback testing to SQLite and just keep a couple of basic tests in our suite.
msg394405 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-05-25 22:36
Berker, does this look ok to you?
History
Date User Action Args
2021-05-25 22:36:30erlendaaslandsetmessages: + msg394405
title: [sqlite3] consider removing special rollback handling -> [sqlite3] Remove special rollback handling
2021-05-19 10:42:54erlendaaslandsetmessages: + msg393945
2021-05-19 10:25:11erlendaaslandsetmessages: + msg393944
2021-05-19 10:15:45erlendaaslandsetmessages: - msg393367
2021-05-19 10:15:40erlendaaslandsetmessages: - msg393339
2021-05-19 10:15:20erlendaaslandsetfiles: - patch.diff
2021-05-10 22:38:06erlendaaslandsetstage: patch review
pull_requests: + pull_request24676
2021-05-10 15:47:37erlendaaslandsetmessages: + msg393411
2021-05-10 08:40:53erlendaaslandsetmessages: + msg393367
2021-05-09 21:40:34erlendaaslandsetmessages: + msg393339
2021-05-09 21:39:36erlendaaslandsetfiles: + patch.diff
keywords: + patch
2021-05-09 21:36:02erlendaaslandcreate