[Django] #29280: Fix for a class of SQLite "database is locked" problems
Django
-------------------------------------+-------------------------------------
Reporter: Ove Kåven | Owner: nobody
Type: Bug | Status: new
Component: Database | Version: 2.0
layer (models, ORM) |
Severity: Normal | Keywords: sqlite, databases
Triage Stage: | Has patch: 1
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
-------------------------------------+-------------------------------------
I'd like to propose a change like this, which I think would fix a class of
SQLite "database is locked" problems, e.g. maybe #29062. But it's possible
you want to add a config option, or an argument to transaction.atomic(),
or something of the kind.
{{{
diff --git a/django/db/backends/sqlite3/base.py
b/django/db/backends/sqlite3/base.py
index 3989028930..391a50789e 100644
--- a/django/db/backends/sqlite3/base.py
+++ b/django/db/backends/sqlite3/base.py
@@ -272,7 +272,7 @@ class DatabaseWrapper(BaseDatabaseWrapper):
Staying in autocommit mode works around a bug of sqlite3 that
breaks
savepoints when autocommit is disabled.
"""
- self.cursor().execute("BEGIN")
+ self.cursor().execute("BEGIN IMMEDIATE")
def is_in_memory_db(self):
return self.creation.is_in_memory_db(self.settings_dict['NAME'])
}}}
Explanation: Consider a type of transaction consisting of, for example, an
update_or_create() call, which you run in a "with transaction.atomic()"
context. Suppose two threads or processes run such a transaction at
exactly the same time (but with different keys, doesn't matter, SQLite
locks the whole database anyway).
1. The current transaction.atomic() implementation results in a "BEGIN",
which tells SQLite to start deferred transactions, i.e., to *not* acquire
any locks before it has to.
2. Normally, update_or_create would first do a "SELECT FOR UPDATE", but
SQLite doesn't support that, so a plain "SELECT" is done. Both threads
acquire a shared read lock, and so the SELECTs succeed for both threads.
3. Next, update_or_create needs to do an INSERT or UPDATE (not important
which), so the threads needs to upgrade the read locks to write locks.
Unfortunately, only one thread can have the write lock.
Now, the designers of SQLite apparently realized that if you already have
the read lock, then it's not a good idea to start a blocking wait on the
write lock. If you wait while holding the read lock, then nobody will ever
be able to acquire exclusive access to the database, and everything will
just hang. On the other hand, if you drop the read lock and then wait,
then you lose the 'serializable' isolation guarantee that SQLite tries to
give. Hence, SQLite has only one choice.
4. The thread that didn't get the write lock immediately gets a "database
is locked" error. Its transaction is aborted.
The timeout mentioned in the Django documentation will have absolutely no
effect on this, and it doesn't matter how short-lived your transactions
are. It only matters that they do a read before they do a write.
I can see three possible solutions to this problem:
1. "Don't do that". Don't use SQLite, or don't do concurrency. (Many
answers on StackOverflow.com and such are essentially this.)
2. Treat the "database is locked" as if it were a "serialization error",
which it kind of is. That is, the app must retry the transaction until it
commits. That works, but is somewhat unsatisfactory.
3. Grab the write lock immediately (like "SELECT FOR UPDATE" would have,
if SQLite had supported it). That's what "BEGIN IMMEDIATE", in the above
patch, does. After all, if you're not holding the read lock when you try
to grab the write lock, then this particular problem won't happen. (Of
course, the lock timeout thing that's mentioned in the Django
documentation can still happen if your transactions are too long-lived,
but that's different.)
And I think that if you're using "with transaction.atomic()" in the first
place, then you most likely want to write to the database, in which case
there's not much reason not to always grab the write lock right away. But
even if it's decided that some config option would be good, it probably
shouldn't be too hard to add? Or?
--
Ticket URL: <https://code.djangoproject.com/ticket/29280>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Ove Kåven | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: sqlite, databases | Triage Stage:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
New description:
I'd like to propose a change like this, which I think would fix a class of
SQLite "database is locked" problems. But it's possible you want to add a
--
--
Ticket URL: <https://code.djangoproject.com/ticket/29280#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Ove Kåven | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Aymeric Augustin (added)
Comment:
Aymeric, do you have expertise to offer on this proposal?
--
Ticket URL: <https://code.djangoproject.com/ticket/29280#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Ove Kåven | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Aymeric Augustin):
So the proposal here is to always use immediate rather than deferred
transactions.
Per https://www.sqlite.org/lang_transaction.html:
> After a BEGIN IMMEDIATE, no other database connection will be able to
write to the database or do a BEGIN IMMEDIATE or BEGIN EXCLUSIVE.
This means all `atomic` blocks will be serialized. Combined with
`ATOMIC_REQUESTS = True`, HTTP requests will be serialized. I don't think
that's an acceptable behavior.
In general the performance hit of removing the possibility of concurrent
database reads — and web workflows tend to be read-heavy — seems too high
to implement this in Django.
Also I'm not sure it fits well with the premise of allowing concurrency.
It brings us back to "Don't use SQLite, or don't do concurrency."
FWIW you can trivially subclass (or monkey-patch) the SQLite database
adapter to make this change in your project.
--
Ticket URL: <https://code.djangoproject.com/ticket/29280#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Ove Kåven | Owner: nobody
Component: Database layer | Version: 2.0
(models, ORM) |
Keywords: sqlite, databases | Triage Stage:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => invalid
--
Ticket URL: <https://code.djangoproject.com/ticket/29280#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Ove Kåven | Owner: nobody
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Normal | Resolution: invalid
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by btimby):
Just a not for anyone who has this same problem and is looking for a
solution...
I agree with the assessment that transforming all atomic blocks into
immediate transactions would have a huge negative impact. However, SOME
atomic blocks (those containing select_for_update or update_or_create)
when on sqlite and using threads / processes _could_ be immediate
transactions without too great of an impact. This can be accomplished by
monkey patching the atomic decorator / context manager to support an
immediate argument.
If you add the following to your project's {{{__init__.py}}}:
{{{
from django.db import connection, DEFAULT_DB_ALIAS
from django.db import transaction
def _monkey_patch_atomic():
def atomic(using=None, savepoint=True, immediate=False):
# Bare decorator: @atomic -- although the first argument is called
# `using`, it's actually the function being decorated.
if callable(using):
a = transaction.Atomic(DEFAULT_DB_ALIAS, savepoint)(using)
# Decorator: @atomic(...) or context manager: with atomic(...):
...
else:
a = transaction.Atomic(using, savepoint)
a.immediate = immediate
return a
def __enter__(self):
connection = transaction.get_connection(self.using)
if not connection.in_atomic_block:
# Reset state when entering an outermost atomic block.
connection.commit_on_exit = True
connection.needs_rollback = False
if not connection.get_autocommit():
# Pretend we're already in an atomic block to bypass the
code
# that disables autocommit to enter a transaction, and
make a
# note to deal with this case in __exit__.
connection.in_atomic_block = True
connection.commit_on_exit = False
if connection.in_atomic_block:
# We're already in a transaction; create a savepoint, unless
we
# were told not to or we're already waiting for a rollback.
The
# second condition avoids creating useless savepoints and
prevents
# overwriting needs_rollback until the rollback is performed.
if self.savepoint and not connection.needs_rollback:
sid = connection.savepoint()
connection.savepoint_ids.append(sid)
else:
connection.savepoint_ids.append(None)
else:
if self.immediate:
connection.cursor().execute('BEGIN IMMEDIATE')
else:
connection.set_autocommit(False,
force_begin_transaction_with_broken_autocommit=True)
connection.in_atomic_block = True
transaction.atomic = atomic
transaction.Atomic.immediate = False
transaction.Atomic.__enter__ = __enter__
_monkey_patch_atomic()
}}}
You can then do:
{{{
with atomic(immediate=True):
ModelName.objects.update_or_create(foo=foo)
}}}
And the transaction will be started with {{{BEGIN IMMEDIATE}}} avoiding
the deadlock described here. With this change in place the erstwhile
deadlock victim will wait for the victor.
--
Ticket URL: <https://code.djangoproject.com/ticket/29280#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Ove Kåven | Owner: nobody
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Simon Charette):
* status: closed => new
* type: Bug => New feature
* easy: 1 => 0
* has_patch: 1 => 0
* resolution: invalid =>
* stage: Unreviewed => Accepted
Comment:
Re-opening as a new feature to target adding a SQLite specific
`OPTIONS["transaction_mode"]` option per
[https://forum.djangoproject.com/t/sqlite-and-database-is-locked-
error/26994/12 the forum] and
[https://discord.com/channels/856567261900832808/859997770274045954/1197570022973898752
Discord] discussions.
--
Ticket URL: <https://code.djangoproject.com/ticket/29280#comment:6>
Django
Reporter: Ove Kåven | Owner: Anže
| Pečar
Type: New feature | Status: assigned
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* owner: nobody => Anže Pečar
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/29280#comment:7>
Django
Reporter: Ove Kåven | Owner: Anže
| Pečar
Type: New feature | Status: assigned
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Anže Pečar):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/29280#comment:8>
Django
Reporter: Ove Kåven | Owner: Anže
| Pečar
Type: New feature | Status: assigned
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
[https://github.com/django/django/pull/17760 PR]. Please don't mark your
own PRs as "Ready for checkin".
--
Ticket URL: <https://code.djangoproject.com/ticket/29280#comment:9>
Django
Reporter: Ove Kåven | Owner: Anže
| Pečar
Type: New feature | Status: assigned
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/29280#comment:10>
Django
Reporter: Ove Kåven | Owner: Anže
| Pečar
Type: New feature | Status: assigned
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/29280#comment:11>
Django
Reporter: Ove Kåven | Owner: Anže
| Pečar
Component: Database layer | Version: 2.0
(models, ORM) |
Keywords: sqlite, databases | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"a0204ac183ad6bca71707676d994d5888cf966aa" a0204ac1]:
{{{#!CommitTicketReference repository=""
revision="a0204ac183ad6bca71707676d994d5888cf966aa"
Fixed #29280 -- Made the transactions behavior configurable on SQLite.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29280#comment:12>
Django
| Pečar
Type: New feature | Status: closed
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: sqlite, databases | Triage Stage: Ready for
| checkin
-------------------------------------+-------------------------------------
Comment (by Sarah Boyce <42296566+sarahboyce@…>):
In [changeset:"9d5c02446e5947fbeb74dda1b6b246fd8268009d" 9d5c0244]:
{{{#!CommitTicketReference repository=""
revision="9d5c02446e5947fbeb74dda1b6b246fd8268009d"
Refs #29280 -- Moved release note about transaction_mode to "Database
backends" section.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29280#comment:13>