Code

Opened 7 years ago

Closed 7 years ago

#5030 closed (fixed)

Remove COUNT(*) aggregate from model.save() when determining whether row exists

Reported by: zigiDev@… Owned by: adrian
Component: Database layer (models, ORM) Version: master
Severity: Keywords: model save
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Currently, the save method of the Model object uses an aggregate function to determine if a row with the specified primary key already exists. However, we don't need to know the count just whether the row exists or not. An alternative is to just simply SELECT 1 so if the row exists it will return 1, and if not we get a null. On PostgreSQL the COUNT(*) version takes over twice as long as the SELECT 1 version (it's not that significant but it is unnecessary work). Patch attached.

Attachments (1)

aggregateDiff.diff (957 bytes) - added by zigiDev@… 7 years ago.
Patch

Download all attachments as: .zip

Change History (10)

Changed 7 years ago by zigiDev@…

Patch

comment:1 Changed 7 years ago by adrian

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Are you peeking over my shoulder or something? I *just* noticed this literally 5 minutes ago, when I was snooping around in base.py. Wow.

Yes, we should make this change.

comment:2 Changed 7 years ago by zigiDev@…

That is weird! :)

comment:3 Changed 7 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Accepted

Promoting to accepted because Adrian was obviously too shocked to do that himself. Just wanted to check, COUNT 1 works across all supported databases?

comment:4 Changed 7 years ago by adrian

The patch looks perfect, but I'm not in a position to test it against any database other than Postgres. Can somebody test this against all the other DBs?

comment:5 Changed 7 years ago by zigiDev@…

I've run the django test suite over both Sqlite and Postgres and all tests passed. However, I can't test MySQL, SQL Server or Oracle myself.

comment:6 Changed 7 years ago by scott@…

I ran the tests with a MySQL database against r5783. One test failed with and without the patch.

So the patch seems to work fine with MySQL.

comment:7 Changed 7 years ago by Simon G. <dev@…>

comment:8 Changed 7 years ago by Simon G. <dev@…>

  • Triage Stage changed from Accepted to Ready for checkin

Commenters to the above thread show that the SELECT 1 does work on Oracle and SQL Server and is more efficient. I've check the explain plans under SQlite and MySQL (both InnoDB and MyISAM tables) and SELECT 1 is more efficient on SQLite and exactly the same in MySQL.

comment:9 Changed 7 years ago by adrian

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

(In [5882]) Fixed #5030 -- Removed 'COUNT()' from Model.save() when determining whether a row exists. We're now using a SELECT 1 LIMIT 1 instead, as it's more efficient in some databases. Thanks, zigiDev@… and the various folks who verified this patch works

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.