Code

Opened 11 months ago

Closed 11 months ago

Last modified 11 months ago

#20988 closed Cleanup/optimization (fixed)

Model.save() refactoring incompatibilities

Reported by: akaariai Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Release blocker Keywords:
Cc: marti@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Ticket #16649 / commit 6b4834952dcce0db5cbc1534635c00ff8573a6d8 introduced a new algorithm for saving.

Previously when saving an object with primary key set, Django first did a SELECT to check if an row existed in the database. If it existed, an UPDATE was performed, otherwise an INSERT was performed. In Django 1.6 this is change so that a direct UPDATE is done. The database should return how many rows were found for the UPDATE. If no rows were found, then Django knows to perform an INSERT. If rows were found, then the UPDATE is already done. This way reduces one query in the common case where there is a matching row in the database.

However, there are some rare cases where the database doesn't return "row found" for UPDATE even if there is a matching row in the DB. One case is PostgreSQL and ON UPDATE trigger that returns NULL (returning NULL from ON UPDATE trigger skips the UPDATE operation). There are likely some other cases, at least when considering all the 3rd party backends.

When the database doesn't return "row found" for UPDATE, the result is that Django tries to insert and an IntegrityError follows. Thus, those users who are hit by the above condition simply cannot update existing objects. Django itself shouldn't ever generate schemas that have this problem, but again, who knows about third party backends.

It would be really nice to offer some fallback way for users who have the above problem. One possibility is introduced in https://github.com/akaariai/django/commit/1ca398b0352b11868ec236f92cf17b6ce82ff88c, that is Model.Meta option select_on_save that switches the model back to the old behaviour.

I think at least adding the "forced_update" argument to Model._do_update() before release candidate would be good. If it turns out later on that we want "select_on_save" flag, then there is no need to change the _do_update() signature mid stable release. Even if it is private API that would be nice for users. In addition, this allows those who have this problem to override _do_update(), and avoid the problem (private API override, but still better than "use raw SQL").

In addition release notes should be updated, the current note about this is too small, and isn't easy to understand if you don't already know what it means (same thing goes for .save() documentation). This can be done post RC1.

It is worth considering if adding the "select_on_save" flag should be done. A possibility is to wait-and-see, and add in mid 1.6 if enough complaints are risen.

I am marking this as release blocker, RC1 for the _do_update() signature change, and 1.6 release for the release notes change.

Attachments (0)

Change History (5)

comment:1 Changed 11 months ago by intgr

  • Cc marti@… added

comment:2 Changed 11 months ago by akaariai

Currently adding "select_on_save" flag is the favoured approach. This is discussed at https://groups.google.com/forum/#!topic/django-developers/coQ19f1VhTk.

There are pull requests for 1.6 and master for this at https://github.com/django/django/pull/1522 and https://github.com/django/django/pull/1523. They should be pretty much ready-for-checkin at the moment.

I will wait for more feedback for some time. Unless objections are risen I will commit this one by the end of this week.

comment:3 Changed 11 months ago by Anssi Kääriäinen <akaariai@…>

  • Resolution set to fixed
  • Status changed from new to closed

In e973ee6a9879969b8ae05bb7ff681172cc5386a5:

Fixed #20988 -- Added model meta option select_on_save

The option can be used to force pre 1.6 style SELECT on save behaviour.
This is needed in case the database returns zero updated rows even if
there is a matching row in the DB. One such case is PostgreSQL update
trigger that returns NULL.

Reviewed by Tim Graham.

Refs #16649

comment:4 Changed 11 months ago by Anssi Kääriäinen <akaariai@…>

In 76e38a21777243fec58c640d617bb71a251c5ba1:

[1.6.x] Fixed #20988 -- Added model meta option select_on_save

The option can be used to force pre 1.6 style SELECT on save behaviour.
This is needed in case the database returns zero updated rows even if
there is a matching row in the DB. One such case is PostgreSQL update
trigger that returns NULL.

Reviewed by Tim Graham.

Refs #16649

Backport of e973ee6a9879969b8ae05bb7ff681172cc5386a5 from master

Conflicts:

django/db/models/options.py
tests/basic/tests.py

comment:5 Changed 11 months ago by intgr

Thanks for the fix! :)

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.