Opened 15 years ago

Closed 13 years ago

Last modified 13 years ago

#11604 closed (wontfix)

ChangeList should pass depth=1 to .select_related()

Reported by: rlaager@… Owned by: nobody
Component: contrib.admin Version: 1.0
Severity: Keywords: efficient-admin
Cc: Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Currently, if a ModelAdmin has a ForeignKey field in list_display, ChangeList will call .select_related() on the QuerySet (with some exceptions). As it does not specify a depth, this leads to joining against up to 5 levels (the arbitrary default limit). If it passed depth=1, this would be avoided. In the case where the related model needs another related model to calculate its unicode(), lazy loading will happen. In the event that matters for performance, there's already a mechanism (list_select_related on the ModelAdmin) to specify that you want a full (well, 5 levels deep) .select_related().

Attachments (1)

11604-changelist-select-related-depth.diff (542 bytes ) - added by rlaager@… 15 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 by Ramiro Morales, 15 years ago

See also #10348, #10742, #9554 and some other related tickets for previous activity in this front.

comment:2 by Alex Gaynor, 15 years ago

Triage Stage: UnreviewedDesign decision needed

comment:3 by nick@…, 15 years ago

Despite what it says at the start of this thread, select_related making very complicated queries is what is causing mysql to get stuck in state statistics.

http://groups.google.com/group/django-developers/browse_thread/thread/ad87d824d4910284

This is certainly a mysql bug, but it is triggered by the admin's uncontrolled use of select_related.

Applying the patch above for select_related(depth=1) fixes the mysql lockups

So I'd vote for this patch as it is non-intrusive and fixes our rather difficult to track down mysql lockups.

comment:4 by Thejaswi Puthraya, 15 years ago

Component: Uncategorizeddjango.contrib.admin

comment:5 by rlaager@…, 14 years ago

Has patch: set

comment:6 by ishirav, 14 years ago

Yes! Please please please apply this patch. We've seen ridiculous queries joining 20 or more tables needlessly, and MySQL hanging while trying to run these queries.

As a temporary workaround, here's a way to override the select_related depth in the admin queryset:

class FoobarAdmin(admin.ModelAdmin):
    def queryset(self, request):
        qs = super(FoobarAdmin, self).queryset(request)
        return qs.select_related(depth=1)

comment:7 by anonymous, 14 years ago

milestone: 1.3

comment:8 by Julien Phalip, 13 years ago

Resolution: wontfix
Status: newclosed

It is quite common to require access to the related models, for example to use as list filters, so it makes sense that Django uses select_related(). However, by arbitrarily using a specific depth of 1 (or whatever number), Django would be taking a guess about what performance trick would work for the majority of sites. A depth of 1 may work well with your use case, but pretty poorly in others. For what it's worth, in 4+ years using Django in many projects I've rarely witnessed such performance issues in the admin, even with MySQL. I'm not saying that your use case doesn't exist, but it doesn't seem like it is common enough to justify changing Django's default behaviour. What's more is that ModelAdmin already offers some hooks to get exactly what you want, as illustrated in ishirav's code snippet above.

comment:9 by Luke Plant, 13 years ago

Just to add to what Julian said:

The alternatives (not using selected_related or limiting to 1 etc) could just as easily cause 'silly' performance problems where hundreds of queries are executed instead of one. This would make other people just as unhappy (or the same people unhappy in different ways). What is more, this change would be a regression for lots of people. Whether or not 5 is the best default doesn't matter - it is the number we have picked and you can override the behaviour. See Jacob's comment on #9554.

comment:10 by Jacob, 13 years ago

milestone: 1.3

Milestone 1.3 deleted

Note: See TracTickets for help on using tickets.
Back to Top