INTPYTHON-355 Add transaction support #313

aclark4life · 2025-06-04T21:16:32Z

todo:

transaction support in migrations
investigate remaining test failures
determine why there's a large speed differences between tests Atlas and GitHub actions w/replica set
test on sharded cluster?

ShaneHarvey · 2025-06-05T19:03:39Z

django_mongodb_backend/features.py

@@ -97,6 +95,22 @@ class DatabaseFeatures(BaseDatabaseFeatures):
        "expressions.tests.ExpressionOperatorTests.test_lefthand_transformed_field_bitwise_or",
    }

+    @cached_property
+    def supports_transactions(self):


What's the purpose of this property? Is it just for the test suite or do apps rely on it?

Not just for testing.

In this case, rather than declare False or True for supports_transactions we dynamically set True or False based on what type of MongoDB we are connected to.

Then DatabaseFeatures is passed to DatabaseWrapper IIRC and the runtime behavior of Django changes accordingly.

Based on this: https://github.com/django/django/blob/main/django/db/backends/base/features.py#L419-L431

It depends on what we want to implement. If we want to dynamically enable transactions based on whether or not the database supports it, we could use it internally. Otherwise, we'll have to introduce a separate setting the user sets to indicate whether or not they want to use transactions, and raise an error if it's not supported.

It's also used by Django in a couple places. For example, to determine whether or not to use transactions to speed up tests.

The issue is that there are certain storage engines where a replica set or sharded cluster does not actually support transactions so this check isn't 100% accurate. That said, those should be rare so I'm fine with the current "hello" based approach. We can always revisit later if it causes an issue.

Fixed in c8f8881 (assuming wired tiger storage engine is correct or we could also check to make sure it is not mmapv1)

django_mongodb_backend/features.py

ShaneHarvey · 2025-06-05T22:28:15Z

django_mongodb_backend/features.py

@@ -97,6 +95,22 @@ class DatabaseFeatures(BaseDatabaseFeatures):
        "expressions.tests.ExpressionOperatorTests.test_lefthand_transformed_field_bitwise_or",
    }

+    @cached_property
+    def supports_transactions(self):


The issue is that there are certain storage engines where a replica set or sharded cluster does not actually support transactions so this check isn't 100% accurate. That said, those should be rare so I'm fine with the current "hello" based approach. We can always revisit later if it causes an issue.

.github/workflows/test-python-replica.yml

ShaneHarvey · 2025-06-09T17:12:44Z

django_mongodb_backend/base.py

@@ -140,6 +142,10 @@ def _isnull_operator(a, b):
    ops_class = DatabaseOperations
    validation_class = DatabaseValidation

+    def __init__(self, settings_dict, alias=DEFAULT_DB_ALIAS):
+        super().__init__(settings_dict, alias=alias)
+        self.session = None


What about thread safety here?

For example:

# Thread 1 with atomic(): .... # Thread 2 with atomic(): ....

Won't the session property be mistakenly shared between threads?

We rely on Django for that.

Can you explain?

I mean if the session property were to be mistakenly shared between threads that would be a Django bug … there's nothing we can do within the framework they provide (_commit, _rollback, etc) to manage threads as far as I know.

In Django, database connections are thread-local, so the use of atomic() in two separate threads will use two separate connections (and thus two separation sessions). It's no different from any other state stored on DatabaseWrapper

[My understanding of thread locals, etc. isn't 100% rock solid, particularly since pymongo operates a bit differently than the PEP 249-conforming drivers for Django's other database backends, but I think what I've stated is correct. Let me know if you have a particular concern.]

Thanks, since each thread gets it's own instance of our DatabaseWrapper I agree this is not an issue. I have a feeling this is not the first nor the last time I will ask this same question so bare with me, Django uses a different model than I'm used to.

aclark4life · 2025-06-10T00:32:15Z

investigate remaining test failures

Referring to tests defined in _django_test_expected_failures_transactions or something else ?

Transactions are tested in Atlas tests.

aclark4life · 2025-06-10T17:20:24Z

django_mongodb_backend/features.py

+        is_sharded_cluster = hello_response.get("msg") == "isdbgrid"
+        if is_replica_set or is_sharded_cluster:
+            engine = client.command("serverStatus").get("storageEngine", {})
+            return engine.get("name") == "wiredTiger"


I find this less readable than the logical grouping, but not enough to protest more than adding this comment!

I nested it to avoid the call to command("serverStatus") when unnecessary, since Shane mentioned it's expensive.

aclark4life changed the title ~~Check for replica set or sharded cluster~~ INTPYTHON-355 Check for replica set or sharded cluster Jun 5, 2025

ShaneHarvey reviewed Jun 5, 2025

View reviewed changes

aclark4life changed the title ~~INTPYTHON-355 Check for replica set or sharded cluster~~ INTPYTHON-355 Add transaction support Jun 6, 2025

aclark4life marked this pull request as ready for review June 6, 2025 23:37

This comment was marked as resolved.

Sign in to view

aclark4life requested a review from timgraham June 6, 2025 23:39

github-advanced-security bot found potential problems Jun 7, 2025

View reviewed changes

.github/workflows/test-python-replica.yml Fixed Show fixed Hide fixed

.github/workflows/test-python-replica.yml Fixed Show fixed Hide fixed

timgraham force-pushed the INTPYTHON-355 branch from 69716b0 to 8361c6b Compare June 7, 2025 12:35

github-advanced-security bot found potential problems Jun 7, 2025

View reviewed changes

.github/workflows/test-python-replica.yml Fixed Show fixed Hide fixed

timgraham force-pushed the INTPYTHON-355 branch 2 times, most recently from e68f8c9 to 4a8209a Compare June 7, 2025 14:37

github-advanced-security bot found potential problems Jun 7, 2025

View reviewed changes

.github/workflows/test-python-replica.yml Fixed Show fixed Hide fixed

timgraham force-pushed the INTPYTHON-355 branch 3 times, most recently from 8205175 to 1c784eb Compare June 8, 2025 02:08

ShaneHarvey reviewed Jun 9, 2025

View reviewed changes

aclark4life requested a review from Jibola June 9, 2025 17:44

timgraham marked this pull request as draft June 9, 2025 19:28

aclark4life and others added 10 commits June 10, 2025 09:38

Check for replica set or sharded cluster

cf614ed

Check for wired tiger storage engine

392bc2f

Add transaction support

ca235e2

pass session to queries

001cc47

disable transactions if not supported

04d3f2f

add replica set tests

176205b

skip union tests on transactions

b3b956f

add transaction logging

8087a7c

add skips for composite_pk tests

c9b639a

fix backend_ tests

9391599

timgraham added 5 commits June 10, 2025 09:38

add expected transaction failures

4eb66e9

move session to instance variable; clear commit hooks in close_pool()

749ab89

remove redundant test script

f26e114

Transactions are tested in Atlas tests.

add tests for DatabaseFeatures.supports_transactions

07df027

first draft docs

44bfab1

timgraham force-pushed the INTPYTHON-355 branch from 16967dd to 763a039 Compare June 10, 2025 13:38

add validation of broken transactions

f007803

timgraham force-pushed the INTPYTHON-355 branch from 763a039 to f007803 Compare June 10, 2025 15:22

aclark4life commented Jun 10, 2025

View reviewed changes

INTPYTHON-355 Add transaction support #313

Are you sure you want to change the base?

INTPYTHON-355 Add transaction support #313

Uh oh!

Conversation

aclark4life commented Jun 4, 2025 • edited by timgraham Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aclark4life commented Jun 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aclark4life commented Jun 4, 2025 •

edited by timgraham

Loading