Opened 6 years ago
Last modified 6 months ago
#28344 new New feature
Add for_update parameter to Model.refresh_from_db()
Reported by: | Patryk Zawadzki | Owned by: | |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Raphael Michel, Semyon Pupkov, Andrey Maslov | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
I have a project where we deal with many financial operations that rely on transactions and row-level locking a lot to guard against race conditions. Unfortunately it's a common pattern for a function to accept an instance of model A, start an atomic block, fetch a new instance of the same row with select_for_update()
, do a change, call save()
and then refresh the original instance. It would all be easier if I could just call instance.refresh_from_db(for_update=True)
and it would save a me a DB roundtrip to refresh the original instance passed (Django ORM does not have a session manager so the old object will not otherwise reflect the changes).
Change History (18)
comment:1 Changed 6 years ago by
Summary: | Add `for_update=False` to `refresh_from_db()` → Add for_update parameter to Model.refresh_from_db() |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:2 Changed 6 years ago by
Owner: | changed from nobody to Mariusz Felisiak |
---|---|
Status: | new → assigned |
comment:3 Changed 6 years ago by
Version: | 1.11 → master |
---|
Count me -0 on adding this given all the available select_for_update()
options. I fear we might open a can of worms here and have to either say no to nowait
, skip_locked
and now of
support in the future or allow for_update
to be a dict delegating kwargs to the underlying select_for_update()
call.
From my personal experience using select_for_update()
with refresh_from_db()
is not a common enough pattern to warrant the addition of this kwarg. Just me two cents.
comment:4 Changed 6 years ago by
Owner: | Mariusz Felisiak deleted |
---|---|
Status: | assigned → new |
The basic implementation is fairly simple, but I also share concerns about full (with all options) support for select_for_update()
with refresh_from_db()
.
comment:5 Changed 6 years ago by
Would refresh_for_update
make more sense? My case is really common in domains where resources are either shared or have to be protected from race conditions.
comment:6 Changed 6 years ago by
I've had to deal with similar patterns many times (mostly, when avoiding race conditions is critical -- for example, when closing and charging an order).
However, I think I might prefer an alternative pattern for this; I'd very much prefer if a model instance can lock it's own row. Currently, I've to construct a single queryset like :
# Assume obj is a model instance: MyModel.objects.filter(pk=obj.pk).select_for_update() obj.some_attr = 13 obj.save()
A cleaner pattern for this sort of of thing would probably be something like:
with obj.lock_for_update(): obj.some_attr = 13 # Would this be auto-saved at the end of the CM's block?
comment:7 Changed 6 years ago by
I don't think a context manager would work as there is no way to unlock a row save for committing the transaction.
Also would self.lock_for_update()
be a refresh_from_db
in disguise or would you prefer for it not to update the instance (potentially overwriting any unsaved changes)?
comment:8 Changed 6 years ago by
If the transaction is written as I did in my example, you don't need to call refresh_from_db
, since you only ever keep the reference to one loaded instance.
I guess that saving when exiting the context manager would solve any doubts.
comment:9 Changed 6 years ago by
Hugo, I appreciate your example but in my case I am dealing with financial transactions where I absolutely need to keep a database audit trail. This means I can't just wrap everything in a large transaction and risk it getting rolled back. Instead I have a number of tiny atomic blocks that need to be placed with surgical precision. This means I have to fetch the same object multiple times for it to act as the transaction synchronization point (via SELECT FOR UPDATE
within each atomic block).
Maybe it would be easier to introduce transaction.atomic_with_locked(obj)
that would be the equivalent of:
with transaction.atomic(): Foo.objects.select_for_update.get(pk=foo.pk) # discarding the result is fine as the lock is now in place yield
comment:10 Changed 5 years ago by
@Patryk that code block you suggest has a bug.
foo = Foo.objects.get(id=1) # loads entire record (call these _old_ values) # Now, what happens here if somebody _else_ writes to foo? with transaction.atomic(): Foo.objects.select_for_update.get(pk=foo.pk) # selects and ignores _new_ values yield # Modifies foo ... but foo.save() will save the _old_ values
I think this goes to show how necessary this feature is. Until it comes, I'll use this workaround:
foo = Foo.objects.get(id=1) with transaction.atomic(): Foo.objects.select_for_update.get(pk=foo.pk) foo.reload_from_db() yield
... which selects the row three times
comment:11 Changed 5 years ago by
Description: | modified (diff) |
---|
comment:12 Changed 5 years ago by
Description: | modified (diff) |
---|
comment:13 Changed 5 years ago by
I'm investigating this ticket for possible implementation at PyCon UK sprints. Initial thoughts:
I like the pattern suggested above:
with obj.lock_for_update(): obj.some_attr = 13 # Would this be auto-saved at the end of the CM's block?
However, this one implies both a transaction and an auto-save to me. So my proposal would be to "decorate" the lock_for_update()
with three parameters:
atomic: bool=True
auto-save: bool=True
refresh_from_db: bool=True
Would this be an acceptable solution?
comment:14 Changed 5 years ago by
Description: | modified (diff) |
---|
comment:15 Changed 4 years ago by
Cc: | Raphael Michel added |
---|
comment:16 Changed 4 years ago by
Cc: | Semyon Pupkov added |
---|
comment:17 Changed 4 years ago by
Cc: | Andrey Maslov added |
---|
comment:18 Changed 6 months ago by
Sorry for commenting on a ticket update 3 years ago, but any plans on it? Honestly I don't think the trick-around is not a common enough pattern...
Looks okay at first glance. Is the implementation fairly simple?