Skip to content

Don't require exclusive transactions for entire SQLite DB #132

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 4, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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