Opened 7 years ago

Closed 3 years ago

#13017 closed Bug (needsinfo)

generic relation generates needless join

Reported by: MS Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: database joins generic relation
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I have an generic relation, and tries to do some database queries with this relation.
When looking at the generated sql query i saw, that double joins are created. This happens
because generic relations are based on m2m table with a missing database table.

An example for the model:

from django.db import models
from django.contrib.contenttypes.models import ContentType
from django.contrib.contenttypes import generic

class GenericTest(models.Model):
    contentType = models.ForeignKey(ContentType)
    objectId = models.PositiveIntegerField()
    contentObject = generic.GenericForeignKey('contentType', 'objectId')

class RefTest(models.Model):
    generic = generic.GenericRelation(GenericTest, content_type_field='contentType', object_id_field='objectId')
    testdata = models.PositiveIntegerField()

the query:

GenericTest.objects.filter(reftest__testdata=0)

as sql:

('SELECT U0.`id` FROM `test_generictest` U0 INNER JOIN `test_generictest` U1 ON (U0.`id` = U1.`id`) INNER JOIN `test_reftest` U2 ON (U1.`objectId` = U2.`id`) WHERE U2.`testdata` = %s ', (0,))

With the applied patch, which made the same test, which is done in the direct case, the sql query looks like:

('SELECT U0.`id` FROM `test_generictest` U0 INNER JOIN `test_reftest` U1 ON (U0.`id` = U1.`id`) WHERE U1.`testdata` = %s ', (0,))

For simple databases and simple sql queries, this extra join is no problem, but with complex ones, its a huge performance lack.

Attachments (1)

sql_query.patch (1.4 KB) - added by MS 7 years ago.
Patch für Double Joins

Download all attachments as: .zip

Change History (15)

comment:1 Changed 7 years ago by Russell Keith-Magee

milestone: 1.2
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted

This is possibly a regression caused by the recent m2m changes; needs further investigation.

comment:2 Changed 7 years ago by MS

This bug is also in Django 1.1 so i think it's not a regression.

comment:3 Changed 7 years ago by MS

The new patch has removed the double line, which was not nessecary in the patch before.

comment:4 Changed 7 years ago by Russell Keith-Magee

milestone: 1.21.3

Not critical for 1.2

Changed 7 years ago by MS

Attachment: sql_query.patch added

Patch für Double Joins

comment:5 Changed 7 years ago by MS

There was another mistake in the patch, using the wrong database field with generic relations.

I am sad, that this patch won't be in 1.2, because it saves us a lot of time and ram used the database. Without it we would need a bigger server for database requests.

comment:6 Changed 5 years ago by Luke Plant

Type: Cleanup/optimization

comment:7 Changed 5 years ago by Luke Plant

Severity: Normal

comment:8 Changed 5 years ago by blacklwhite

Triage Stage: AcceptedReady for checkin
Last edited 5 years ago by blacklwhite (previous) (diff)

comment:9 Changed 5 years ago by blacklwhite

Triage Stage: Ready for checkinAccepted

Sorry, wrong ticket number.

Last edited 5 years ago by blacklwhite (previous) (diff)

comment:10 Changed 5 years ago by Julien Phalip

Needs tests: set
Type: Cleanup/optimizationBug

comment:11 Changed 5 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

comment:11 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:12 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:13 Changed 3 years ago by Anssi Kääriäinen

Resolution: needsinfo
Status: newclosed

I am closing this as needsinfo. The query GenericTest.objects.filter(reftest__testdata=0) isn't valid Django query.

Note: See TracTickets for help on using tickets.
Back to Top