Opened 6 months ago

Last modified 3 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: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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 (9)

comment:1 Changed 6 months ago by Tim Graham

Summary: Add `for_update=False` to `refresh_from_db()`Add for_update parameter to Model.refresh_from_db()
Triage Stage: UnreviewedAccepted

Looks okay at first glance. Is the implementation fairly simple?

comment:2 Changed 6 months ago by felixxm

Owner: changed from nobody to felixxm
Status: newassigned

comment:3 Changed 6 months ago by Simon Charette

Version: 1.11master

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 months ago by felixxm

Owner: felixxm deleted
Status: assignednew

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 months ago by Patryk Zawadzki

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 3 months ago by Hugo Osvaldo Barrera

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 3 months ago by Patryk Zawadzki

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 3 months ago by Hugo Osvaldo Barrera

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 3 months ago by Patryk Zawadzki

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
Note: See TracTickets for help on using tickets.
Back to Top