[Django] #29943: Slow admin chanelist query (because of adding `pk` to ordering)
Django
-----------------------------------------+------------------------
Reporter: Taha Jahangir | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
-----------------------------------------+------------------------
Consider a simple model with this definition for model and admin:
{{{
class MyModel(models.Model):
class Meta:
ordering = ('-created',)
created = models.DateTimeField(default=now, db_index=True)
message = models.CharField(max_length=20)
@admin.register(MyModel)
class MyModelAdmin(admin.ModelAdmin):
pass
}}}
We created a model with the **indexed** `created` field, and `ordering`
field set to it . It should works nicely. But if the tables go large, the
listing will be slow, because the generated query is like:
{{{
SELECT "myapp_mymodel"."id", "myapp_mymodel"."created",
"myapp_mymodel"."message" FROM "myapp_mymodel" ORDER BY
"myapp_mymodel"."created" DESC, "myapp_mymodel"."id" DESC LIMIT 100;
}}}
And the database (in my case, postgresql) **WILL NOT** use the `created`
index and the query becomes very slow.
I treat this as a bug, because the default behavior of admin module is not
sensible (and not documented), and will result in performance bug in
normal setups , and it cannot be changed in a simple manner (without
copying/monkey-patching of `ChangeList.get_ordering` method).
--
Ticket URL: <https://code.djangoproject.com/ticket/29943>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Taha Jahangir | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Description changed by Taha Jahangir:
Old description:
> Consider a simple model with this definition for model and admin:
>
> {{{
> class MyModel(models.Model):
> class Meta:
> ordering = ('-created',)
>
> created = models.DateTimeField(default=now, db_index=True)
> message = models.CharField(max_length=20)
>
> @admin.register(MyModel)
> class MyModelAdmin(admin.ModelAdmin):
> pass
> }}}
>
> We created a model with the **indexed** `created` field, and `ordering`
> field set to it . It should works nicely. But if the tables go large, the
> listing will be slow, because the generated query is like:
>
> {{{
> SELECT "myapp_mymodel"."id", "myapp_mymodel"."created",
> "myapp_mymodel"."message" FROM "myapp_mymodel" ORDER BY
> "myapp_mymodel"."created" DESC, "myapp_mymodel"."id" DESC LIMIT 100;
> }}}
>
> And the database (in my case, postgresql) **WILL NOT** use the `created`
> index and the query becomes very slow.
>
> I treat this as a bug, because the default behavior of admin module is
> not sensible (and not documented), and will result in performance bug in
> normal setups , and it cannot be changed in a simple manner (without
> copying/monkey-patching of `ChangeList.get_ordering` method).
New description:
Consider a simple model with this definition for model and admin:
{{{
class MyModel(models.Model):
class Meta:
ordering = ('-created',)
created = models.DateTimeField(default=now, db_index=True)
message = models.CharField(max_length=20)
@admin.register(MyModel)
class MyModelAdmin(admin.ModelAdmin):
pass
}}}
We created a model with the **indexed** `created` field, and `ordering`
field set to it . It should works nicely. But if the tables go large, the
listing will be slow, because the generated query is like:
{{{
SELECT "myapp_mymodel"."id", "myapp_mymodel"."created",
"myapp_mymodel"."message" FROM "myapp_mymodel" ORDER BY
"myapp_mymodel"."created" DESC, "myapp_mymodel"."id" DESC LIMIT 100;
}}}
And the database (in my case, postgresql) **WILL NOT** use the `created`
index and the query becomes very slow.
I treat this as a bug, because the default behavior of admin module is not
sensible (and not documented), and will result in performance bug in
normal setups , and it cannot be changed in a simple manner (without
copying/monkey-patching of `ChangeList.get_ordering` method).
A stackoverflow topic about this behavior:
https://stackoverflow.com/questions/32419190/django-admin-incorrectly-
adds-order-by-into-query
The issue (and commit) when this behavior is introduced (~7 years ago)
#17198 -- Ensured that a deterministic order is used across all database
backends
In reply to https://code.djangoproject.com/ticket/17198#comment:14 :
In our cases (a ~2M row table), the duration of `count(*)` query is
~300ms, but viewing the 2000th page (`LIMIT 100 OFFSET 200000`) is 8s!
--
--
Ticket URL: <https://code.djangoproject.com/ticket/29943#comment:1>
Django
Reporter: Taha Jahangir | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------+--------------------------------------
Changes (by Tim Graham):
* easy: 1 => 0
Comment:
Do you have a suggestion of what to do?
--
Ticket URL: <https://code.djangoproject.com/ticket/29943#comment:2>
Django
--------------------------------------+------------------------------------
Reporter: Taha Jahangir | Owner: nobody
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Changes (by Tim Graham):
* component: contrib.admin => Documentation
* type: Bug => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
Absent another proposal, the behavior could be documented.
--
Ticket URL: <https://code.djangoproject.com/ticket/29943#comment:3>
Django
--------------------------------------+------------------------------------
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Comment (by Simon Charette):
Taha, out of curiosity does adding a composite `Index(fields=('-created',
'id'))` [https://docs.djangoproject.com/en/2.1/ref/models/indexes/#index-
options index] instead of using `db_index=True` helps anyhow? It shouldn't
be significantly larger than the `created` index `db_index=True` created
and provide a total ordering on your table.
--
Ticket URL: <https://code.djangoproject.com/ticket/29943#comment:4>
Django
--------------------------------------+------------------------------------
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Comment (by Taha Jahangir):
There are two proposals for this issue:
1) Adding a variable (perhaps a property on `ModelAdmin`) to easily
enable/disable this behavior.
2) If we think in a larger scope, django-admin could have a **huge-table
mode**. There are also other issues with large tables (like querying for
distinct values in filters or displaying a long list of related items when
filtering by foreign-key)
--
Ticket URL: <https://code.djangoproject.com/ticket/29943#comment:5>
Django
--------------------------------------+------------------------------------
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Comment (by Carlton Gibson):
Quickly researching this suggested that the answer is to use SQL which
uses a `> pk` rather than an `OFFSET pk` (since the `OFFSET` requires
reading every proceeding row before skipping it). As such, Simon's
suggestion above has to be the first option to explore: if adding the
index allows the ORM to generate the `> pk` query then job done. (We could
add documentation advising adding such an index.)
--
Ticket URL: <https://code.djangoproject.com/ticket/29943#comment:6>
Django
Reporter: Taha Jahangir | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: assigned
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Hasan Ramezani):
* owner: nobody => Hasan Ramezani
* status: new => assigned
* has_patch: 0 => 1
Comment:
[https://code.djangoproject.com/ticket/29943 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/29943#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Taha Jahangir | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: assigned
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
Ref PR: The suggestion to index both (e.g.) `-created` and `id` was made
here and on #17198#comment#2, so should probably be mentioned in any
documentation fix.
--
Ticket URL: <https://code.djangoproject.com/ticket/29943#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Taha Jahangir | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: assigned
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
Comment:
the comment from the mentioned ticket added to note.
I also checked the override get_ordering(), so the behavior doesn't
change.
--
Ticket URL: <https://code.djangoproject.com/ticket/29943#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Taha Jahangir | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: assigned
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
Patch needs updating to reflect changes in
f84ad16ba4bcf5fce6fc76593e0606573dec4697 (which was ref #17198)
Rough outline of the change needed is something like:
* If no total ordering existing, `pk` is added.
* If this causes performance issues, add composite index.
--
Ticket URL: <https://code.djangoproject.com/ticket/29943#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Taha Jahangir | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: assigned
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
Comment:
I added requested changes in the PR
--
Ticket URL: <https://code.djangoproject.com/ticket/29943#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Taha Jahangir | Owner: Hasan
Type: | Ramezani
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"f2b460231dc15d4d0629b856f31ff9433da07f95" f2b46023]:
{{{
#!CommitTicketReference repository=""
revision="f2b460231dc15d4d0629b856f31ff9433da07f95"
[2.2.x] Fixed #29943 -- Doc'd that admin changelist may add pk to
ordering.
Backport of f63811f4813f0e0439e140a97eeba18a5017e858 from master.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29943#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Taha Jahangir | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: closed
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"f63811f4813f0e0439e140a97eeba18a5017e858" f63811f4]:
{{{
#!CommitTicketReference repository=""
revision="f63811f4813f0e0439e140a97eeba18a5017e858"
Fixed #29943 -- Doc'd that admin changelist may add pk to ordering.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29943#comment:12>