Opened 3 years ago

Closed 3 years ago

#20849 closed Cleanup/optimization (fixed)

ModelForms don't work well with prefetch_related

Reported by: anonymous Owned by: nobody
Component: Forms Version: master
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


Following the example at, 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@… 3 years ago.
One line patch that avoids reduces sql multiplication for prefetched forms

Download all attachments as: .zip

Change History (9)

comment:1 Changed 3 years ago by Simon Charette

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

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

Changed 3 years ago by david08uc@…

Attachment: lazy_model_form.diff added

One line patch that avoids reduces sql multiplication for prefetched forms

comment:2 Changed 3 years ago by david08uc@…

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 Changed 3 years ago by Tim Graham

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 Changed 3 years ago by Tim Graham

Has patch: set
Needs tests: set

comment:5 Changed 3 years ago by Jim Bailey


I have also run into this issue, so I fixed it on this branch:

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 Changed 3 years ago by Jim Bailey

Needs tests: unset

I have made the following pull request:

comment:7 Changed 3 years ago by loic84

Triage Stage: AcceptedReady for checkin
Version: 1.5master

Looks good to me.

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

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

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