Allow UPDATE of only specific fields in model.save()

I like this functionality for the stated performance reasons, but I can't help thinking that this could be done automatically by just looking for anything that's changed (although the tracking of before and after states may be more trouble than it's worth!).

If accepted, this would need a brief bit of documentation for database-api too (just a description and an example). Some tests would be good too.

Simon, tracking "before" vs. "after" states can't really be done automatically, since we would have to trap every attribute access to the class (which are, after all, just normal Python attribute accesses). Doing an extra database lookup to work out this information would also be bad.

I'm slightly in favour of this feature at the moment, but still thinking about it. I think it would need to be implemented with a manual list of the fields, as in Collin's patc if we did it.

Patch with unit tests and docs

I've uploaded a patch based on Collin's original (using only_fields instead of save_fields as the keyword argument -- this implies a possible except_fields keyword argument, or perhaps exclude=False|True which defaults to False), including unit tests and docs.

Should an exception be thrown if invalid field names are given in the list?

This came up again at work and it was mentioned that you should be able to override __setattr__ for the meta class to trap attribute changes and set a dirty flag for various columns, which could enable some automation of this.

initial shot at 'dirty' flag, seems to work so far

updated patch to only set dirty if the new value is different than current

fixed another bug - _dity check died if the field didn't exist but was being assigned to

Found another issue in the previous patches - if you created an object, for instance Poll(id=1, question='What color is the sky?') and then tried to save it, it would not recognize that you had modified those attributes.

Added a kwarg to __init__ to decide if the dirty flags should be washed at the end of __init__, and modified the model creation in query.py to use the flag, so that queries from the database are not flagged as dirty, but manual object creation can be done either way.

Updated patch against [5832], fixed bug with kwargs instantiation and dirty flags

converted patch to use set on suggestion of SmileyChris?, also found another bug when you used a kwargs of a field name instead of attname

removed keyword from __init__, added call to _wash in query.py

woops, removed old commented line

sigh, patch hates me - fixed duplicate test issue

This is looking nice and shiny. With some docs, it'd be ready for core IMO.

Cleaner implementation: doesn't mess with init

small fix (a del exception is an AttributeError, not a KeyError)

Malcolm asked about the performance impact. It doesn't look good. Here's a quick mock test I created:

>>> from timeit import Timer

# A control model
>>> setup_a = '''
class Model(object):
    pass
m = Model()
'''

# A model which has a __setattr__ which does nothing
>>> setup_b = '''
class Model(object):
    def __setattr__(self, name, value):
        super(Model, self).__setattr__(name, value)
m = Model()
'''

# A model which has a __setattr__ which matches this patch
>>> setup_c = '''
class Model(object):
    def __setattr__(self, name, value):
        if name != '_modified_attrs' and (not hasattr(self, name) or
                                          value != getattr(self, name)):
            if hasattr(self, '_modified_attrs'):
                if value not in self._modified_attrs: 
                    self._modified_attrs.add(name)
            else:
                self._modified_attrs = set((value,))
        super(Model, self).__setattr__(name, value)
m = Model()
'''

>>> Timer('m.an_attribute="test"', setup_a).timeit()
0.17214338693884201

>>> Timer('m.an_attribute="test"', setup_b).timeit()
1.6591357281431556

>>> Timer('m.an_attribute="test"', setup_c).timeit()
2.6405875607088092

small fix to the set creation - need to add name to the set, not the value

I would think by doing value comparisons on the data is a main cause for the overhead. To mark something dirty do it based on the setting of the attribute only and not based on what data is being changed. If I am having to update a model with a 3.4 MB chunk of data it will be comparing that data with the 3 MB that may already be in there. At that point I don't care about only updating the fields I change since it will take much more time than just updating it.

fixed another value/name mixup, optimized a few operations

More performance checks:

# A model which has a __setattr__ which *really* matches this patch
>>> setup_c = '''
class Model(object):
    def __setattr__(self, name, value):
        if name != '_modified_attrs':
            if hasattr(self, '_modified_attrs'):
                if name not in self._modified_attrs: 
                    self._modified_attrs.add(name)
            else:
                self._modified_attrs = set((name,))
        super(Model, self).__setattr__(name, value)
m = Model()
'''

# Optimized for speed, and dropped the value check as Brian suggested
>>> setup_d = '''
class Model(object):
    def __setattr__(self, name, value):
        try:
            if name not in self._modified_attrs: 
                self._modified_attrs.add(name)
        except AttributeError:
            if name != '_modified_attrs':
                self._modified_attrs = set((name,))
        super(Model, self).__setattr__(name, value)
m = Model()
'''

>>> Timer('m.an_attribute="test"', setup_a).timeit()
0.17163466306471914
>>> Timer('m.an_attribute="test"', setup_b).timeit()
1.7086492836380041
>>> Timer('m.an_attribute="test"', setup_c).timeit()
2.3102257409810463
>>> Timer('m.an_attribute="test"', setup_d).timeit()
1.9328342009948187

So setup_d is only 13% slower than a plain __setattr__ override... that's not bad...

(actually, I dropped the value check in setup_c too, so it's still more like 2.71 for the speed of a __setattr__ which matches the current patch)

Fixed duplicate tests due to svn glitch again

perf test script

Attached the earlier tests (slightly corrected) - it seems that the majority of the extra time used is merely from defining __setattr__, strangely enough...

Results of script, showing single vs double:

Single Assignment
Control:   0.29817199707
noop:      1.92452192307
patch:     3.07881999016
optimized: 2.29438686371

Double Assignment
Control:   0.298352003098
Noop:      1.92282295227
Patch:     3.08132410049
Optimized: 2.29805707932

The odd thing (to me at least) is that the set creation doesn't appear to take any more time than simply adding a value to the set (as shown by how the 2nd assignment takes just as long as the first).

If the performance difference caused by the __setattr__ stuff is too much, I think the original manual idea of being able to specify save/exclude fields in save() could be a decent compromise.

BTW the latest patch obviously is very backwards-incompatible for anyone using obj.dict.update() to update a lot of attributes at once, or using obj.dict.update(form.cleaned_data) as I've seen on a few blog posts.

new properties method, faster than noop :)

Replying to Collin Grady <cgrady@the-magi.us>:

The odd thing (to me at least) is that the set creation doesn't appear to take any more time than simply adding a value to the set (as shown by how the 2nd assignment takes just as long as the first).

It's because you were doing it in set up, not in the actual test!

Results of script:

Single Assignment
Control:   0.168
noop:      1.689 (10.0x)
patch:     2.581 (15.3x)
optimized: 1.872 (11.1x)
property:  1.494 (8.9x)

Double Assignment
Control:   0.368
noop:      3.400 (9.2x)
patch:     5.222 (14.2x)
optimized: 3.887 (10.6x)
property:  2.951 (8.0x)

"hyper-optimized" method, and showing the downfall of the properties method

Ok, so I've got the timing down to under 6x control.

I think it's also important to mention that the writing of an attribute isn't really where most of the time is going to be taken up anyway - it's not like this is slowing down model access or saving by any real measure.

Single Assignment
Control:   0.170
noop:      1.939 (11.4x)
patch:     2.566 (15.1x)
optimized: 1.852 (10.9x)
noop-h-op: 0.787 (4.6x)
h-optimiz: 0.988 (5.8x)
property:  1.473 (8.6x)
new-attr:  1.779 (10.4x)

Retreiving
Control:   0.139
noop:      0.137 (1.0x)
patch:     0.177 (1.3x)
optimized: 0.162 (1.2x)
noop-h-op: 0.136 (1.0x)
h-optimiz: 0.135 (1.0x)
property:  0.787 (5.7x)
new-attr:  0.992 (7.2x)

Latest version of the patch, reimplemented checking against the existing value after a discussion with Collin because otherwise user input like forms/etc is going to mark everything modified regardless

Two things: 1. while you are overloading setattr, you could also add an option to allow only defined fields to be set and rising exception on setting nonexistent field. This is a great debugging mechanism to weed out incidental typos which usually lead to quiet dataloss until you notice it at some time much too late.

2. Are you really sure you should optimize for form-update when saving on-change - and thus needing a compare? I don't really believe that is the case which people need this db performance advantage for... Usually save() gets performance-problematic when you want to change your data in bulk... and you just need to update a counter or a date field for thousands of records, but don't want to touch all the textfields

Replying to andraz@zemanta.com:

Two things: 1. while you are overloading setattr, you could also add an option to allow only defined fields to be set and rising exception on setting nonexistent field.

That is well outside of the scope of this ticket.

2. Are you really sure you should optimize for form-update when saving on-change - and thus needing a compare?

You're not being very clear, but I think you are asking why I re-implemented the change to check against existing values.

The point in that change isn't optimization but to help with the minor threading issues. And I think you may have overlooked the fact that it only does this comparison for fields when they are being set to ensure they really are "changing".

Why not just set self.db_values = db_values under save() and get() etc? Then compare current values to db_values during the loop that compiles the INSERT/UPDATEs?

It would likely be less efficient to store an entire second copy of all model data when you can simply store a modified flag. Especially if you had a large number of fields or fields with large amounts of data in them.

Sorry just read Brian's point, re the big blob data scenario. Agreed setattr overloading does sound better in that light, +1 :-)

Another point. Rather than suggest it should be kept _private, why not make it clearly available and show in the docs. It will be very useful for deciding when to update related tables or not, during save() overrides etc.

See this is ugly --

def save(self):
  if not self.id:
    retag = True
  else:
    # Unfortunately, we need to hit the db to see if things have changed
    check = Tag.objects.get(pk=self.id)
    retag = (check.name != self.name)
  super(Tag, self).save()
  if retag:
    self.retag_articles()

Be great to be able to do something like this --

def save(self):
  retag = ('name' in self.modified_fields)
  super(Tag, self).save()
  if retag:
    self.retag_articles()

I don't see how anything stops you from reading it like that already with this patch, it isn't a __ var, and you would then be internal to the model anyway, so it seems fine to me to read from it as is :)

Need to write a new patch

(22:09:40) Magus-: SmileyChris: oh, btw, your most recent patch for 4102 is busted, I forgot to talk to you about it earlier :)
(22:09:50) SmileyChris: oh noes!
(22:09:56) SmileyChris: Magus-: what did I bust?
(22:09:59) Magus-: you can't use self.__dict__ like that
(22:10:03) Magus-: it breaks on fkeys and other things
(22:10:09) Magus-: well, only fkeys so far

fixed glitch with foreignkeys

post-qsrf patch

You've obviously spent a lot of time investigating the performance overhead incurred by over-riding setattr, but have you done any benchmarking of the performance improvement gained by performing significantly smaller UPDATE statements? It may well be that the improvement from the more efficient UPDATEs makes the performance overhead of setattr irrelevant.

Patch against django 1.0. Removed duplicate test.

I've a table where some columns are read-only (have SELECT, but no UPDATE privilege on some columns).
Before that patch I was unable to modify an object and save() it, as save() tried to write all the fields, thus triggering an exception.
Now I can, thanks !!! (at the cost of enumerating in each save() all the fields I can/want update).
Maybe this 'read-only' attribute, per column basis, could be specified in the meta class.
Regards

Patch 4102.3 does not work correctly with multi-table inheritance -- child._modified_attrs is not visible by the parent, so a modified parent field is not updated. Something like this fixes it:

Index: third-party/Django-1.0.2-final+4102.3+2705/django/db/models/base.py =================================================================== --- third-party/Django-1.0.2-final+4102.3+2705/django/db/models/base.py (revision 3362) +++ third-party/Django-1.0.2-final+4102.3+2705/django/db/models/base.py (working copy) @@ -358,7 +358,10 @@

non_pks = [f for f in meta.local_fields if not f.primary_key] modified_attrs = self._modified_attrs non_pks = [f for f in non_pks if (f.name in modified_attrs or f.attname in modified_attrs)]

- self._reset_modified_attrs() + + # Reset modified attribute accumulator after parent fields have been updated + if cls == self.class: + self._reset_modified_attrs()

# First, try an UPDATE. If that doesn't update anything, do an INSERT. pk_val = self._get_pk_val(meta)

Patches Django-1.0.2 with patch 4102.3 applied to fix multi-table inheritance

Apologies for the formatting snafu above. I've uploaded the patch as attachment 4102.3.fix-multi-table.

patch for django1.1

If query at the same time use function 'select_related' and 'only', it will be wrong.

s = Station_Info.objects.select_related('player_info').only('name','player_infonickname').get(pk=1) s.name='test11' s.save()

Traceback (most recent call last):

File "<console>", line 1, in <module> File "/usr/lib/python2.6/dist-packages/django/db/models/base.py", line 420, in save

self.save_base(force_insert=force_insert, force_update=force_update)

File "/usr/lib/python2.6/dist-packages/django/db/models/base.py", line 462, in save_base

self.save_base(cls=parent, origin=org)

File "/usr/lib/python2.6/dist-packages/django/db/models/base.py", line 487, in save_base

rows = manager.filter(pk=pk_val)._update(values)

File "/usr/lib/python2.6/dist-packages/django/db/models/query.py", line 449, in _update

return query.execute_sql(None)

File "/usr/lib/python2.6/dist-packages/django/db/models/sql/subqueries.py", line 120, in execute_sql

cursor = super(UpdateQuery?, self).execute_sql(result_type)

File "/usr/lib/python2.6/dist-packages/django/db/models/sql/query.py", line 2385, in execute_sql

cursor.execute(sql, params)

File "/usr/lib/python2.6/dist-packages/django/db/backends/util.py", line 19, in execute

return self.cursor.execute(sql, params)

File "/usr/lib/python2.6/dist-packages/django/db/backends/mysql/base.py", line 89, in execute

raise Database.IntegrityError(tuple(e))

IntegrityError: (1048, "Column 'map_info_id' cannot be null")

Please don't mark tickets against the v1.1 milestone unless they are critical failures. New features are _definitely_ not for the milestone.

#12172 raised a side effect that should be a logical consequence of implementing this feature.