Add for_update parameter to Model.refresh_from_db()

[Patryk Zawadzki]

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

[Tim Graham]

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

[Simon Charette]

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.

[Mariusz Felisiak]

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

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

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

[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)?

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

[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

[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