Code

Opened 11 months ago

Closed 8 months 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

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 months ago.
One line patch that avoids reduces sql multiplication for prefetched forms

Download all attachments as: .zip

Change History (9)

comment:1 Changed 11 months ago by charettes

  • 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 11 months ago by david08uc@…

One line patch that avoids reduces sql multiplication for prefetched forms

comment:2 Changed 11 months 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 11 months ago by timo

  • Component changed from Uncategorized to Forms
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Cleanup/optimization

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

comment:4 Changed 11 months ago by timo

  • Has patch set
  • Needs tests set

comment:5 Changed 8 months ago by Jim Bailey

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 Changed 8 months ago by Jim Bailey

  • Needs tests unset

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

comment:7 Changed 8 months ago by loic84

  • Triage Stage changed from Accepted to Ready for checkin
  • Version changed from 1.5 to master

Looks good to me.

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

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

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.