#34462 closed Cleanup/optimization (fixed)
Deletions in admin panel create N + 1 queries
Reported by: | Mike Lissner | Owned by: | Akash Kumar Sen |
---|---|---|---|
Component: | contrib.admin | Version: | 3.2 |
Severity: | Normal | Keywords: | performance |
Cc: | David Wobrock | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description
Sentry has a new feature that identifies when your app has an N+1 query:
https://docs.sentry.io/product/issues/issue-details/performance-issues/n-one-queries/
(I'll pause here to note my love of Sentry.)
This new feature just identified that when you delete multiple items from the admin panel, Django does an N+1 query for each of them, so if you delete 100 items, as I just did, it generates 100 queries to your database. The queries are small, but this isn't great.
According to Sentry, the queries it generates are of the form:
INSERT INTO "django_admin_log" ("action_time", "user_id", "content_type_id", "object_id", "object_repr", "action_flag", "change_message") VALUES (%s, %s, %s, %s, %s, %s, %s) RETURNING "django_admin_log"."id"
I think a bulk_insert would fix this, right?
I don't have time to tackle this, but I thought I'd at least report it. Do we care?
I can share the Sentry issue with anybody that wants to tackle this. So we can find it later, it's BIGCASES2-V in our system.
Change History (21)
comment:1 by , 17 months ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 17 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 17 months ago
Has patch: | set |
---|---|
Needs documentation: | set |
Needs tests: | set |
Patch needs improvement: | set |
comment:4 by , 17 months ago
Has patch: | unset |
---|---|
Needs documentation: | unset |
Needs tests: | unset |
Patch needs improvement: | unset |
The proposed PR was targetting the wrong branch, I closed it with an explanatory comment and further guidance on needed tests and docs. I'm unsetting all the flags to allow for the new work to be considered independently.
comment:6 by , 17 months ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
comment:8 by , 17 months ago
Needs documentation: | set |
---|
comment:11 by , 16 months ago
Akash the mergers are busy so a little patience is required. They have a list of PRs that they work through and yours is on the list ;)
comment:12 by , 16 months ago
Needs documentation: | unset |
---|---|
Patch needs improvement: | unset |
comment:13 by , 15 months ago
Cc: | added |
---|---|
Needs tests: | set |
comment:14 by , 15 months ago
Needs tests: | unset |
---|
Test added at : New testcase
Getting some weird and untraceable failures, some review will be helpful: PR
comment:15 by , 11 months ago
Needs documentation: | set |
---|---|
Patch needs improvement: | set |
comment:16 by , 11 months ago
Needs documentation: | unset |
---|---|
Patch needs improvement: | unset |
comment:17 by , 11 months ago
Patch needs improvement: | set |
---|
comment:18 by , 11 months ago
Patch needs improvement: | unset |
---|
comment:20 by , 10 months ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:21 by , 10 months ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Fixed in 40b3975e7d3e1464a733c69171ad7d38f8814280.
In order to solve this issue we'd have to introduce a new
ModelAdmin.log_deletions
method aslog_deletion
only allows a single object to be passed which would require adding aLogEntryManager.log_actions
method. If we're going forward with this change we should also make something for the changelist form view that relies onlog_addition
andlog_change
. Any bypass oflog_deletion
and friends for newly introduced bulk logging methods should be documented in release notes and we should also possibly keep calling into the old method for a deprecation period if they are overridden in subclasses.The
delete_selected
action has another issue that go beyond the one reported here. It issues acount()
query and then issues a follow up query that retrieve each objects which ultimately makescount()
unnecessary sincelen(queryset)
can be used to reduce the number of queries but more importantly to report the accurate number of deleted objects. It's even a documented anti-pattern in our docs.