prefetch_related uses an inefficient query to get the related fk data

[valtron2000@…]

Consider the following models and query:

class Author(models.Model):
        name = models.CharField(max_length = 20)

class Book(models.Model):
        name = models.CharField(max_length = 20)
        author = models.ForeignKey(Author, related_name = 'books')

Book.objects.all().prefetch_related('author')

The query I'd expect to be issued by the prefetch_related is:

SELECT * FROM author
WHERE id IN (... the author_ids of the books from the main query ...)

However, the query that actually get executed are:

SELECT a.* FROM author a INNER JOIN book b ON (a.id = b.author_id)
WHERE b.id IN (...  the ids of the books from the main query ...)

There are two problems with this approach:

1. It does a join, which wouldn't be necessary if author_ids were used instead of book.ids
2. When one uses prefetch_related in such a case (that is, to prefetch a single fk per object rather than a many to many) instead of select_related, it's usually because there are many primary objects (books) and few unique related objects (authors); select_related, despite getting all the data in one query, could end up returning much more data than is needed because the related objects are duplicated many times over, and will be slower overall. Thus, the approach currently used by prefetch_related ends up using the set of ids that will almost always be larger (the book ids, rather than the authors).

The reverse case (Author.objects.all().prefetch_related('books')) does the efficient, joinless query I'd expect -- namely:

SELECT * FROM book
WHERE author_id IN (... the ids of the authors from the main query ...)

I've attached a sample project with a test.

[valtron2000@…]

Project for book example with a test.

[Anssi Kääriäinen]

Seems OK to me.

[valtron2000@…]

Added a patch & test; seems to do the trick and all tests pass.

[valtron2000@…]

My bad, I only ran the prefetch_related tests. Patch breaks foreign_object.tests.MultiColumnFKTests.test_prefetch_foreignkey_forward_works.

[valtron2000@…]

Fixed patch

[Marti Raudsepp]

I am in the process of migrating an app from Django 1.5 to 1.6; this regression also affects us (it did not occur in Django 1.5). In one example this turns a query from 175 bytes into 66 kB, with a corresponding increase in query time, result set size and memory usage.

I can confirm that the attached patch fixes this issue when applied on top of Django 1.6.2. Can it be included in 1.6.3?

[Tim Graham]

Haven't verified the claim that it's a regression from 1.5, but marking it as a release blocker assuming that's the case. A note in the 1.6.3 release notes would also be helpful.

[loic84]

Related to #21410.

[loic84]

We discussed this issue with @akaariai on IRC.

Like we did for #21410, we'll take a practical approach for fixing the regression from Django 1.6 to master and go for the proposed fix. It's worth noting that the inefficient query will still be present for multicolumn relations, but these are not documented and one needs to use private APIs to make them, so that doesn't qualify as a regression.

The implementation of ReverseSingleRelatedObjectDescriptor.get_prefetch_queryset() really ought to be cleaned up at some point, especially when we land the patch for composite fields but it'll require a fair amount of refactoring so it only qualifies for master.

@valtron2000, would you mind updating the patch?

• The "FIXME" comment would have to be updated to explain this new case with a reference to this ticket.
• The current test is pretty fragile, could you rather check in QS.query if there is a join? (e.g. ​https://gist.github.com/10345520)
• Code needs to be compatible with PY3, use range instead of xrange, or import it from six if you really want xrange.
• The changes inside the if, do they do anything differently or is it just a cleanup? If it's just a cleanup, only fix the if condition, and not the implementation. Because right now it's pretty hard to see where the problem was from.

Feel free to open a PR on github.

Edit: Added a couple more things to the patch review.

[valtron2000@…]

I'll submit a PR soon; thanks for the suggestions.

The fix is a one-liner in the if, and the rest of the changes were just cleanup.

[valtron2000@…]

​https://github.com/django/django/pull/2538

[Loic Bistuer <loic.bistuer@…>]

In d3b71b976dee4b02908235b8007db254a9795268:

Fixed #21760 -- prefetch_related used an inefficient query for reverse FK.

Regression introduced by commit 9777442. Refs #21410.

[Loic Bistuer <loic.bistuer@…>]

In 6b3a8d27052bb5b242ab6c1c2fb0d6da5b49681d:

[1.7.x] Fixed #21760 -- prefetch_related used an inefficient query for reverse FK.

Regression introduced by commit 9777442. Refs #21410.

Backport of d3b71b976d from master

[Loic Bistuer <loic.bistuer@…>]

In 1252b77824639fba398cf70586bdd75c42f86fdf:

[1.6.x] Fixed #21760 -- prefetch_related used an inefficient query for reverse FK.

Regression introduced by commit 9777442. Refs #21410.

Conflicts:

tests/prefetch_related/tests.py

Backport of d3b71b976d from master