Opened 15 months ago
Closed 14 months ago
#34639 closed Bug (fixed)
MySQL 8.0 hangs indefinitely when using the admin search with a Foreign Key and Annotate
Reported by: | Nicolas Lupien | Owned by: | Simon Charette |
---|---|---|---|
Component: | contrib.admin | Version: | 4.2 |
Severity: | Normal | Keywords: | mysql |
Cc: | Simon Charette, Mariusz Felisiak, Natalia Bidart | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
We've moved to MySQL 8.0 in order to use Django 4.2 but our production system went down and we reverted to using MySQL 5.7 with Django 4.1. We've currently found a workaround that I'll add at the end of the bug report.
If we use the search function of the admin on model with a foreign key and we override ModelAdmin.get_queryset with annotate, the search freezes our database. It had the same effect on Google Cloud SQL and on a local docker image of MySQL 8.0 and it works fine on both environment when using MySQL 5.7.
The code:
models.py
class Organization(models.Model): name = models.CharField(max_length=255) class Member(models.Model): name = models.CharField(max_length=255) organization = models.ForeignKey(Organization, on_delete=models.CASCADE, null=True)
admin.py
class OrganizationAdmin(admin.ModelAdmin): search_fields = ["name", "member__name"] list_display = ["name", "member_count"] class Meta: model = models.Organization def get_queryset(self, request): return super().get_queryset(request).annotate(Count("member")) def member_count(self, instance): return instance.member__count
I found that the ChangeList applies the override to get_queryset containing the annotate multiple times making the query extremely expensive. Give only 500 members it goes through 125,000,000 (500 * 500 * 500) rows.
The workaround: If we override the ChangeList queryset, the call to annotate happens only once and the query is fine.
class CustomChangeList(ChangeList): def get_queryset(self, request): return super().get_queryset(request).annotate(Count("member")) class OrganizationAdmin(admin.ModelAdmin): search_fields = ["name", "member__name"] list_display = ["name", "member_count"] class Meta: model = models.Organization def member_count(self, instance): return instance.member__count def get_changelist(self, request, **kwargs): return CustomChangeList
I created a repo with more details and the complete steps to reproduce the issue: https://github.com/betaflag/django-sqlbugdemo
Change History (15)
comment:1 by , 15 months ago
Summary: | MySQL 8.0 hand indefinitely when using the admin search with a Foreign Key and Annotate → MySQL 8.0 hangs indefinitely when using the admin search with a Foreign Key and Annotate |
---|
comment:2 by , 15 months ago
Description: | modified (diff) |
---|
comment:3 by , 15 months ago
Description: | modified (diff) |
---|
comment:4 by , 15 months ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
comment:5 by , 15 months ago
It looks like this relates to #32682 (187118203197801c6cb72dc8b06b714b23b6dd3d).
The fact self.root_queryset
is used without stripping unused annotations (e.g. the Count
one) explains why the reporter is able to work around the issue by only annotating after the Exists
filter is added.
Adjusting SQLQuery.exists
to strip used annotation (like we did with SQLQuery.get_aggreation
in #28477 (59bea9efd2768102fc9d3aedda469502c218e9b7)) could be a possible way to generically optimize this problem away.
comment:6 by , 15 months ago
Resolution: | invalid |
---|---|
Status: | closed → new |
It's definitely a Django issue. The query before #32682 is small instantaneous, and now, it crashes my db because it's astronomically costly, with only 500 rows. On production with 50,000 row, it costs 1.25e+15. This is, like @simon said, only because Django is not stripping unused annotations when using the root_queryset. The resulting query does a lot of unnecessary calculation. You can see it with all the debug information in my repository that I joined previously https://github.com/betaflag/django-sqlbugdemo
comment:7 by , 15 months ago
Triage Stage: | Unreviewed → Accepted |
---|
Accepting based on the comment from Simon. Not marking it as release blocker since the potential regression was added in 3.2.1 (and afaik, 3.2 only receives security updates at this point, right?).
comment:8 by , 15 months ago
Looking at the generated SQL in more details it seems like the annotation is properly stripped but not the now unnecessary join that was required for it
SELECT `myapp_organization`.`id`, `myapp_organization`.`name`, COUNT(`myapp_member`.`id`) AS `member__count` FROM `myapp_organization` LEFT OUTER JOIN `myapp_member` ON ( `myapp_organization`.`id` = `myapp_member`.`organization_id` ) WHERE EXISTS( SELECT 1 AS `a` FROM `myapp_organization` U0 -- U1 is a completely unused alias now that COUNT(`U1`.`id`) is stripped! LEFT OUTER JOIN `myapp_member` U1 ON (U0.`id` = U1.`organization_id`) LEFT OUTER JOIN `myapp_member` U2 ON (U0.`id` = U2.`organization_id`) WHERE ( ( U0.`name` LIKE '%a%' OR U2.`name` LIKE '%a%' ) AND U0.`id` = (`myapp_organization`.`id`) ) -- Ditto with GROUP BY since aggregation is discarded GROUP BY U0.`id` ORDER BY NULL LIMIT 1 ) GROUP BY `myapp_organization`.`id` ORDER BY `myapp_organization`.`name` ASC, `myapp_organization`.`id` DESC;
That's something the ORM is not good at.
Since annotate
, filter
and friends are additive only (there's no API to discard of them) the ORM was never optimized to have an operation that strips unused JOINs when the select mask changes or filters are elided (e.g. they end up raising EmptyQuerySet
). This was identified when working on stripping unused annotations on aggregations in #28477 (59bea9efd2768102fc9d3aedda469502c218e9b7) and is relatively hard to solve.
What's interesting here is that prior to 187118203197801c6cb72dc8b06b714b23b6dd3d the reporter was likely not even getting the right results when searching because they were running into #2361 (notice the double left join against the multi-valued myapp_member
table) which means their member__count
returned the wrong values.
comment:9 by , 15 months ago
Cc: | added |
---|
Not sure how actionable the ticket is in its current form so I wonder how we should proceed here.
I can see a few paths forward:
- Create an optimization / ORM ticket to have
SQLQuery.exists
and.aggregate
prune unnecessary joins solely used by pruned annotations. It doesn't look like we have one already and this is desirable.
- Adjust #32433 (6307c3f1a123f5975c73b231e8ac4f115fd72c0d) so the
TypeError
is only raised whendistinct(fields)
is used (which was the actually reported issue) so #32682 (187118203197801c6cb72dc8b06b714b23b6dd3d) can be reverted.
- Don't attempt to remove duplicates when the queryset is meant to be used for deletion purposes as there is no value in doing so but that's easier said than done due with how things are currently structured. Here's how it could be achieved.
I think that 1. is desirable no matter what, and that we should choose a solution between 2. and 3. I have a slight preference for 2 given it relaxes an overly aggressive assertion instead of introducing a feature for the sole purpose of working around it.
comment:10 by , 15 months ago
Hello Simon,
I've been reading this ticket and the related tickets to try to provide a sensible advice. The ORM in general is something I'm not familiar with.
Regarding doing (1), and following your advice in terms of that being desirable and feasible, that sounds like a change we should pursue.
Then, I don't understand exactly what (2) involves: is it to remove the self.query.distinct or
from the added if guard in the commit? I fail to see how that is safe (in terms of the original report), since in the PR Mariusz commented Using distinct() with delete() sounds like a bad idea to me.
. Are we confident that allowing distinct().delete()
is safe?
Thanks!
comment:11 by , 14 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Hello Natalia,
In the case of (2), and as originally reported in #32433, calling delete()
after distinct()
is only potentially ambiguous when DISTINCT ON
is used (distinct(*fields)
) or when DISTINCT
is against a subset of fields that doesn't not include the primary key. That is the case because if the subset of fields the distinct grouping is applied on includes the primary key then it's not possible to reduce the set of included rows to a subset (the primary key is distinct for each rows by definition).
The delete
method already prevents its usage after a values()
call (source) so doing values(*subset_not_including_pk).distinct().delete()
is not allowed. This only leaves the distinct(*fields).delete()
case as potentially problematic as reported in #32433.
The patch for #32433 was overly strict though and disallowed the distinct().delete()
case which then forced the logic introduced by 187118203197801c6cb72dc8b06b714b23b6dd3d to address #32682.
I'll work on PR to demonstrate what I mean by the above to make it crystal clear.
comment:12 by , 14 months ago
Has patch: | set |
---|
Thanks for the report, however, as it stands, it's a support question. You're using very expensive query for every row, so it takes time. Please reopen the ticket if you can debug your issue and provide details about why and where Django is at fault. As far as I'm aware, if the generated query is the same, then it's not an issue in Django itself. I'd start from checking database indexes.