[Django] #28344: Add `for_update=False` to `refresh_from_db()`

[Django]

#28344: Add `for_update=False` to `refresh_from_db()`
-------------------------------------+-------------------------------------
Reporter: Patryk | Owner: nobody
Zawadzki |
Type: New | Status: new
feature |
Component: Database | Version: 1.11
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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).

--
Ticket URL: <https://code.djangoproject.com/ticket/28344>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Tim Graham):

* stage: Unreviewed => Accepted

Comment:

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

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:1>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------

Reporter: Patryk Zawadzki | Owner: felixxm
Type: New feature | Status: assigned

Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by felixxm):

* status: new => assigned
* owner: nobody => felixxm

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:2>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: felixxm
Type: New feature | Status: assigned

Component: Database layer | Version: master

(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* version: 1.11 => master

Comment:

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.

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:3>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------

Reporter: Patryk Zawadzki | Owner: (none)

Type: New feature | Status: new

Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by felixxm):

* status: assigned => new
* owner: felixxm => (none)

Comment:

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

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:4>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

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

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:5>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (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?
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:6>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (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)?

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:7>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

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

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:8>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (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
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:9>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by adamhooper):

@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

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:10>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Description changed by Jure Erznožnik:

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

New description:

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 two
parameters:

* `atomic: bool=True`
* `auto-save: bool=True`

Would this be an acceptable solution?

--

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:11>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Description changed by Jure Erznožnik:

Old description:

> 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 two
> parameters:
>
> * `atomic: bool=True`
> * `auto-save: bool=True`
>
> Would this be an acceptable solution?

New description:

--

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:12>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (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 two
parameters:

* `atomic: bool=True`
* `auto-save: bool=True`

Would this be an acceptable solution?

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:13>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Description changed by Jure Erznožnik:

Old description:

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

--

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:14>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Raphael Michel):

* cc: Raphael Michel (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:15>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Semyon Pupkov):

* cc: Semyon Pupkov (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:16>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Andrey Maslov):

* cc: Andrey Maslov (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:17>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new

Component: Database layer | Version: dev

(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (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''...

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:18>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by jakedouglas):

ActiveRecord has `obj.with_lock { do stuff }` which I have used
extensively in a variety of applications. I would appreciate something
similar in Django.

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:19>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Alex Vandiver):

* cc: Alex Vandiver (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:20>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Aivars Kalvāns):

Replying to [comment:13 Jure Erznožnik]:

> {{{
> 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`

Can you elaborate on what those parameters would do? If I understand
`refresh_from_db` correctly - it opens the door to the bug mentioned in
comment:10.

{{{
foo = Foo.objects.filter(...).get()
...
Foo.objects.select_for_update.get(pk=foo.pk)
}}}

it leads to a situation where the object `foo` differs from the now locked
database record. That's why I would like to have a single operation to
lock the object and refresh the object with the latest "locked" values
from DB is valuable. I'm all for something like `refresh_from_db` or
`refresh_for_update` to avoid programming errors and reduce number of
database operations at the same time.

That said, I have objections to RoR way of `obj.lock_for_update` or
`obj.with_lock`: it implies that the lock will be released once the block
ends, but that is not the case. Once locked, the record will be locked
until the main transaction is committed or rolled back. Nested
transactions (savepoints) may release locks when rolled back, but it
differs between databases: Oracle does not release locks, but Postgres
does (IMHO). It looks nice, but it gives the wrong impression of how it
works.

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:21>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Aivars Kalvāns):

* cc: Aivars Kalvāns (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:22>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Aivars Kalvāns):

{{{
with transaction.atomic():
a = Account.objects.get(id=1)
with transaction.atomic():
Account.objects.select_for_update().get(id=a.pk)
a.refresh_from_db()
# The lock is not released here
# The lock is released here
}}}

For `obj.with_lock` to match what is happening in the database we would
have to ensure it's called outside transaction but that makes it useless
when we have to acquire more than one lock

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:23>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Aivars Kalvāns):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:24>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Aivars Kalvāns):

Replying to [comment:3 Simon Charette]:

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

I think most of those parameters do not apply to operations on the
specific model:

`of` when multiple models have to be locked, a QuerySet should be the
right choice

`skip_locked` does not make sense when working with one specific instance
of Model (primary key)

`nowait` might be interesting - something similar to
`threading.Lock.acquire(blocking=False)`. But unlike `acquire` which
returns a boolean, `select_for_update` raises `DatabaseError`. Raising an
exception from `refresh_from_db` would suggest the object is in an invalid
state. Code that uses the object after the exception would raise eyebrows
during code review. Making `refresh_from_db` return a boolean indicating
if it succeeded or not could also lead to issues because people are not
used to checking the result of `refresh_from_db`.

So I think the default `select_for_update` is the only right solution.
Adding `nowait` might be an option together with a new dedicated method
`refresh_for_update` but I haven't seen use for that in my domain.

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:25>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* cc: Simon Charette (added)

Comment:

> of when multiple models have to be locked, a QuerySet should be the
right choice

What about MTI when a single model involves multiple tables which is
something `refresh_from_db(fields)` supports? MTI is the main reason `of`
[https://docs.djangoproject.com/en/5.0/ref/models/querysets/#select-for-
update was added in the first place] so isn't incoherent that
`refresh_from_db(fields)` supports inherited fields but
`refresh_from_db(for_update)` doesn't?

> `skip_locked` does not make sense when working with one specific
instance of Model (primary key)

That I agree with.

> So I think the default select_for_update is the only right solution.

I'm still struggling to see the appeal for adding this option to be
honest. The reported use case is to handle calls that pass model instance
and want to lock the object for the duration of the transaction. Thing is
`refresh_from_db` overwrites any attributes that might have been set on
the objet passed by reference and results in the creation of a new
temporary model instance anyway so what are the benefits over simply doing
a queryset lookup with `select_for_update` by the primary key and doing a
few attribute assignments?

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:26>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Aivars Kalvāns):

Replying to [comment:26 Simon Charette]:

> > So I think the default select_for_update is the only right solution.
>
> I'm still struggling to see the appeal for adding this option to be
honest. The reported use case is to handle calls that pass model instance
and want to lock the object for the duration of the transaction. Thing is
`refresh_from_db` overwrites any attributes that might have been set on
the objet passed by reference and results in the creation of a new
temporary model instance anyway so what are the benefits over simply doing
a queryset lookup with `select_for_update` by the primary key and doing a
few attribute assignments?

I think all this applies to `refresh_from_db` regardless of locking or
not. I don't know what pros and cons were discussed before adding
`refresh_from_db`. Nothing prevents us from assigning attributes after
lookup by the primary key but `refresh_from_db` makes it easier when we do
not care about previous values.

The locking part gets a bit more annoying in the financial domain because
it's a common approach to optimize for concurrency: first reading models
with all related models and performing status and conditions checks and
locking models just before modifications. I also write entry journals and
audit trails before doing updates and rely on rollbacks to clean that up
if updates fail.

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:27>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Description changed by Raphael Michel:

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

New description:

To provide some additional perspective with some real-world use cases:

I agree that there is no use case for `skip_locked` here, and for `of`
only with model inheritance (which I never used). I do see a theoretical
use-case for `nowait`, but I have never come across wanting it in real
code.

I have come across wanting a `refresh_for_update`-style thing as discussed
here many times, though. As others mentioned, usually in relation with an
encapsulated piece of business logic that gets passed an instance and
requires locking. Sure, the business logic could just acquire a new
version of that instance, but then the code calling the business logic
needs to know that the instance it passed into the business logic no
longer is a valid version and *also* needs to refresh their instance,
which is not an intuitive interface.

In at least some situation, I even built my own little `refresh_from_db`
within my own code, which certainly is not nice:

https://github.com/pretix/pretix/blob/40cdb0c5077ae34b56e0263cfcf87c3558a1d042/src/pretix/base/models/orders.py#L1797-L1814

So I'm +1 for adding *something* that helps with this, and I'd personally
think a new `refresh_for_update(using, fields, of, nowait)` function is
the clearest and most future-safe design.

--

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:28>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Description changed by Raphael Michel:

Old description:

> To provide some additional perspective with some real-world use cases:

>
> I agree that there is no use case for `skip_locked` here, and for `of`
> only with model inheritance (which I never used). I do see a theoretical
> use-case for `nowait`, but I have never come across wanting it in real
> code.
>
> I have come across wanting a `refresh_for_update`-style thing as
> discussed here many times, though. As others mentioned, usually in
> relation with an encapsulated piece of business logic that gets passed an
> instance and requires locking. Sure, the business logic could just
> acquire a new version of that instance, but then the code calling the
> business logic needs to know that the instance it passed into the
> business logic no longer is a valid version and *also* needs to refresh
> their instance, which is not an intuitive interface.
>
> In at least some situation, I even built my own little `refresh_from_db`
> within my own code, which certainly is not nice:
>
> https://github.com/pretix/pretix/blob/40cdb0c5077ae34b56e0263cfcf87c3558a1d042/src/pretix/base/models/orders.py#L1797-L1814
>
> So I'm +1 for adding *something* that helps with this, and I'd personally
> think a new `refresh_for_update(using, fields, of, nowait)` function is
> the clearest and most future-safe design.

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

--

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:29>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Raphael Michel):

To provide some additional perspective with some real-world use cases:

I agree that there is no use case for `skip_locked` here, and for `of`
only with model inheritance (which I never used). I do see a theoretical
use-case for `nowait`, but I have never come across wanting it in real
code.

I have come across wanting a `refresh_for_update`-style thing as discussed
here many times, though. As others mentioned, usually in relation with an
encapsulated piece of business logic that gets passed an instance and
requires locking. Sure, the business logic could just acquire a new
version of that instance, but then the code calling the business logic
needs to know that the instance it passed into the business logic no
longer is a valid version and *also* needs to refresh their instance,
which is not an intuitive interface.

In at least some situation, I even built my own little `refresh_from_db`
within my own code, which certainly is not nice:

https://github.com/pretix/pretix/blob/40cdb0c5077ae34b56e0263cfcf87c3558a1d042/src/pretix/base/models/orders.py#L1797-L1814

So I'm +1 for adding *something* that helps with this, and I'd personally
think a new `refresh_for_update(using, fields, of, nowait)` function is
the clearest and most future-safe design.

(Sorry for first updating the description instead of adding a commend, I
have not used trac in a while and used the wrong text box. I reverted it
immediately)

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:30>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

Thanks for chiming in everyone.

So given the main benefits of `refresh_from_db` is about the encapsulation
of object fetching and attribute assignment and the main concerns are
about choosing an interface that isn't too limiting giving the large
number of options that `QuerySet` has could an acceptable solution be that
instead of focusing on `select_for_update` we'd allow a `from_queryset:
QuerySet` to be passed to the method and would be used instead of
`self.__class__._base_manager` when provided.

That would allow

{{{#!python
queryset = Author.objects.select_for_update()
author.refresh_from_db(from_queryset=queryset)
}}}

while allowing any necessary option to be passed to `select_for_update`
but also allow `refresh_from_db` to respect other criteria such as soft-
deletion

{{{#!python
queryset = Author.objects.all() # As opposed to `_base_manager` the
default manager could be doing `filter(deleted_at=None)`
author.refresh_from_db(from_queryset=queryset)
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:31>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Aivars Kalvāns):

Replying to [comment:31 Simon Charette]:

> Thanks for chiming in everyone.
>

> could an acceptable solution be that instead of focusing on
`select_for_update` we'd allow a `from_queryset: QuerySet` to be passed to
the method and would be used instead of `self.__class__._base_manager`
when provided.
>

That would do it but I started thinking about what other possibilities it
opens. Current parameters `using` and `fields` can be applied directly to
the queryset and the whole function would be `refresh_from_queryset`
instead of `refresh_from_db`.

{{{
queryset = Author.objects.using('default').only(*fields)
author.refresh_from_queryset(queryset)
or
author.refresh_from_db(from_queryset=queryset)
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:32>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

I don't think it would be a problem for `refresh_from_db` to apply fields
and `using` if explicitly provided with `from_queryset`

{{{#!diff
diff --git a/django/db/models/base.py b/django/db/models/base.py
index 6eaa600f10..8a9cbd606e 100644
--- a/django/db/models/base.py
+++ b/django/db/models/base.py
@@ -672,7 +672,7 @@ def get_deferred_fields(self):
if f.attname not in self.__dict__
}

- def refresh_from_db(self, using=None, fields=None):
+ def refresh_from_db(self, using=None, fields=None,
from_queryset=None):
"""
Reload field values from the database.

@@ -704,10 +704,15 @@ def refresh_from_db(self, using=None, fields=None):
"are not allowed in fields." % LOOKUP_SEP
)

- hints = {"instance": self}
- db_instance_qs = self.__class__._base_manager.db_manager(
- using, hints=hints
- ).filter(pk=self.pk)
+ if from_queryset is not None:
+ if using is not None:
+ from_queryset = from_queryset.using(using)
+ else:
+ hints = {"instance": self}
+ from_queryset = self.__class__._base_manager.db_manager(
+ using, hints=hints
+ )
+ db_instance_qs = from_queryset.filter(pk=self.pk)

# Use provided fields, if not set then reload all non-deferred
fields.
deferred_fields = self.get_deferred_fields()
}}}

That would mean that

{{{#!python
refresh_from_db(using='other',
from_queryset=Author.objects.using('default')) # would use `other`
refresh_from_db(fields=['first_name'],
from_queryset=Author.objects.only('last_name')) # would use ['first_name']

Why do you think the addition of a `refresh_from_queryset` method is
warranted?

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:33>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Aivars Kalvāns):

I'm ok with `refresh_from_db`, I was thinking out loud that the
possibility of passing a queryset makes existing parameters redundant.

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:34>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Aivars Kalvāns):

Just another idea: using `from_queryset` allows us to specify
`select_related` on the queryset and with a couple of lines in
`refresh_from_db` we could make it work and avoid extra queries when
accessing related fields later.

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:35>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

> I'm ok with refresh_from_db, I was thinking out loud that the

possibility of passing a queryset makes existing parameters redundant.

Makes sense, we do have similar APIs where some flags are redundant with
others so I don't think it's a huge deal.

> Just another idea: using from_queryset allows us to specify
select_related on the queryset and with a couple of lines in
refresh_from_db we could make it work and avoid extra queries when
accessing related fields later.

yeah it seems like it opens more doors and embrace the facilitation of
attribute assignment pattern better.

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:36>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Aivars Kalvāns):

I updated the PR

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:37>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------

Reporter: Patryk Zawadzki | Owner: Aivars
| Kalvāns
Type: New feature | Status: assigned

Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* owner: (none) => Aivars Kalvāns
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:38>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: Aivars
| Kalvāns
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:39>

[Django]

#28344: Add for_update parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: Aivars
| Kalvāns
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Aivars Kalvāns):

* needs_better_patch: 1 => 0

Comment:

Solved all review comments

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:40>

[Django]

#28344: Add from_queryset parameter to Model.refresh_from_db()

-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: Aivars
| Kalvāns
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:41>

[Django]

#28344: Add from_queryset parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: Aivars
| Kalvāns
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:42>

[Django]

#28344: Add from_queryset parameter to Model.refresh_from_db()
-------------------------------------+-------------------------------------
Reporter: Patryk Zawadzki | Owner: Aivars
| Kalvāns

Type: New feature | Status: closed

Component: Database layer | Version: dev
(models, ORM) |

Severity: Normal | Resolution: fixed

Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak <felisiak.mariusz@…>):

* status: assigned => closed
* resolution: => fixed

Comment:

In [changeset:"f92641a636a8cb75fc9851396cef4345510a4b52" f92641a6]:
{{{
#!CommitTicketReference repository=""
revision="f92641a636a8cb75fc9851396cef4345510a4b52"
Fixed #28344 -- Allowed customizing queryset in
Model.refresh_from_db()/arefresh_from_db().

The from_queryset parameter can be used to:
- use a custom Manager
- lock the row until the end of transaction
- select additional related objects
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/28344#comment:43>