#20528 closed Bug (fixed)
QuerySet Q + select_related regression with multiple ForeignKeys
Reported by: | Tim Graham | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Release blocker | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
The attached test last passed in 3fef304ff237fe692459c1f5b840acf7886c50bb, generating the following query:
SELECT (fields) FROM "select_related_regress_childb" LEFT OUTER JOIN "select_related_regress_parent" ON ("select_related_regress_childb"."parent_id" = "select_related_regress_parent"."id") LEFT OUTER JOIN "select_related_regress_childa" ON ("select_related_regress_childb"."child_id" = "select_related_regress_childa"."id") WHERE ("select_related_regress_childb"."parent_id" = 1 OR "select_related_regress_childa"."parent_id" = 1 )
The query changed in edf93127bf2f9dc35b45cdea5d39a1b417ab1638 (affects master/1.6 only) with the replacement of the first "LEFT OUTER JOIN" with an "INNER JOIN":
SELECT (fields) FROM "select_related_regress_childb" INNER JOIN "select_related_regress_parent" ON ("select_related_regress_childb"."parent_id" = "select_related_regress_parent"."id") LEFT OUTER JOIN "select_related_regress_childa" ON ("select_related_regress_childb"."child_id" = "select_related_regress_childa"."id") WHERE ("select_related_regress_childb"."parent_id" = 1 OR "select_related_regress_childa"."parent_id" = 1 )
Attachments (2)
Change History (6)
by , 12 years ago
Attachment: | 20528-test-edf93127bf2f9dc35b45cdea5d39a1b417ab1638.diff added |
---|
by , 12 years ago
Attachment: | 20528-test-master.diff added |
---|
comment:1 by , 12 years ago
Severity: | Normal → Release blocker |
---|---|
Triage Stage: | Unreviewed → Accepted |
The regression is caused by trimmed join. The OR condition would promote the join to LEFT OUTER, but the join is already trimmed so it isn't seen when the promotion happens. Then select_related adds a join to the same model and there pre-exists an INNER JOIN. That gets reused and the result is failure.
There are two obvious ways to fix this, first is: https://github.com/akaariai/django/commit/62b1d93d638ae05312af77775c96bdd602f075a5 - that is when Query.join(outer_if_first=True) is used, then aliases which exists but have refcount of 0 are promoted. The alternate is to promote the joins in ORed queries even for trimmed joins. EDIT: https://github.com/akaariai/django/compare/ticket_20528 for the alternate approach
Both approaches lead to test failures. The failures aren't critical, the queries will produce correct results but the join types aren't as efficient as possible.
If I am not mistaken in 1.5 the join wasn't trimmed in any case for the ORed query (not 100% sure of this), and select_related() always force-promoted any nullable relation it saw (this I am sure of). So in that regard there will be no performance regression compared to 1.5.
I will try to think if there is some better solution. It seems we need a bit more information about the join type (that is "forced INNER/OUTER joins") to fix this without breaking any other tests. But that is somewhat larger change (OTOH this would allow creating EmptyQuerySets for some cases, for example qs.filter(parent=None, parent__name='foo')
- the parent join, if it exists, can't be INNER, and it must be INNER => EmptyQuerySet).
comment:2 by , 12 years ago
The optimal solution for this seems to be to do yet another refactor to join promotion in add_q. Basically the add_q/_add_q needs to coalesce needed inner & outer joins for each child, then do the join promotions when all operations are processed and all join types are thus known (that is, in the end of _add_q). This isn't a small refactor, so I don't want to do this in alpha stage.
I will use the approach of "outer_if_first=True" => aliases with refcount 0 are promoted for 1.6. This will mean a couple of tests will need to be moved to expected failure status. These failures are just using LEFT OUTER JOIN when INNER JOIN could be used. So, query results will be correct but the join type isn't optimal. The join type wasn't correct for these cases in 1.5 either.
comment:3 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:4 by , 12 years ago
I ended up doing the promotion of trimmed joins anyways instead of promoting refcount=0 joins. There might be other parts of the code that expect existing joins to have correct type, so better program defensively.
Test that applies cleanly to master