select_for_update(of=...) ignores "self" when using values_list() when not selecting a column from the model

[OutOfFocus4]

As of the most recent version of Django, if a queryset uses values_list(...) and select_for_update(of=("self", ...)), and the resulting SQL contains a nullable join, the resulting SQL will cause a database error if none of the values in values_list(...) involve columns from the queryset's model's table.

[Sarah Boyce]

Thank you for the ticket
I needed to add available_apps = ["django.contrib.auth"] to the test case and both tests passed for me on SQLite. Based off the ticket description, I think one of them should have raised a database error? Can you clarify how to replicate the issue?

[OutOfFocus4]

Replying to Sarah Boyce:

Thank you for the ticket
I needed to add available_apps = ["django.contrib.auth"] to the test case and both tests passed for me on SQLite. Based off the ticket description, I think one of them should have raised a database error? Can you clarify how to replicate the issue?

SQLite doesn't support SELECT ... FOR UPDATE, so the ORM skips the code that is raising the error. If you run the code with a PostgreSQL database, test_proof_of_concept will fail; the stacktrace should end with django.db.utils.NotSupportedError: FOR UPDATE cannot be applied to the nullable side of an outer join.

This error should not occur, because the .select_for_update(of=("self",)) on line 47 should apply the FOR UPDATE to only the auth_user table.

If you execute print(User.objects.values_list("groups__name", flat=True).order_by("groups__name").select_for_update(of=("self",)).filter(pk=1).query) in an atomic block, it prints this SQL:

SELECT "auth_group"."name" AS "groups__name" FROM "auth_user" LEFT OUTER JOIN "auth_user_groups" ON ("auth_user"."id" = "auth_user_groups"."user_id") LEFT OUTER JOIN "auth_group" ON ("auth_user_groups"."group_id" = "auth_group"."id") WHERE "auth_user"."id" = 1 ORDER BY 1 ASC FOR UPDATE

Executing print(User.objects.values_list("groups__name", 'pk').order_by("groups__name").select_for_update(of=("self",)).filter(pk=1).query) in an atomic block prints this SQL:

SELECT "auth_group"."name" AS "groups__name", "auth_user"."id" AS "pk" FROM "auth_user" LEFT OUTER JOIN "auth_user_groups" ON ("auth_user"."id" = "auth_user_groups"."user_id") LEFT OUTER JOIN "auth_group" ON ("auth_user_groups"."group_id" = "auth_group"."id") WHERE "auth_user"."id" = 1 ORDER BY 1 ASC FOR UPDATE OF "auth_user"

Notice how FOR UPDATE OF "auth_user" is only added when a column from the auth_user table is SELECTed.

[Sarah Boyce]

Thank you, replicated
That this is a m2m field is important I believe

Possible test:

tests/select_for_update/models.py

@@ -41 +41 @@
     name = models.CharField(max_length=30)
     born = models.ForeignKey(City, models.CASCADE, related_name="+")
     died = models.ForeignKey(City, models.CASCADE, related_name="+")
+    lived = models.ManyToManyField(City, related_name="people_set")
 
 
 class PersonProfile(models.Model):

tests/select_for_update/tests.py

@@ -278 +278 @@
             )
         self.assertEqual(values, [(self.person.pk,)])
 
+    @skipUnlessDBFeature("has_select_for_update_of")
+    def test_for_update_of_followed_by_values_list_of_m2m_field(self):
+        self.person.lived.add(self.city1)
+        self.person.lived.add(self.city2)
+
+        with transaction.atomic():
+            values = list(
+                Person.objects.select_for_update(of=("self",))
+                .values_list("lived__name", flat=True)
+            )
+        self.assertEqual(values, [self.city1.name, self.city2.name])
+
     @skipUnlessDBFeature("has_select_for_update_of")
     def test_for_update_of_self_when_self_is_not_selected(self):

[OutOfFocus4]

Replying to Sarah Boyce:

Thank you, replicated
That this is a m2m field is important I believe

I have replicated the error with reverse foreign keys and nullable forward foreign keys. I have attached the tests. The models.py is identical to the one currently used for Django tests, except the died field of the Person model has null=True.

[OutOfFocus4]

The issue is still present in Django 5.2.4.

[OutOfFocus4]

The issue is still present in Django 5.2.5.

[Sarah Boyce]

This won't be resolved until someone fixes the ticket, so there's no need to confirm with each release that this is still open (that the ticket is open is enough to confirm that this should still be an open issue)

We also wouldn't backport a fix into 5.2 unless this is a bug that was introduced by 5.2. If you believe this was introduced in 5.2, you can do a git bisect to find the commit that introduces this issue. This would increase the priority of the ticket.

[OutOfFocus4]

Replying to Sarah Boyce:

This won't be resolved until someone fixes the ticket, so there's no need to confirm with each release that this is still open (that the ticket is open is enough to confirm that this should still be an open issue)

We also wouldn't backport a fix into 5.2 unless this is a bug that was introduced by 5.2. If you believe this was introduced in 5.2, you can do a git bisect to find the commit that introduces this issue. This would increase the priority of the ticket.

I can replicate this issue with older versions of Django, so I don't believe this was introduced in 5.2.

I believe I may have found (part of) the cause. From ​https://github.com/django/django/blob/d82f25d3f0f4eb7be721a72d0e79a8d13d394d32/django/db/models/sql/compiler.py#L1432C1-L1443C56:

        def _get_first_selected_col_from_model(klass_info):
            """
            Find the first selected column from a model. If it doesn't exist,
            don't lock a model.

            select_fields is filled recursively, so it also contains fields
            from the parent models.
            """
            concrete_model = klass_info["model"]._meta.concrete_model
            for select_index in klass_info["select_fields"]:
                if self.select[select_index][0].target.model == concrete_model:
                    return self.select[select_index][0]

If the docstring is to be believed, only tables whose columns are in the query's SELECT can be locked. I do not know enough about Django's internals to know if that is what the function actually does.

[Shai Berger]

While the code provided in the attached tests can be used to reproduce the problem, only the second test fails, and its failure is a "second order result" -- an error trying to lock the nullable side of a join, when Django was explicitly asked to lock only the non-nullable side, but ignored the request.

This change causes an existing test to fail, on the immediate cause, and so I think it is useful as a step towards resolving this ticket.

tests/select_for_update/tests.py

@@ -284 +284 @@
         select_for_update(of=['self']) when the only columns selected are from
         related tables.
         """
-        with transaction.atomic():
+        with transaction.atomic(), CaptureQueriesContext(connection) as ctx:
             values = list(
-                Person.objects.select_related("born")
+                Person.objects  # Note: select_related() is canceled by values()
                 .select_for_update(of=("self",))
                 .values("born__name")
             )
         self.assertEqual(values, [{"born__name": self.city1.name}])
+        # Check for #36398 -- locking is limited to self
+        features = connections["default"].features
+        if features.select_for_update_of_column:
+            expected = [
+                'select_for_update_person"."id',
+            ]
+        else:
+            expected = ["select_for_update_person"]
+        expected = [connection.ops.quote_name(value) for value in expected]
+        self.assertTrue(self.has_for_update_sql(ctx.captured_queries, of=expected))
 
     @skipUnlessDBFeature(
         "has_select_for_update_of",