Skip to content

Commit

Permalink
Don't require exclusive transactions for entire SQLite DB (#132)
Browse files Browse the repository at this point in the history
Where needed, queries are run in an exclusive transaction. The rest don't need to be, which hopefully improves throughput and prevents lock contention
  • Loading branch information
RealOrangeOne authored Feb 4, 2025
1 parent 4edc550 commit c84d060
Show file tree
Hide file tree
Showing 4 changed files with 16 additions and 45 deletions.
19 changes: 1 addition & 18 deletions django_tasks/backends/database/backend.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,10 @@
from dataclasses import dataclass
from typing import TYPE_CHECKING, Any, TypeVar

import django
from django.apps import apps
from django.core.checks import messages
from django.core.exceptions import ValidationError
from django.db import connections, router, transaction
from django.db import transaction
from typing_extensions import ParamSpec

from django_tasks.backends.base import BaseTaskBackend
Expand Down Expand Up @@ -81,9 +80,6 @@ async def aget_result(self, result_id: str) -> TaskResult:
raise ResultDoesNotExist(result_id) from e

def check(self, **kwargs: Any) -> Iterable[messages.CheckMessage]:
from .models import DBTaskResult
from .utils import connection_requires_manual_exclusive_transaction

yield from super().check(**kwargs)

backend_name = self.__class__.__name__
Expand All @@ -93,16 +89,3 @@ def check(self, **kwargs: Any) -> Iterable[messages.CheckMessage]:
f"{backend_name} configured as django_tasks backend, but database app not installed",
"Insert 'django_tasks.backends.database' in INSTALLED_APPS",
)

db_connection = connections[router.db_for_read(DBTaskResult)]
# Manually called to set `transaction_mode`
db_connection.get_connection_params()
if (
# Versions below 5.1 can't be configured, so always assume exclusive transactions
django.VERSION >= (5, 1)
and connection_requires_manual_exclusive_transaction(db_connection)
):
yield messages.Error(
f"{backend_name} is using SQLite non-exclusive transactions",
f"Set settings.DATABASES[{db_connection.alias!r}]['OPTIONS']['transaction_mode'] to 'EXCLUSIVE'",
)
7 changes: 4 additions & 3 deletions django_tasks/backends/database/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ def connection_requires_manual_exclusive_transaction(
if django.VERSION < (5, 1):
return True

if not hasattr(connection, "transaction_mode"):
# Manually called to set `transaction_mode`
connection.get_connection_params()

return connection.transaction_mode != "EXCLUSIVE" # type:ignore[attr-defined,no-any-return]


Expand All @@ -35,9 +39,6 @@ def exclusive_transaction(using: Optional[str] = None) -> Generator[Any, Any, An
connection: BaseDatabaseWrapper = transaction.get_connection(using)

if connection_requires_manual_exclusive_transaction(connection):
if django.VERSION >= (5, 1):
raise RuntimeError("Transactions must be EXCLUSIVE")

with connection.cursor() as c:
c.execute("BEGIN EXCLUSIVE")
try:
Expand Down
5 changes: 0 additions & 5 deletions tests/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
import sys

import dj_database_url
import django

BASE_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))

Expand Down Expand Up @@ -59,10 +58,6 @@
)
}

# Set exclusive transactions in 5.1+
if django.VERSION >= (5, 1) and "sqlite" in DATABASES["default"]["ENGINE"]:
DATABASES["default"].setdefault("OPTIONS", {})["transaction_mode"] = "EXCLUSIVE"

if "sqlite" in DATABASES["default"]["ENGINE"]:
DATABASES["default"]["TEST"] = {"NAME": os.path.join(BASE_DIR, "db-test.sqlite3")}

Expand Down
30 changes: 11 additions & 19 deletions tests/tests/test_database_backend.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
from django.db import connection, connections, transaction
from django.db.models import QuerySet
from django.db.utils import IntegrityError, OperationalError
from django.test import TestCase, TransactionTestCase, override_settings
from django.test import TransactionTestCase, override_settings
from django.test.testcases import _deferredSkip # type:ignore[attr-defined]
from django.urls import reverse
from django.utils import timezone
Expand Down Expand Up @@ -264,23 +264,6 @@ def test_database_backend_app_missing(self) -> None:
self.assertEqual(len(errors), 1)
self.assertIn("django_tasks.backends.database", errors[0].hint) # type:ignore[arg-type]

@skipIf(
connection.vendor != "sqlite", "Transaction mode is only applicable on SQLite"
)
@skipIf(django.VERSION < (5, 1), "Manual transaction check only runs on 5.1+")
def test_check_non_exclusive_transaction(self) -> None:
try:
with mock.patch.dict(
connection.settings_dict["OPTIONS"], {"transaction_mode": None}
):
errors = list(default_task_backend.check())

self.assertEqual(len(errors), 1)
self.assertIn("['transaction_mode'] to 'EXCLUSIVE'", errors[0].hint) # type:ignore[arg-type]
finally:
connection.close()
connection.get_connection_params()

def test_priority_range_check(self) -> None:
with self.assertRaises(IntegrityError):
DBTaskResult.objects.create(
Expand Down Expand Up @@ -981,12 +964,13 @@ def test_get_locked_with_locked_rows(self) -> None:
new_connection.close()


class ConnectionExclusiveTranscationTestCase(TestCase):
class ConnectionExclusiveTranscationTestCase(TransactionTestCase):
def setUp(self) -> None:
self.connection = connections.create_connection("default")

def tearDown(self) -> None:
self.connection.close()
# connection.close()

@skipIf(connection.vendor == "sqlite", "SQLite handled separately")
def test_non_sqlite(self) -> None:
Expand Down Expand Up @@ -1018,6 +1002,14 @@ def test_explicit_transaction(self) -> None:
connection_requires_manual_exclusive_transaction(self.connection)
)

@skipIf(connection.vendor != "sqlite", "SQLite only")
def test_exclusive_transaction(self) -> None:
with self.assertNumQueries(2) as c:
with exclusive_transaction():
pass

self.assertEqual(c.captured_queries[0]["sql"], "BEGIN EXCLUSIVE")


@override_settings(
TASKS={
Expand Down

0 comments on commit c84d060

Please sign in to comment.