QuerySet Q + select_related regression with multiple ForeignKeys

[Tim Graham]

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 )

[Tim Graham]

Test that applies cleanly to master

[Anssi Kääriäinen]

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.

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).

[Anssi Kääriäinen]

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.

[Anssi Kääriäinen <akaariai@…>]

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.

[Anssi Kääriäinen]

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.