select_for_update code snippet example selects for update outside of it's transaction

[Thomas Frössman]

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.

[Mariusz Felisiak]

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.

[Thomas Frössman]

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?

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.

[Mariusz Felisiak]

Replying to 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."

[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.