Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#25496 closed Bug (fixed)

ModelChoiceField generates extra queries by not respecting prefetch_related

Reported by: Matt d'Entremont Owned by: nobody
Component: Forms Version: 1.8
Severity: Release blocker Keywords:
Cc: marti@… 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

Note: could be worked around if this were resolved: https://code.djangoproject.com/ticket/22841

When a ModelChoiceField's choices are generated, the queryset's prefetch_related calls are not evaluated, which can lead to 1 query per choice if the model's unicode accesses a related field.

Example models:

from django.db import models
class RelatedObj(models.Model):
    name = models.CharField(max_length=255)


class ObjWithRelated(models.Model):
    name = models.CharField(max_length=255)
    related = models.ManyToManyField(RelatedObj)

    def __unicode__(self):
        return '{}: {}'.format(self.name, self.related.count())

With many models, we can see the following results:

from django.db import connection
from django.forms import ModelChoiceField
from django.test.utils import CaptureQueriesContext


queryset = ObjWithRelated.objects.prefetch_related('related')
field = ModelChoiceField(queryset)

with CaptureQueriesContext(db.connection) as queries:
    list(field.choices)
print 'ModelChoiceField query count: {}'.format(len(queries))

with CaptureQueriesContext(db.connection) as queries:
    [str(obj) for obj in queryset]
print 'Regular query count: {}'.format(len(queries))

This will have the output of:

There are ways to work around this, but ideally there would be a solution which wouldn't require a workaround. We're using django-parler to translate models, without the prefetching we get 1 query per dropdown choice, per dropdown when a translated model is displayed in the django admin.

Change History (14)

comment:1 Changed 4 years ago by Simon Charette

Resolution: invalid
Status: newclosed

Hi mdentremont,

I think you misunderstood how prefetch_related and count() interact. The count() method never takes prefetched data into consideration and always result in a new query.

If you want to avoid an extra query I suggest you annotate() your queryset with its count of related objects and adapt your models __unicode__ to lookup the annotated attribute first and fallback to calling self.related.count():

from django.db import models

class RelatedObj(models.Model):
    name = models.CharField(max_length=255)

class ObjWithRelated(models.Model):
    related = models.ManyToManyField(RelatedObj)

    def __unicode__(self):
        related_count = getattr(self, 'related_count', None)
        if related_count is None:
            related_count = self.related.count()
        return '{}: {}'.format(self.name, related_count)

queryset = ObjWithRelated.objects.annotate(related_count=models.Count('related'))
field = ModelChoiceField(queryset)

Please use support channels before filling a ticket wiki:TicketClosingReasons/UseSupportChannels

comment:2 Changed 4 years ago by Matt d'Entremont

Hello charettes,

Sorry, I used .count() in the example to try and simplify things a little. In actuality we are accessing a field on a related object in our unicode, in which case we definitely would want to use prefetch_related to avoid 1 query per __unicode__.

Last edited 4 years ago by Matt d'Entremont (previous) (diff)

comment:3 Changed 4 years ago by Simon Charette

Resolution: invalidduplicate

Then I guess this is simply a duplicate of #22841?

comment:4 Changed 4 years ago by Matt d'Entremont

Potentially?

My thinking was that #22841 is for passing in a queryset which will not be revaluated, where I want the re-evaluation to a avoid stale content, I just want the prefetch to be respected to avoid too many queries.

Also it took us a ton of investigation to determine why the prefetch wasn't performed, it is not obvious at all. If I pass a queryset along, I expect that it will be used entirely, not just certain pieces of it.

comment:5 Changed 4 years ago by Simon Charette

Resolution: duplicate
Status: closednew
Type: UncategorizedBug

Hmm it looks like it might be a regression in 1.8 cause by the fix for #23623 (fa534b92dda0771661a98a1ca302ced264d0a6da) which introduced the use of iterator() to reduce memory usage.

From the prefetch_related docs

Note that if you use iterator() to run the query, prefetch_related() calls will be ignored since these two optimizations do not make sense together.

Last edited 4 years ago by Simon Charette (previous) (diff)

comment:6 Changed 4 years ago by Tim Graham

Has patch: set
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted
Version: 1.8

comment:7 Changed 4 years ago by Simon Charette

Triage Stage: AcceptedReady for checkin

comment:8 Changed 4 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 6afa6818:

Fixed #25496 -- Made ModelChoiceField respect prefetch_related().

comment:9 Changed 4 years ago by Tim Graham <timograham@…>

In 6bc8bdf:

[1.9.x] Fixed #25496 -- Made ModelChoiceField respect prefetch_related().

Backport of 6afa6818fcf25665bbf61f0921c8c8c6fa8f223e from master

comment:10 Changed 4 years ago by Tim Graham <timograham@…>

In de570d4:

[1.8.x] Fixed #25496 -- Made ModelChoiceField respect prefetch_related().

Backport of 6afa6818fcf25665bbf61f0921c8c8c6fa8f223e from master

comment:11 Changed 4 years ago by Marti Raudsepp

Cc: marti@… added

It appears that this fix caused a regression. I have submitted pull requests with fixes, see ticket #25683.

comment:12 Changed 4 years ago by Tim Graham <timograham@…>

In 1155843a:

Fixed #25683 -- Allowed ModelChoiceField(queryset=...) to accept Managers.

This fixes a regression from refs #25496.

comment:13 Changed 4 years ago by Tim Graham <timograham@…>

In 8db5122d:

[1.9.x] Fixed #25683 -- Allowed ModelChoiceField(queryset=...) to accept Managers.

This fixes a regression from refs #25496.

Backport of 1155843a41af589a856efe8e671a796866430049 from master

comment:14 Changed 4 years ago by Tim Graham <timograham@…>

In 3144785:

[1.8.x] Fixed #25683 -- Allowed ModelChoiceField(queryset=...) to accept Managers.

This fixes a regression from refs #25496.

Backport of 1155843a41af589a856efe8e671a796866430049 from master

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