Opened 11 years ago

Closed 11 years ago

#20849 closed Cleanup/optimization (fixed)

ModelForms don't work well with prefetch_related

Reported by: anonymous Owned by: nobody
Component: Forms Version: dev
Severity: Normal Keywords:
Cc: 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

Following the example at https://docs.djangoproject.com/en/1.5/ref/models/querysets/#prefetch-related, ModelForms are a pain to use if you want to eagerly load many pizza forms. It is more or less worth it to skip writing forms, since figuring out how to write a manual view that can populate 15 pizza forms with 3 queries is easy, whereas writing a form that can do it for fewer than 33 is hard. Manually setting choices on the form can reduce this down to 17, 1 for the list of pizzas, 1 inner join for each pizza, and 1 master list for all toppings, plus 1 superfluous inner join for each pizza. Removing that last bit seems hard and is certainly undocumented.

Attachments (1)

lazy_model_form.diff (627 bytes ) - added by david08uc@… 11 years ago.
One line patch that avoids reduces sql multiplication for prefetched forms

Download all attachments as: .zip

Change History (9)

comment:1 by Simon Charette, 11 years ago

#18597 is a InlineFormSet related issue, we should really eat our own dog food in ModelForms.

by david08uc@…, 11 years ago

Attachment: lazy_model_form.diff added

One line patch that avoids reduces sql multiplication for prefetched forms

comment:2 by david08uc@…, 11 years ago

I added a patch, which I made to the trunk version.
I ran the test suite, but with many skips and expected failures (and I believe the relavant code path was not exercised). I don't have the knowledge or time to fix that.

However, I believe this patch is a good idea, because the current behavior resorts to a model utility method that always clones the queryset of a many-to-many manager. This is overkill when the goal is to populate a list of pk's, especially when the result is frozen as a list anyway.

comment:3 by Tim Graham, 11 years ago

Component: UncategorizedForms
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

This would a test that uses assertNumQueries() to show that the query count is reduced.

comment:4 by Tim Graham, 11 years ago

Has patch: set
Needs tests: set

comment:5 by Jim Bailey, 11 years ago

Hello,

I have also run into this issue, so I fixed it on this branch:
https://github.com/dgym/django/tree/ticket_20849

The commit includes a unit test that now passes.

The fix only changes the behaviour if there is prefetched data, as the above patch would hurt performance in the non-prefetched case.

comment:6 by Jim Bailey, 11 years ago

Needs tests: unset

I have made the following pull request:
https://github.com/django/django/pull/1840

comment:7 by loic84, 11 years ago

Triage Stage: AcceptedReady for checkin
Version: 1.5master

Looks good to me.

comment:8 by Anssi Kääriäinen <akaariai@…>, 11 years ago

Resolution: fixed
Status: newclosed

In 539e3693d4712b249a95cfad8cfdeecdad1777a6:

Fixed #20849 -- ModelForms do not work well with prefetch_related.

model_to_dict() (used when rendering forms) queries the database
to get the list of primary keys for ManyToMany fields. This is
unnecessary if the field queryset has been prefetched, all the
keys are already in memory and can be obtained with a simple
iteration.

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