[Django] #21241: Optimize the query generated for admin changelist filters
Django
--------------------------------------+--------------------
Reporter: acdha | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
The admin changelist search implementation currently calls
queryset.filter() for each whitespace-separated term. This can cause the
ORM to generate repeated JOINs of the same table which is both a potential
performance problem and will actually cause MySQL to return an error once
you hit a system limit (~60 on the example I encountered).
The patch at https://github.com/django/django/pull/881 changes the logic
to combine the terms into a single filter() call and adds some tests.
--
Ticket URL: <https://code.djangoproject.com/ticket/21241>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: acdha | Owner: charettes
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: contrib.admin | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by charettes):
* status: new => assigned
* needs_better_patch: => 0
* needs_tests: => 0
* owner: nobody => charettes
* needs_docs: => 0
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/21241#comment:1>
Django
Reporter: acdha | Owner: charettes
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: contrib.admin | Resolution:
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/21241#comment:2>
Django
Reporter: acdha | Owner: charettes
Cleanup/optimization | Version: master
Component: contrib.admin | Resolution: fixed
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Simon Charette <charette.s@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"698dd82eee1cb83f51d4cd39546461bf76976b5e"]:
{{{
#!CommitTicketReference repository=""
revision="698dd82eee1cb83f51d4cd39546461bf76976b5e"
Fixed #21241 -- Avoid extraneous JOINs in admin changelist search.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21241#comment:3>
Django
Reporter: acdha | Owner: charettes
Type: | Status: closed
Cleanup/optimization | Version: master
Component: contrib.admin | Resolution: fixed
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by akaariai):
I am pretty sure the patch is bogus. For multivalued relation doing
.filter(cond1).filter(cond2) isn't equivalent to .filter(cond1, cond2).
The first one creates multiple joins on purpose. The reason for that is
that if you have structure like:
||id||parent_id||intval1||intval2||
||1||null||10||||
||2||1||20||25||
||3||1||30||35||
then doing
`Tree.objects.filter(child__intval1=20).filter(child__intval2=35)` will
result in a match - there is a children with intval1=20 and a children
with intval2=35 (it could be the same children, in this case it isn't).
But if you do `Tree.objects.filter(child__intval1=20, child__intval2=35)`
then the interpretation is that there must be *single* children with
intval1=20 and intval2=35 and that isn't true. (However,
`Tree.objects.filter(child__intval1=20, child__intval2=25)` would match as
there is single children with both values).
On SQL level this single instance versus multiple instances is implemented
as single join to childrens versus multiple joins to childrens. So, it is
intentional that multiple .filter() calls chained together generates
multiple joins, while pushing them all into single .filter() call
generates just one join.
This behaviour of the ORM API isn't easy to understand, and I guess most
users do not actually know of this... But still, that is how it works, and
changing how admin interprets the given filters in this case is a
backwards incompatible change.
The ORM API is documented in
https://docs.djangoproject.com/en/dev/topics/db/queries/#spanning-multi-
valued-relationships.
--
Ticket URL: <https://code.djangoproject.com/ticket/21241#comment:4>
Django
Reporter: acdha | Owner: charettes
Cleanup/optimization | Version: master
Component: contrib.admin | Resolution:
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by charettes):
* status: closed => new
* resolution: fixed =>
* stage: Ready for checkin => Accepted
Comment:
Since this is backward incompatible and both use cases are valid I'll just
revert this commit for now.
Maybe we could use the proposed code path when there's no multi-valued
relationships?
Might be worth writing a regression test case to make sure we're not
breaking the admin changelist behavior in the future.
--
Ticket URL: <https://code.djangoproject.com/ticket/21241#comment:5>
Django
Reporter: acdha | Owner: charettes
Cleanup/optimization | Version: master
Component: contrib.admin | Resolution: fixed
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"a8df8e34f9fc0219de73eabcb215d976e75a89ae"]:
{{{
#!CommitTicketReference repository=""
revision="a8df8e34f9fc0219de73eabcb215d976e75a89ae"
Revert "Fixed #21241 -- Avoid extraneous JOINs in admin changelist
search."
This reverts commit 698dd82eee1cb83f51d4cd39546461bf76976b5e.
The patch introduced a backward incompatible change.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21241#comment:6>
Django
Reporter: acdha | Owner: charettes
Cleanup/optimization | Version: master
Component: contrib.admin | Resolution:
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: fixed =>
--
Ticket URL: <https://code.djangoproject.com/ticket/21241#comment:7>
Django
Reporter: acdha | Owner: charettes
Type: | Status: new
Cleanup/optimization | Version: master
Component: contrib.admin | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
For single valued relationships doing single .filter() call and multiple
chained .filter() calls should both result in a single join (if it doesn't
actually to that then it is a bug in the ORM). There might be a
performance change due to less cloning for single .filter() call, but that
shouldn't be anything dramatic in common cases.
Tests for current behaviour would of course be good. If there is some easy
way to allow users to choose just one filter() call that would be good,
too.
--
Ticket URL: <https://code.djangoproject.com/ticket/21241#comment:8>
Django
Reporter: acdha | Owner: charettes
Type: | Status: new
Cleanup/optimization | Version: master
Component: contrib.admin | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by charettes):
@acdha can you confirm one of your `search_filters` reference a multi
valued relationship?
--
Ticket URL: <https://code.djangoproject.com/ticket/21241#comment:9>
Django
Reporter: acdha | Owner: charettes
Type: | Status: new
Cleanup/optimization | Version: master
Component: contrib.admin | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by acdha):
I would agree about pulling in one of the two test cases as there is
currently nothing which tests how multiple query terms are processed.
My production failure was indeed on an M2M field in search_filters, where
a user pasted in a sentence of text and got a 500 error because MySQL
limited JOINs to 61 tables. On second read, I see why the original
behaviour is technically correct if unfortunate.
--
Ticket URL: <https://code.djangoproject.com/ticket/21241#comment:10>
Django
Reporter: acdha | Owner: charettes
Type: | Status: new
Cleanup/optimization | Version: master
Component: contrib.admin | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/21241#comment:11>
Django
Reporter: Chris Adams | Owner: Simon
Type: | Charette
Cleanup/optimization | Status: closed
Component: contrib.admin | Version: dev
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* status: new => closed
* resolution: => duplicate
Comment:
Duplicate of #16063.
--
Ticket URL: <https://code.djangoproject.com/ticket/21241#comment:12>