sqlite3 module: Improved concurrency

[ghaering]

BPO	3846
Nosy	@loewis, @amauryfa
Files	sqlite_concurrency_prepare.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2008-09-12.22:50:59.725>
created_at = <Date 2008-09-12.13:57:29.793>
labels = ['performance']
title = 'sqlite3 module: Improved concurrency'
updated_at = <Date 2008-09-12.22:50:59.723>
user = 'https://bugs.python.org/ghaering'

bugs.python.org fields:

activity = <Date 2008-09-12.22:50:59.723>
actor = 'ghaering'
assignee = 'ghaering'
closed = True
closed_date = <Date 2008-09-12.22:50:59.725>
closer = 'ghaering'
components = []
creation = <Date 2008-09-12.13:57:29.793>
creator = 'ghaering'
dependencies = []
files = ['11473']
hgrepos = []
issue_num = 3846
keywords = ['needs review']
message_count = 9.0
messages = ['73086', '73088', '73092', '73121', '73124', '73125', '73127', '73146', '73150']
nosy_count = 3.0
nosy_names = ['loewis', 'ghaering', 'amaury.forgeotdarc']
pr_nums = []
priority = 'high'
resolution = 'accepted'
stage = None
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue3846'
versions = ['Python 2.6']

[ghaering]

I'd really like this change to get into Python 2.6. It's pretty trivial
(just releases the GIL when compiling SQLite statements), but improves
concurrency for SQLite. This means less "database is locked" errors for
our users.

Could somebody please review this and give me an ok to check it in?

[amauryfa]

Your patch seems obvious, however:

• Is there the possibility that sqlite3_prepare() calls back into a
  function in the _sqlite module or other python code? In this case the
  GIL has to be re-acquired.

• What if another thread closes the current connection (or drop a table,
  etc.) in-between? Is sqlite really thread-safe?

[ghaering]

1. SQLite calling back

Good that you mention it. During sqlite3_prepare, SQLite can call the
authorizer_callback. Fortunately, it already acquires/releases the GIL
appropriately already.

2. Other thread closing connection, etc.

Connections/cursors cannot be shared among Python threads. Well, with
newer SQLite releases I think it is possible. But we have checks in
place that raise an error if you do it. These are the various calls to
pysqilte_check_thread in the code. If the table is dropped,
sqlite3_prepare will just raise an error about the statement not being
valid.

If, however, the table is dropped between sqlite3_prepare and
sqlite3_step, then the SQLITE_SCHEMA error code is returned. We then try
to recompile the statement (it might have just been an ALTER TABLE and
the statement is valid after compiling it again). If that still fails,
we promote the error to Python (cursor.c lines 605ff).

[loewis]

Asking in the same direction: what is the consequence of this patch when
check_same_thread is False? Couldn't that result in very problematic
overlappings, e.g. when two threads try to execute statements on the
same cursor?

[ghaering]

Interesting. I was smart enough to not document check_same_thread in the
sqlite3 incarnation of the module.

This has nothing to do with releasing/aquiring the GIL around
sqlite3_prepare, though. Adding the macros was just an oversight.
They're there for all other nontrivial SQLite API calls (sqlite3_step,
sqlite3_reset, sqlite3_open, sqlite3_finalize and also already for the
"BEGIN"/"COMMIT" and "ROLLBACK" versions of sqlite3_prepare in connection.c.

[ghaering]

Just to be explicit: check_same_thread is unsupported and while it's
undocumented in sqlite3, the old pysqlite docs say that when you use it,
you have to make sure the connections/cursors are protected otherwise
(via your own mutexes).

[In the current pysqlite docs it's not documented at all at the moment,
because I derived these from the Python sqlite3 ones]

[loewis]

This has nothing to do with releasing/aquiring the GIL around
sqlite3_prepare, though.

You mean, it doesn't make things worse, but I believe they do
(even if so only slightly). If you have two threads simultaneously
attempting an execute on a cursor, one will run into the prepare.

That thread will (now) release the GIL, allowing the second thread
to run into the prepare. That thread will replace the statement object
in the cursor, so when the first thread returns, the statement has
changed underneath, and it will associate any return code with the
incorrect statement object.

In this form, this can't currently happen. Without detailed review,
I can readily believe that there are many other cases where a
False check_same_thread can give surprising results.

They're there for all other nontrivial SQLite API calls (sqlite3_step,
sqlite3_reset, sqlite3_open, sqlite3_finalize and also already for the
"BEGIN"/"COMMIT" and "ROLLBACK" versions of sqlite3_prepare in connection.c.

On the grounds that the code is already full of these GIL release calls,
I'll approve this additional one.

I encourage you to review the entire issue, though, and document
somewhere what promises are made under what conditions. Then a later
review can validate that the promises are really kept.

[ghaering]

Thanks, Martin.

Commited as r66414.

[ghaering]

bpo-3854 was created for documenting sqlite3 vs. threads.