Only update modified fields
SmileyChris
waiting as a design decision. The latest patch is (apart from lack of
docs) ready for check-in in my opinion. Could we have a decision on
whether this is worthy?
Norman Harman
> Collin has put a lot of effort in to this ticket [1] which is still
> waiting as a design decision. The latest patch is (apart from lack of
> docs) ready for check-in in my opinion. Could we have a decision on
> whether this is worthy?
Nobody knows me, so my opinion isn't worth much. But, this would be good. Better would
be if it were transparent(properties or some other mechanism to notice when fields are set
and therefore dirty/need updating). I'm fairly surprised something like this isn't
already the default behavior.
njharman
Collin Grady
> Nobody knows me, so my opinion isn't worth much. But, this would be good. Better would
> be if it were transparent(properties or some other mechanism to notice when fields are set
> and therefore dirty/need updating). I'm fairly surprised something like this isn't
> already the default behavior.
The newer patches on the ticket do what you describe - transparently
track attribute changes and only include those which changed in the
updates :)
--
Collin Grady
The most important service rendered by the press is that of educating
people to approach printed matter with distrust.
Malcolm Tredinnick
See earlier threads for why transparent behaviour is not a very safe
idea. If you override save(), for example, it's going to be very easy to
end up with things not being saved. This is not common behaviour, so
asking somebody to at least pass in an extra param or two isn't going to
kill them.
Regards,
Malcolm
--
How many of you believe in telekinesis? Raise my hand...
http://www.pointy-stick.com/blog/
Collin Grady
> See earlier threads for why transparent behaviour is not a very safe
> idea. If you override save(), for example, it's going to be very easy to
> end up with things not being saved. This is not common behaviour, so
> asking somebody to at least pass in an extra param or two isn't going to
> kill them.
Actually in the previous thread you only seem to have mentioned
overriding __setattr__ :)
However, I'm sure the issue could be worked around (even though it seems
pretty uncommon) - for instance, having __setattr__ create the dirty set
when it doesn't exist, and only have the partial updates kick in if it
does - so if you were to replace __setattr__ and didn't call the model's
version, it would just revert to saving everything.
--
Collin Grady
I bought some used paint. It was in the shape of a house.
-- Steven Wright
Malcolm Tredinnick
> Malcolm Tredinnick said the following:
> > See earlier threads for why transparent behaviour is not a very safe
> > idea. If you override save(), for example, it's going to be very easy to
> > end up with things not being saved. This is not common behaviour, so
> > asking somebody to at least pass in an extra param or two isn't going to
> > kill them.
>
> Actually in the previous thread you only seem to have mentioned
> overriding __setattr__ :)
save(), __setattr__, whatever. I was pointing out that there was an
existing problem that had already been raised in the archives, not doing
the original poster's research for him. :-)
The current latest patch fails in a dangerous way, which is why I'm
against it. Fail safely (so that my data always gets saved when I call
save()) and it's up for consideration. Performance considerations aside
(and having every assignment pass through __setattr__ isn't free, so we
need to look at that), if we were to include this features, making it as
easy to use as possible would be a plus. But not at the expense of
safety.
Regards,
Malcolm
--
If you think nobody cares, try missing a couple of payments.
http://www.pointy-stick.com/blog/
Collin Grady
> against it. Fail safely (so that my data always gets saved when I call
> save()) and it's up for consideration.
Is the failure you mention just the __setattr__ issue, or is there
another issue you haven't mentioned? :)
If that is what you're referring to, I'll get to work on fixing it :)
--
Collin Grady
Man's reach must exceed his grasp, for why else the heavens?
Malcolm Tredinnick
> Malcolm Tredinnick said the following:
> > The current latest patch fails in a dangerous way, which is why I'm
> > against it. Fail safely (so that my data always gets saved when I call
> > save()) and it's up for consideration.
>
> Is the failure you mention just the __setattr__ issue, or is there
> another issue you haven't mentioned? :)
It's only two methods in the current patch. How far wrong could you go
with that little code? Basically, all the points I mentioned in the
original quick review back on June 26 still hold (since I've spent
precisely three minutes thinking about this further since then).
Name reset_modified_fields() so that it's clear it's not part of the
public API (_reset_modified_fields() would be standard), since I can't
see that having any use outside of being an implementation detail. Can
you?
Does it need documentation?
What's the performance impact? Creating and, particularly, assigning to
model fields is something we do a fair bit, so it's on the popular
execution path. You really don't want people showing up at your front
door with pitchforks and flaming torches asking for a quiet word and an
explanation and would you mind stepping outside where it will make less
mess.
Regards,
Malcolm
--
The only substitute for good manners is fast reflexes.
http://www.pointy-stick.com/blog/
SmileyChris
wrote:
> The current latest patch fails in a dangerous way, which is why I'm
> against it. Fail safely (so that my data always gets saved when I call
> save()) and it's up for consideration.
> Does it need documentation?
Whether it needs documentation isn't usually a defining factor of the
design decision, is it?
> What's the performance impact? Creating and, particularly, assigning to
> model fields is something we do a fair bit, so it's on the popular
> execution path.
I have attached a quick performance ramifications of the current patch
to the ticket
http://code.djangoproject.com/ticket/4102#comment:7