Opened 22 months ago

Closed 22 months ago

Last modified 22 months ago

#33970 closed Bug (invalid)

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

Reported by: Thomas Frössman Owned by: nobody
Component: Documentation Version: 4.0
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (4)

comment:1 by Mariusz Felisiak, 22 months ago

Resolution: invalid
Status: newclosed

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.

comment:2 by Thomas Frössman, 22 months ago

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.

Last edited 22 months ago by Thomas Frössman (previous) (diff)

in reply to:  2 comment:3 by Mariusz Felisiak, 22 months ago

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

comment:4 by Thomas Frössman, 22 months ago

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

But sure, do what you want, I won't disturb anymore.

Last edited 22 months ago by Thomas Frössman (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top