#16649: Models.save() refactoring: check updated rows to determine action
-------------------------------------+-------------------------------------
Reporter: akaariai | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: 1.3
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Design
Severity: Normal | decision needed
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
Sorry, the explanations above are a little confusing. Still another try:
{{{
s = SomeModel.objects.get(pk=someval)
s.somecol = someval
s.save()
}}}
Here save is implemented as SELECT - if not found INSERT - else UPDATE.
The SELECT here is redundant, we have the information that SELECT was just
done in model._state. So, we can do directly an UPDATE. If it happens so
that nothing is updated (likely because of concurrent delete) then we will
still do the INSERT and things work as expected.
I have done a
[https://github.com/akaariai/django/compare/model_save_refactor full
refactor] of model.save_base(), this includes 4 parts:
- What this ticket deals with - trying directly to UPDATE if the model
was fetched from the same DB we are saving to. Also assorted cleanup to
make this possible.
- Cleanup to proxy parents handling (the logic is somewhat ugly
currently)
- A bug fix for #17341 (do not commit transaction after every parent
model save)
- Splitting save_base into parts so that the saving logic is easier to
follow
Above, the bug fix is of course needed, and the proxy parents handling
cleanup is IMO also needed.
I can't see any downsides to trying UPDATE directly as done in the patch.
This should result in clear performance improvement in most cases. There
is a problem that we have documented very explicitly the sequence of SQL
commands .save() does but I don't think that documentation should be
considered part of the public API. So, docs update is needed.
The refactoring of .save_base() into parts is a stylistic question. I
surprisingly prefer the refactored way. Some examples of things that are
hard to see from the current writing of the code:
- Which signals are sent for which models
- The actual hard work is done in the if not meta.proxy: branch.
However, it is impossible that meta.proxy is True here. And it isn't
exactly easy to see this.
- The origin parameter is a bit weird - it is None except for the
outermost table save.
Still, I'd like to get a confirmation that others prefer the new writing,
too.
--
Ticket URL: <https://code.djangoproject.com/ticket/16649#comment:5>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to
django-updates+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
