[Django] #33970: select_for_update code snippet example selects for update outside of it's transaction
Django
transaction
-----------------------------------------+------------------------
Reporter: thomasf | Owner: nobody
Type: Bug | Status: new
Component: Documentation | Version: 4.0
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 |
-----------------------------------------+------------------------
The docs suggest this for select_for_update:
{{{
entries = Entry.objects.select_for_update().filter(author=request.user)
with transaction.atomic():
for entry in entries:
...
}}}
If a user does this under the default auto commit transaction
circumstances the select_for_update should happen inside the transaction,
not before it so it is probably best if the example follows real world
usage.
--
Ticket URL: <https://code.djangoproject.com/ticket/33970>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
transaction
Reporter: thomasf | Owner: nobody
Type: Bug | Status: closed
Component: Documentation | Version: 4.0
Severity: Normal | Resolution: invalid
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 Mariusz Felisiak):
* status: new => closed
* resolution: => invalid
Comment:
Querysets are lazy, so:
{{{
entries = Entry.objects.select_for_update().filter(author=request.user)
}}}
doesn't evaluate it. It's evaluated when we iterate over `entries`. Also,
`select_for_update()` raises `TransactionManagementError` when you'll try
to evaluate it outside of transaction.
--
Ticket URL: <https://code.djangoproject.com/ticket/33970#comment:1>
Django
transaction
-------------------------------+--------------------------------------
Reporter: thomasf | Owner: nobody
Type: Bug | Status: closed
Component: Documentation | Version: 4.0
Severity: Normal | Resolution: invalid
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 thomasf):
Right.. I've been writing too much pure SQL lately to pick up on that.
Even if this isn't a hard bug would it not still make sense to move the
select statement inside the transaction it logically belongs in the code
snippet?
--
Ticket URL: <https://code.djangoproject.com/ticket/33970#comment:2>
Django
transaction
Reporter: Thomas Frössman | Owner: nobody
Type: Bug | Status: closed
Component: Documentation | Version: 4.0
Severity: Normal | Resolution: invalid
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 Mariusz Felisiak):
Replying to [comment:2 Thomas Frössman]:
> Even if this isn't a hard bug would it not still make sense to move the
select statement inside the transaction it logically belongs in the code
snippet?
>
> I would probably request a change to that piece of code in a code review
99% of all use cases. In fact I did find code like this out in the wile
just now and the only reason I could track it down why someone would write
it like that is that the manual gives this example.
The code is correct. There is no need to create and to be in an atomic
transaction while querysets are prepared. I see no reason to change this
example, TBH. It's also explained in details in docs:
> ''"When the queryset is evaluated (`for entry in entries` in this case),
all matched entries will be locked until the end of the transaction block,
meaning that other transactions will be prevented from changing or
acquiring locks on them."''
--
Ticket URL: <https://code.djangoproject.com/ticket/33970#comment:3>
Django
transaction
---------------------------------+--------------------------------------
Reporter: Thomas Frössman | Owner: nobody
Type: Bug | Status: closed
Component: Documentation | Version: 4.0
Severity: Normal | Resolution: invalid
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 Thomas Frössman):
Djangos own test suite almost always does the select_for_update inside a
transaction.atomic even when in places where it isn't technically required
to.
I've had a quick read through them and might be that every instance of
select_for_update is inside a transaction.atomic except when the test is
about failing transaction error.
I see this as a clear indication that it is how code like that should look
as a "best practice".
This is an example from the tests that also doesn't require qs to be
defined bu
{{{
def test_ordered_select_for_update(self):
with transaction.atomic():
qs = Person.objects.filter(
id__in=Person.objects.order_by("-id").select_for_update()
)
self.assertIn("ORDER BY", str(qs.query))
}}}
But sure, do what you want, I won't disturb anymore.
--
Ticket URL: <https://code.djangoproject.com/ticket/33970#comment:4>