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 Jure Erznožnik)

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 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 years ago by Mariusz Felisiak

Owner: changed from nobody to Mariusz Felisiak
Status: newassigned

comment:3 Changed 6 years 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 years ago by Mariusz Felisiak

Owner: Mariusz Felisiak 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 years 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 6 years 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 6 years 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 6 years 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 6 years 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

comment:10 Changed 5 years ago by Adam Hooper

@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 Jure Erznožnik

Description: modified (diff)

comment:12 Changed 5 years ago by Jure Erznožnik

Description: modified (diff)

comment:13 Changed 5 years ago by Jure Erznožnik

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?

Last edited 5 years ago by Jure Erznožnik (previous) (diff)

comment:14 Changed 5 years ago by Jure Erznožnik

Description: modified (diff)

comment:15 Changed 4 years ago by Raphael Michel

Cc: Raphael Michel added

comment:16 Changed 4 years ago by Semyon Pupkov

Cc: Semyon Pupkov added

comment:17 Changed 4 years ago by Andrey Maslov

Cc: Andrey Maslov added

comment:18 Changed 6 months ago by Alessio Fachechi

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

Note: See TracTickets for help on using tickets.
Back to Top