Opened 5 years ago
Last modified 3 months ago
#29725 assigned Cleanup/optimization
Inefficient SQL generated when counting a ManyToMany
Reported by: | Gavin Wahl | Owned by: | Shiwei Chen |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Simon Charette | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | yes | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
When calling count() on an unfiltered many to many relation, a useless join is included in the SQL that makes it much slower than it should be. On my dataset, the difference is 1000ms to 100ms, because an index-only scan can be used.
This is the SQL that is currently generated:
SELECT COUNT(*) AS "__count" FROM "app_foo" INNER JOIN "app_foo_bar" ON ("app_foo"."id" = "app_foo_bar"."foo_id") WHERE "app_foo_bar"."foo_id" = ?;
This is the SQL that should be generated:
SELECT COUNT(*) AS "__count" FROM "app_foo_bar" WHERE "app_foo_bar"."foo_id" = ?;
This optimization can only be applied when there are no filters applied, because then the join is used to satisfy the filters. In the no-filters case, only the through table needs to be consulted.
Change History (22)
comment:1 Changed 5 years ago by
Owner: | changed from nobody to oliver |
---|---|
Status: | new → assigned |
comment:2 follow-up: 4 Changed 5 years ago by
Triage Stage: | Unreviewed → Accepted |
---|---|
Type: | Uncategorized → Cleanup/optimization |
Version: | 2.1 → master |
comment:3 Changed 5 years ago by
Cc: | Simon Charette added |
---|
comment:4 Changed 5 years ago by
Thanks for your advice.
I will try to correct it according to your advice
Replying to Simon Charette:
Hello Olivier,
I think you should be able to achieve this by defining a
count()
method on the dynamic class created bycreate_forward_many_to_many_manager
by filteringself.through._default_manager
based onself.instance
and return itscount()
.
comment:6 Changed 5 years ago by
Has patch: | set |
---|---|
Needs tests: | set |
comment:7 Changed 5 years ago by
Have we considered making this change for exists()
as well? I'm sure this will generate the join in the same way that count()
does.
Also somewhat related: #28477
comment:9 Changed 5 years ago by
Needs tests: | unset |
---|---|
Patch needs improvement: | set |
comment:10 Changed 5 years ago by
Patch needs improvement: | unset |
---|
comment:14 Changed 4 years ago by
Resolution: | fixed |
---|---|
Status: | closed → new |
Solution has been reverted because it caused the inconsistent behavior of count()
and exists()
on a reverse many-to-many relationship with a custom manager (see new tests in 9ac8520fcde29840a1345be19d80dbda53aa6d03).
comment:15 Changed 4 years ago by
Has patch: | unset |
---|
comment:16 Changed 4 years ago by
Has patch: | set |
---|---|
Patch needs improvement: | set |
Switched back to patch needs improvement instead as it can probably be adapted to skip the optimization when a custom manager is defined.
comment:17 Changed 4 years ago by
Ticket #30325 revealed one more issue, i.e. optimization doesn't work when chaining all()
after reverse M2M relations.
comment:18 Changed 4 years ago by
Additionally, the optimization causes a query to be done if the related objects have already been prefetched.
comment:19 Changed 5 months ago by
Owner: | changed from oliver to Shiwei Chen |
---|---|
Status: | new → assigned |
Thanks everyone for your insights. Im going to try to implement this optimization, accounting for all the issues listed here so far.
comment:20 Changed 5 months ago by
Needs tests: | set |
---|
Patch needs a few tweaks to properly disable the custom manager (most notably tests). It also lacks coverage for the prefetched case as reported by comment:18.
comment:21 Changed 4 months ago by
Needs tests: | unset |
---|---|
Patch needs improvement: | unset |
Updated the patch to properly deal with cases involving custom managers and prefetch queries, and implemented test cases for them.
comment:22 Changed 3 months ago by
Needs tests: | set |
---|---|
Patch needs improvement: | set |
Hello Olivier,
I think you should be able to achieve this by defining a
count()
method on the dynamic class created bycreate_forward_many_to_many_manager
by filteringself.through._default_manager
based onself.instance
and return itscount()
.