Code

Opened 11 months ago

Closed 10 months ago

Last modified 10 months ago

#20528 closed Bug (fixed)

QuerySet Q + select_related regression with multiple ForeignKeys

Reported by: timo Owned by: nobody
Component: Database layer (models, ORM) Version: master
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)

20528-test-edf93127bf2f9dc35b45cdea5d39a1b417ab1638.diff (2.5 KB) - added by timo 11 months ago.
20528-test-master.diff (2.5 KB) - added by timo 11 months ago.
Test that applies cleanly to master

Download all attachments as: .zip

Change History (6)

Changed 11 months ago by timo

Test that applies cleanly to master

comment:1 Changed 10 months ago by akaariai

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to 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).

Last edited 10 months ago by akaariai (previous) (diff)

comment:2 Changed 10 months ago by akaariai

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 Changed 10 months ago by Anssi Kääriäinen <akaariai@…>

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

In 89bf7a45250cf554295f3d17d708311d3edba101:

Fixed #20528 -- regression in select_related join promotion

The join used by select_related was incorrectly INNER when the query
had an ORed filter for nullable join that was trimmed away. Fixed this
by forcing the join type to LOUTER even when a join was trimmed away
in ORed queries.

comment:4 Changed 10 months ago by akaariai

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.

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.