Opened 6 years ago

Closed 5 years ago

Last modified 4 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: UI/UX:

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@… 6 years ago.

Download all attachments as: .zip

Change History (11)

Changed 6 years ago by rlaager@…

comment:1 Changed 6 years ago by ramiro

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

comment:2 Changed 6 years ago by Alex

  • Triage Stage changed from Unreviewed to Design decision needed

comment:3 Changed 6 years ago by nick@…

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 Changed 6 years ago by thejaswi_puthraya

  • Component changed from Uncategorized to django.contrib.admin

comment:5 Changed 6 years ago by rlaager@…

  • Has patch set

comment:6 Changed 5 years ago by ishirav

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 Changed 5 years ago by anonymous

  • milestone set to 1.3

comment:8 Changed 5 years ago by julien

  • Resolution set to wontfix
  • Status changed from new to closed

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 Changed 5 years ago by lukeplant

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 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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