Ticket #36915 - Request for Triage Review - select_for_update for implicit transactions

Django Internals ORM
1

Hi everyone,

I have been working on ticket #36915
(Add support for select_for_update for implicit transactions).

The problem is that select_for_update() raises
TransactionManagementError when used as a subquery
inside .update() even though DML creates an implicit
transaction at the database level.

I have a working proof of concept fix:
PR: Fixed #36915 -- Allow select_for_update() in subqueries of DML statements. by MANAS225 · Pull Request #20773 · django/django · GitHub

The PR was closed because the ticket was not yet
accepted. Could someone please review ticket #36915
and accept it so the PR can be reconsidered?

Ticket: #36915 (Add support for select_for_update for implicit transactions) – Django

Thank you!

2

I agree with Jacob assessent that no changes are required and the docs are adequate.

The patch you’ve proposed that relies on self.subquery assumes that all subqueries are nested im DML statement (e.g. UPDATE, DELETE) that create implicit transactions but it’s not the case.

It would wrongly allow queries of the form

Person.objects.filter(
    pk__in=Person.objects.filter(name="Reinhardt").select_for_update()
).get()

without an explicit wrapping in a transaction.atomic.

There are likely ways to adjust compilers (e.g. SQLUpdateCompiler and SQLDeleteCompiler) to denote whether or not they result in an implicit transaction but the challenge will be that we’d need to adjust the compiling logic to keep track of parent compilers which is not trivial.

In other words, I don’t think implementing such feature is worth the complexity it would introduce.

3

I was curious to see how invasive such a change would be and I figured I’d share what I’ve found

diff --git a/django/db/models/sql/compiler.py b/django/db/models/sql/compiler.py
index cfb4b93e65..1558e1cec3 100644
--- a/django/db/models/sql/compiler.py
+++ b/django/db/models/sql/compiler.py
@@ -43,6 +43,7 @@ class SQLCompiler:
         r"^(.*)\s(?:ASC|DESC).*",
         re.MULTILINE | re.DOTALL,
     )
+    in_implicit_transaction = False
 
     def __init__(self, query, connection, using, elide_empty=True):
         self.query = query
@@ -828,7 +829,8 @@ class SQLCompiler:
 
                 if self.query.select_for_update and features.has_select_for_update:
                     if (
-                        self.connection.get_autocommit()
+                        not self.in_implicit_transaction
+                        and self.connection.get_autocommit()
                         # Don't raise an exception when database doesn't
                         # support transactions, as it's a noop.
                         and features.supports_transactions
@@ -1671,6 +1673,7 @@ class SQLCompiler:
 class SQLInsertCompiler(SQLCompiler):
     returning_fields = None
     returning_params = ()
+    in_implicit_transaction = True
 
     def field_as_sql(self, field, get_placeholder, val):
         """
@@ -1954,6 +1957,8 @@ class SQLInsertCompiler(SQLCompiler):
 
 
 class SQLDeleteCompiler(SQLCompiler):
+    in_implicit_transaction = True
+
     @cached_property
     def single_alias(self):
         # Ensure base table is in aliases.
@@ -2016,6 +2021,7 @@ class SQLDeleteCompiler(SQLCompiler):
 class SQLUpdateCompiler(SQLCompiler):
     returning_fields = None
     returning_params = ()
+    in_implicit_transaction = True
 
     def as_sql(self):
         """
diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py
index 7a4cf843c1..125f740555 100644
--- a/django/db/models/sql/query.py
+++ b/django/db/models/sql/query.py
@@ -1314,7 +1314,9 @@ class Query(BaseExpression):
             self.clear_ordering(force=False)
             for query in self.combined_queries:
                 query.clear_ordering(force=False)
-        sql, params = self.get_compiler(connection=connection).as_sql()
+        query_compiler = self.get_compiler(connection=connection)
+        query_compiler.in_implicit_transaction |= compiler.in_implicit_transaction
+        sql, params = query_compiler.as_sql()
         if self.subquery:
             sql = "(%s)" % sql
         return sql, params

The idea is that SQLCompiler has a in_implicit_transaction: bool flag that determines whether it compiles a query meant to be executed in an implicit transaction and sql.Query.as_sql (called at subquery compiling time) union this flag.

This means that subqueries in DML get their flag enabled while ones that are not remain False (the core problem for checking self.subquery like the patch did).

I still remain on the fence about whether it’s worth adding but I thought I’d share how this can be achieved if it can be handy in the future.