I believe there's the opportunity for a subtle bug in applications if
Django moves from its current default behaviour of saving all fields to
saving only dirty fields. Values in a model instance can become "torn"
between two versions by two concurrent processes even though neither wanted
to do this.
Take this model:
class Photo(models.Model):
likes = models.IntegerField()
multiplier = models.IntegerField()
total = models.IntegerField()
Let's say total should always equal likes * multiplier .
Process 1 (which could be a view, management command, celery task, etc.)
increases a Photo's likes:
photo = Photo.objects.get(id=id)
photo.likes += 1
photo.total = photo.likes * photo.multiplier
photo.save()
Process 2 updates a Photo's multiplier:
photo = Photo.objects.get(id=id)
photo.multiplier = new_multiplier
photo.total = photo.likes * photo.multiplier
photo.save()
To avoid problems if these two processes run at once on the same Photo
instance,
transactions or row-level locking with select_for_update() should be used,
so they don't run actually in parallel. These aren't the default in Django
though (I wish ATOMIC_REQUESTS was on by default).
Assuming the application is not using either a transaction or
select_for_update(), current Django does still guarantee that either the
complete state after Process 1 or 2 is in the database, by always writing
back all the fields. That is, no Photo instance can have its total set to
anything but likes * multiplier , since Django's save() sends all fields in
a single UPDATE statement, which is always atomic. Sure there's a race
condition and the results of either process 1 or 2 can be lost, but still
there's a guarantee on the relationship between the values, which
downstream logic might rely on.
Moving to dirty field tracking would break this guarantee. This would
probably cause untold low-level breakage in applications where atomicity
has not been considered. Therefore it seems like it would be a breaking
change which is hard to communicate to users, a complicated situation.
On Wed, 30 Jan 2019 at 13:51, Josh Smeaton <josh.smea...@gmail.com> wrote:
> That's a +1 from me. I've certainly hit the bugs you've mentioned before,
> and I can't think of a good reason not to do dirty field tracking,
> especially if it were to be opt in.
>
> Bikeshedding a little bit, rather than having a global setting, I'd rather
> see an option on Options/Meta so that opt in is per-model. On a related
> note, I'd like to see some real world testing of any solution with packages
> that implement polymorphic models.
>
> On Tuesday, 29 January 2019 01:00:21 UTC+11, Daniel Tao wrote:
>>
>> Hi!
>>
>> This is my first post on this list. I recently left a comment on #27017
>> <https://code.djangoproject.com/ticket/27017#comment:3> requesting to
>> re-open the topic of only saving dirty fields in save(). Tim Graham
>> helpfully directed to #4102 <https://code.djangoproject.com/ticket/4102> and
>> advised that I make the proposal on the dev mailing list, so that's what
>> I'm doing :)
>>
>> I've gone through the history of #4102 and taken notes on the challenges
>> that arose when this was first attempted 12 years ago (!). The TL;DR is
>> that, while there were indeed quite a few complications, I don't see
>> anything that came up that should be considered a flat-out showstopper.
>> Rather, what happened was that at about the 69th comment
>> <https://code.djangoproject.com/ticket/4102#comment:69>, back in 2012,
>> the conversation shifted as Matt Long made this observation:
>>
>> what started as a simple opt-in feature request of adding a field
>>> white-list to the Model's save function morphed into a complicated dirty
>>> flag approach that obviously has many edge cases and performance
>>> implications given that this ticket has been open for 5 years now
>>
>>
>> From here it seems things progressed towards the current solution of
>> supporting the update_fields argument, and that's where things ended. I
>> would like to point out that Matt did *not* advocate for completely
>> abandoning all efforts to support dirty field tracking; to the contrary, in
>> the same comment he said this (emphasis mine):
>>
>> Clearly some people feel differently and favor the dirty flag approach
>>> for a more hands-off approach. As such, I propose adding support for *both
>>> methods*
>>
>>
>> With that in mind, I believe it's worth re-opening this discussion. For a
>> fairly lengthy justification, see my aforementioned comment on #27017
>> <https://code.djangoproject.com/ticket/27017#comment:3>. I'll copy the
>> effective TL;DR of the proposal here for convenience:
>>
>> In my opinion Django could make most code bases inherently more resilient
>>> against latent race conditions by implementing some form of dirty field
>>> tracking and effectively providing the functionality of update_fields
>>> automatically. I would like to propose a new setting, something like
>>> SAVE_UPDATE_DIRTY_FIELDS_ONLY, to change the ORM's default behavior so
>>> that calls to Model.save() only update the fields that have been set on
>>> the model instance. Naturally for backwards compatibility this setting
>>> would be False by default.
>>
>>
>> As for the concerns that were raised when this was first attempted, I
>> will now attempt to summarize what I found along with, in most cases, a bit
>> of editorializing from me.
>>
>> Performance
>>
>> The performance angle was first explored in a comment
>> <https://code.djangoproject.com/ticket/4102#comment:7> that said it
>> "doesn't look good" and provided some benchmarks showing a performance hit
>> from 0.17s to 2.64s for setting an attribute using the timeit
>> <https://docs.python.org/3/library/timeit.html> package. I didn't see
>> anyone point out that the timeit method defaults to executing code *a
>> million times*; so the throughput of the operation went from about 6
>> million to closer to 400 thousand times per second. (The percentage change
>> is indeed significant, but this doesn't *smell* like a potential
>> bottleneck.)
>>
>> It was noted in a couple
>> <https://code.djangoproject.com/ticket/4102#comment:14> places
>> <https://code.djangoproject.com/ticket/4102#comment:29> that it seems
>> potentially shortsighted to focus so much on the performance of getting and
>> setting attributes without taking into account the potential performance
>> *benefits* of executing smaller UPDATE statements that write fewer
>> columns. As far as I can tell, no one in the thread on #4102 actively
>> investigated the latter.
>>
>> Based on the unlikelihood of attribute setting representing a performance
>> bottleneck, and the lack of data for the performance impact of executing
>> smaller updates, I would consider the performance discussion largely a
>> distraction. Though I do think it's worth measuring the latter.
>>
>> Compatibility
>>
>> It was observed that the two approaches considered on the ticket
>> (overriding __setattr__ or defining custom property setters) would not
>> work with obj.__dict__.update
>> <https://code.djangoproject.com/ticket/4102#comment:12>, which is
>> apparently an optimization you can find prescribed on some blogs out int he
>> wild. This supports the premise that this behavior should be opt in, so
>> devs who are using incompatible techniques can stay away (unless/until
>> they've removed those optimizations from their code).
>>
>> I had already suggested putting this functionality behind a setting, so I
>> think we're good here.
>>
>> Correctness
>>
>> There were some bugs related to
>> <https://code.djangoproject.com/ticket/4102#comment:39> multi-table
>> inheritance <https://code.djangoproject.com/ticket/4102#comment:67>. It
>> appears these were both followed by patches with proposed fixes.
>>
>> Another bug was that fields updated in pre_save such as
>> DateTimeField(auto_now=True) were not being updated
>> <https://code.djangoproject.com/ticket/4102#comment:60>. There was a
>> patch to fix this as well.
>>
>> The point was raised that this approach poses a problem for mutable
>> field types <https://code.djangoproject.com/ticket/4102#comment:68> (e.g.,
>> ArrayField). Presumably one approach for addressing that would be the
>> one employed by django-save-the-change here
>> <https://github.com/karanlyons/django-save-the-change/blob/master/save_the_change/decorators.py#L121>
>> .
>>
>> One gets the sense from following the thread that, surely, there would be
>> more bugs discovered were someone to resurrect this work. However, again, I
>> don't see any showstoppers here. We're talking about an advanced framework
>> with a large set of features, and so naturally it's going to be difficult
>> to get this right while taking all supported use cases into account.
>>
>> Conclusion
>>
>> As I said up top, I think it's worth re-opening #27017, or creating a new
>> ticket for the behavior it was requesting (updating only dirty fields
>> automatically in save()) where future contributors might discuss
>> possible approaches and, with any luck, submit pull requests. I believe it
>> would be both possible and helpful, and I also *suspect* that the
>> process would go a bit more smoothly today, as the project has a huge suite
>> of tests to catch possible edge cases and provide greater confidence about
>> the correctness of any patches submitted.
>>
>> P.S. I'm a little worried that someone will now direct me to *another*
>> long thread I should read filled with even more reasons this idea caused
>> problems in the past. If so, no worries! I'll do it. I've already come this
>> far :)
>>
> --
> You received this message because you are subscribed to the Google Groups
> "Django developers (Contributions to Django itself)" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to django-developers+unsubscr...@googlegroups.com.
> To post to this group, send email to django-developers@googlegroups.com.
> Visit this group at https://groups.google.com/group/django-developers.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/django-developers/45cc1f85-06ad-4f65-866f-1454eb301c9d%40googlegroups.com
> <https://groups.google.com/d/msgid/django-developers/45cc1f85-06ad-4f65-866f-1454eb301c9d%40googlegroups.com?utm_medium=email&utm_source=footer>
> .
> For more options, visit https://groups.google.com/d/optout.
>
--
Adam
--
You received this message because you are subscribed to the Google Groups
"Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-developers/CAMyDDM2zQsrFN3HRo-e9pgMvujwwZCZOh2mfUweaGF%2BXFy38dQ%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
