[Django] #23435: GFK should be indexed

Django

unread,

Sep 6, 2014, 4:51:33 AM9/6/14

to django-...@googlegroups.com

#23435: GFK should be indexed
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database | Keywords:
layer (models, ORM) | Has patch: 0
Severity: Normal | Needs tests: 0
Triage Stage: | Easy pickings: 0
Unreviewed |
Needs documentation: 0 |
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
(Yes, I know GFKs are evil and should be avoided at all costs.
Nonetheless, as long as Django supports them, people will use them.)

Django automatically adds an index on foreign keys, for fast reverse
lookups, unless it's explicitly disabled with `db_index = False`.

I believe it should also add an index on `(content_type, object_id)` on
GFK. If we make this the default in a future version of Django, users will
just have to run `makemigrations` on apps that have a GFK and they'll get
the index for free.

The alternative would be to update the documentation to recommend adding
the index explicitly.

{{{
class Meta:
index_together = [('content_type', 'object_id')]
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/23435>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

Django

unread,

Sep 6, 2014, 8:07:21 AM9/6/14

to django-...@googlegroups.com

#23435: GFK should be indexed
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master

* stage: Unreviewed => Accepted

Comment:

It's indeed a big performance footgun, accepting on this basis.

I vote for adding the index automatically but how would that work in
practice? The field would manipulate the model's `Meta` from
`contribute_to_class`?

--
Ticket URL: <https://code.djangoproject.com/ticket/23435#comment:1>

Django

unread,

Sep 6, 2014, 1:15:38 PM9/6/14

to django-...@googlegroups.com

#23435: GFK should be indexed
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master

Comment (by aaugustin):

I haven't thought about the implementation yet.

Also we should make sure not to create a duplicate index for the people
who added the right one.

--
Ticket URL: <https://code.djangoproject.com/ticket/23435#comment:2>

Django

unread,

Dec 4, 2014, 6:41:01 AM12/4/14

to django-...@googlegroups.com

#23435: GFK should be indexed
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master

Comment (by alexanderad):

I've started looking at this ticket, and first tricky thing I caught
(using {{{contrubute_to_class}}}) is following scenario (taken from
model_meta tests):
(in short: is there a simple way to distinguish abstract field from multi-
table inherited?)
{{{
#!python
class AbstractPerson(models.Model):
...
# GFK fields
content_type_abstract = models.ForeignKey(ContentType,
related_name='+')
object_id_abstract = models.PositiveIntegerField()
content_object_abstract = GenericForeignKey('content_type_abstract',
'object_id_abstract')

class Meta:
abstract = True
}}}
This one is easy, here we need:
{{{
index_together = (('object_id_abstract', 'content_object_abstract'),)
}}}

----

{{{
#!python
class BasePerson(AbstractPerson):
...
# GFK fields
content_type_base = models.ForeignKey(ContentType, related_name='+')
object_id_base = models.PositiveIntegerField()
content_object_base = GenericForeignKey('content_type_base',
'object_id_base')
}}}

This one is easy too, at this point we need:
{{{
index_together = (
('object_id_abstract', 'content_object_abstract'),
('content_type_base', 'object_id_base')
)
}}}

----

But then I stuck:
{{{
#!python
class Person(BasePerson):
...
# GFK fields
content_type_concrete = models.ForeignKey(ContentType,
related_name='+')
object_id_concrete = models.PositiveIntegerField()
content_object_concrete = GenericForeignKey('content_type_concrete',
'object_id_concrete')
}}}

At this point in {{{contribute_to_class}}} we are going to add two more
fields to index from {{{Person}}}, this is OK. But we have also inherited
from {{{BasePerson}}} + abstract from {{{ AbstractPerson}}}. All those
fields belong to {{{BasePerson}}}, but we need to take from there only
abstract ones, so that for {{{Person}}} we have:
{{{
index_together = (
('object_id_abstract', 'content_object_abstract'),
('content_type_concrete', 'object_id_concrete')
)
}}}

We can't add to index BasePerson fields since those live in different
table. But we still need ones from AbstractPerson.
Probably I'm terribly wrong, but for this case I can't see obvious way to
tell which inherited fields came from Abstract model and which from Base
one.

{{{
ipdb> cls
<class 'model_meta.models.Person'>
ipdb> [x.name for x in cls._meta.local_fields]
['baseperson_ptr', 'data_inherited', 'fk_inherited',
'data_not_concrete_inherited', 'content_type_concrete',
'object_id_concrete']

ipdb> [x.name for x in cls._meta.fields]
['id', 'data_abstract', 'fk_abstract', 'data_not_concrete_abstract',
'content_type_abstract', 'object_id_abstract', 'data_base', 'fk_base',
'data_not_concrete_base', 'content_type_base', 'object_id_base',
'baseperson_ptr', 'data_inherited', 'fk_inherited',
'data_not_concrete_inherited', 'content_type_concrete',
'object_id_concrete']

ipdb> cls._meta.get_field('object_id_base').model # << this one is from
base, we need to omit it
<class 'model_meta.models.BasePerson'>
ipdb> cls._meta.get_field('object_id_abstract').model # <<< this one is
from abstract, we need it
<class 'model_meta.models.BasePerson'>
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/23435#comment:3>

Django

unread,

Dec 4, 2014, 9:56:52 AM12/4/14

to django-...@googlegroups.com

#23435: GFK should be indexed
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master

Comment (by charettes):

The `('object_id_abstract', 'content_object_abstract')` index should only
be added to `BasePerson` indexes and not `Person` ones.

In this case you can solely rely on `opts.local_fields` to make sure both
`ct_field` and `fk_field` are local in order to add the index.

If one of the fields is not local (it belongs to a concrete parent) then
the index shouldn't be created.

--
Ticket URL: <https://code.djangoproject.com/ticket/23435#comment:4>

Django

unread,

Dec 5, 2014, 7:34:52 AM12/5/14

to django-...@googlegroups.com

#23435: GFK should be indexed
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master

Comment (by alexanderad):

Oh, so, this means that {{{Person}}} in fact has no abstract model columns
in its table. In this case that makes sense.

One more thing: it seems that {{{cls._meta.local_fields}}} is not
guaranteed to be fully populated during {{{contribute_to_class}}} call, is
it? From run to run I'm getting different set of fields in
{{{local_fields}}} during {{{contribute_to_class}}} calls.

However, the order of calls are always consistent, for example,
{{{Person}}} gets contributions from itself
({{{Person.content_object_concrete}}}), then from {{{BasePerson}}} and
finally from {{{AbstractPerson}}}.

I need to go deeper.

--
Ticket URL: <https://code.djangoproject.com/ticket/23435#comment:5>

Django

unread,

Dec 5, 2014, 9:56:19 AM12/5/14

to django-...@googlegroups.com

#23435: GFK should be indexed
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master

Comment (by alexanderad):

What also interests me, what kind of index do we want? Composite one,
which has two columns, or two separate? We automatically get
{{{fk_field}}} indexed, do we need to add, in this case, separate index on
{{{ct_field}}}?

--
Ticket URL: <https://code.djangoproject.com/ticket/23435#comment:6>

Django

unread,

Dec 5, 2014, 10:11:28 AM12/5/14

to django-...@googlegroups.com

#23435: GFK should be indexed
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master

Comment (by alexanderad):

Here is my naive implementation for this ticket:
https://github.com/alexanderad/django/compare/alexanderad:master...ticket_23435_indexed_gfk

--
Ticket URL: <https://code.djangoproject.com/ticket/23435#comment:7>

Django

unread,

Jul 31, 2015, 2:49:51 PM7/31/15

to django-...@googlegroups.com

#23435: GenericForeignKey should be indexed
--------------------------------------+------------------------------------
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.contenttypes | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Changes (by timgraham):

* component: Database layer (models, ORM) => contrib.contenttypes

--
Ticket URL: <https://code.djangoproject.com/ticket/23435#comment:8>

Django

unread,

Jul 27, 2016, 9:50:46 PM7/27/16

to django-...@googlegroups.com

#23435: GenericForeignKey should be indexed
--------------------------------------+------------------------------------

Reporter: aaugustin | Owner: nobody

Comment (by timgraham):

I closed #26964 as a duplicate which suggests to document the lack of this
index. Until the implementation of that ticket is completed, if someone
wants to submit a PR with that documentation it could be accepted.

--
Ticket URL: <https://code.djangoproject.com/ticket/23435#comment:9>

Django

unread,

Nov 13, 2019, 12:36:19 AM11/13/19

to django-...@googlegroups.com

#23435: GenericForeignKey should be indexed
--------------------------------------+------------------------------------

Reporter: Aymeric Augustin | Owner: nobody

Comment (by Asif Saifuddin Auvi):

considering the model meta refactor and class-based Index and many ORM
improvement in recent years, what is the best possible solution for this?
I would to know the recent consensus on this :)

--
Ticket URL: <https://code.djangoproject.com/ticket/23435#comment:10>

Django

unread,

Apr 29, 2022, 3:23:30 AM4/29/22

to django-...@googlegroups.com

#23435: GenericForeignKey should be indexed
--------------------------------------+------------------------------------
Reporter: Aymeric Augustin | Owner: nobody
Type: Cleanup/optimization | Status: new

Component: contrib.contenttypes | Version: dev

Comment (by Mariusz Felisiak <felisiak.mariusz@…>):

In [changeset:"2308fb5806bd30befc9f076905c1fd865e1176bf" 2308fb58]:
{{{
#!CommitTicketReference repository=""
revision="2308fb5806bd30befc9f076905c1fd865e1176bf"
[4.0.x] Refs #23435 -- Added note about GenericForeignKey indexes to docs.

Backport of 562e3bc09aa094a2ebbd3890fa233d04daafa8c9 from main
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/23435#comment:12>

Django

unread,

Apr 29, 2022, 3:23:31 AM4/29/22

to django-...@googlegroups.com

#23435: GenericForeignKey should be indexed
--------------------------------------+------------------------------------
Reporter: Aymeric Augustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.contenttypes | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------

Comment (by Mariusz Felisiak <felisiak.mariusz@…>):

In [changeset:"562e3bc09aa094a2ebbd3890fa233d04daafa8c9" 562e3bc]:
{{{
#!CommitTicketReference repository=""
revision="562e3bc09aa094a2ebbd3890fa233d04daafa8c9"