[Django] #36398: select_for_update(of=...) ignores "self" when using values_list() when not selecting a column from the model

[Django]

#36398: select_for_update(of=...) ignores "self" when using values_list() when not
selecting a column from the model
-------------------------------------+-------------------------------------
Reporter: OutOfFocus4 | Type:
| Uncategorized
Status: new | Component: Database
| layer (models, ORM)
Version: 5.2 | Severity: Normal
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36398>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36398: select_for_update(of=...) ignores "self" when using values_list() when not
selecting a column from the model
-------------------------------------+-------------------------------------

Reporter: OutOfFocus4 | Owner: (none)
Type: Uncategorized | Status: new
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by OutOfFocus4):

* Attachment "tests.py" added.

[Django]

#36398: select_for_update(of=...) ignores "self" when using values_list() when not
selecting a column from the model
-------------------------------------+-------------------------------------
Reporter: OutOfFocus4 | Owner: (none)

Type: Uncategorized | Status: closed

Component: Database layer | Version: 5.2
(models, ORM) |

Severity: Normal | Resolution: needsinfo

Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* resolution: => needsinfo
* status: new => closed

Comment:

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?
--
Ticket URL: <https://code.djangoproject.com/ticket/36398#comment:1>

[Django]

#36398: select_for_update(of=...) ignores "self" when using values_list() when not
selecting a column from the model
-------------------------------------+-------------------------------------
Reporter: OutOfFocus4 | Owner: (none)
Type: Uncategorized | Status: closed
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by OutOfFocus4):

Replying to [comment:1 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 `SELECT`ed.
--
Ticket URL: <https://code.djangoproject.com/ticket/36398#comment:2>

[Django]

#36398: select_for_update(of=...) ignores "self" when using values_list() when not
selecting a column from the model
-------------------------------------+-------------------------------------
Reporter: OutOfFocus4 | Owner: (none)

Type: Bug | Status: new
Component: Database layer | Version: dev

(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* resolution: needsinfo =>
* stage: Unreviewed => Accepted
* status: closed => new
* type: Uncategorized => Bug
* version: 5.2 => dev

Comment:

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

Possible test:

{{{#!diff
--- a/tests/select_for_update/models.py
+++ b/tests/select_for_update/models.py
@@ -41,6 +41,7 @@ class Person(models.Model):
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):
diff --git a/tests/select_for_update/tests.py
b/tests/select_for_update/tests.py
index 1bc87113ba..49453240c5 100644
--- a/tests/select_for_update/tests.py
+++ b/tests/select_for_update/tests.py
@@ -278,6 +278,18 @@ class SelectForUpdateTests(TransactionTestCase):
)
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):
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36398#comment:3>

[Django]

#36398: select_for_update(of=...) ignores "self" when using values_list() when not
selecting a column from the model
-------------------------------------+-------------------------------------
Reporter: OutOfFocus4 | Owner:

| JakeWalson
Type: Bug | Status: assigned

Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by JakeWalson):

* owner: (none) => JakeWalson
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36398#comment:4>

[Django]

#36398: select_for_update(of=...) ignores "self" when using values_list() when not
selecting a column from the model
-------------------------------------+-------------------------------------
Reporter: OutOfFocus4 | Owner:
| JakeWalson
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by OutOfFocus4):

Replying to [comment:3 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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/36398#comment:5>

[Django]

#36398: select_for_update(of=...) ignores "self" when using values_list() when not
selecting a column from the model
-------------------------------------+-------------------------------------
Reporter: OutOfFocus4 | Owner:
| JakeWalson
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by OutOfFocus4):

* Attachment "models.py" added.

--
Ticket URL: <https://code.djangoproject.com/ticket/36398>

[Django]

#36398: select_for_update(of=...) ignores "self" when using values_list() when not
selecting a column from the model
-------------------------------------+-------------------------------------
Reporter: OutOfFocus4 | Owner:
| JakeWalson
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by OutOfFocus4):

* Attachment "tests.2.py" added.

[Django]

#36398: select_for_update(of=...) ignores "self" when using values_list() when not
selecting a column from the model
-------------------------------------+-------------------------------------
Reporter: OutOfFocus4 | Owner:
| JakeWalson
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by OutOfFocus4):

The issue is still present in Django 5.2.4.
--
Ticket URL: <https://code.djangoproject.com/ticket/36398#comment:6>

[Django]

#36398: select_for_update(of=...) ignores "self" when using values_list() when not
selecting a column from the model
-------------------------------------+-------------------------------------
Reporter: OutOfFocus4 | Owner:
| JakeWalson
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by OutOfFocus4):

The issue is still present in Django 5.2.5.
--
Ticket URL: <https://code.djangoproject.com/ticket/36398#comment:7>

[Django]

#36398: select_for_update(of=...) ignores "self" when using values_list() when not
selecting a column from the model
-------------------------------------+-------------------------------------
Reporter: OutOfFocus4 | Owner:
| JakeWalson
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36398#comment:8>

[Django]

#36398: select_for_update(of=...) ignores "self" when using values_list() when not
selecting a column from the model
-------------------------------------+-------------------------------------
Reporter: OutOfFocus4 | Owner:
| JakeWalson
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by OutOfFocus4):

Replying to [comment:8 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:

{{{#!python
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36398#comment:9>

[Django]

#36398: select_for_update(of=...) ignores "self" when using values_list() when not
selecting a column from the model
-------------------------------------+-------------------------------------
Reporter: OutOfFocus4 | Owner:
| JakeWalson
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Shai Berger):

* cc: Shai Berger (added)

Comment:

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.

{{{#!diff
diff --git a/tests/select_for_update/tests.py
b/tests/select_for_update/tests.py
index 460e279770..f022b179be 100644
--- a/tests/select_for_update/tests.py
+++ b/tests/select_for_update/tests.py
@@ -284,13 +284,23 @@ class SelectForUpdateTests(TransactionTestCase):
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",
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36398#comment:10>