Opened 11 months ago

Closed 4 months ago

Last modified 4 months ago

#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 Simon Charette, 11 months ago

Triage Stage: UnreviewedAccepted

In order to solve this issue we'd have to introduce a new ModelAdmin.log_deletions method as log_deletion only allows a single object to be passed which would require adding a LogEntryManager.log_actions method. If we're going forward with this change we should also make something for the changelist form view that relies on log_addition and log_change. Any bypass of log_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 a count() query and then issues a follow up query that retrieve each objects which ultimately makes count() unnecessary since len(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.

comment:2 by Akash Kumar Sen, 11 months ago

Owner: changed from nobody to Akash Kumar Sen
Status: newassigned

comment:3 by Natalia Bidart, 10 months ago

Has patch: set
Needs documentation: set
Needs tests: set
Patch needs improvement: set

comment:4 by Natalia Bidart, 10 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 David Sanders, 10 months ago

Has patch: set
Patch needs improvement: set

comment:7 by GitHub <noreply@…>, 10 months ago

In 851b6879:

Refs #34462 -- Fixed queryset antipattern when processing object deletion.

comment:8 by Mariusz Felisiak, 10 months ago

Needs documentation: set

comment:11 by David Sanders, 10 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 Akash Kumar Sen, 10 months ago

Needs documentation: unset
Patch needs improvement: unset

comment:13 by David Wobrock, 9 months ago

Cc: David Wobrock added
Needs tests: set

comment:14 by Akash Kumar Sen, 9 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 Mariusz Felisiak, 5 months ago

Needs documentation: set
Patch needs improvement: set

comment:16 by Akash Kumar Sen, 5 months ago

Needs documentation: unset
Patch needs improvement: unset

comment:17 by Mariusz Felisiak, 4 months ago

Patch needs improvement: set

comment:18 by Akash Kumar Sen, 4 months ago

Patch needs improvement: unset

comment:19 by Mariusz Felisiak <felisiak.mariusz@…>, 4 months ago

In 45e0c589:

Refs #34462 -- Moved ModelAdmin.log_deletion() test to a separate test case.

comment:20 by Mariusz Felisiak, 4 months ago

Triage Stage: AcceptedReady for checkin

comment:21 by Mariusz Felisiak, 4 months ago

Resolution: fixed
Status: assignedclosed

In 40b3975e:

Fixed #34462 -- Made admin log actions in bulk.

This also deprecates ModelAdmin.log_deletion() and
LogEntryManager.log_action().

Last edited 4 months ago by Tim Graham (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top