django/django-contrib-comments

indexing content_type and object_pk

crolfe opened this issue · 6 comments

Hello!

I have encountered a major performance issue with this library, due to the generic relation defined in BaseCommentAbstractModel

I was hoping it'd be as simple as adding this to the Meta:

index_together = [                                          
    ('content_type', 'object_pk')
]

Alas, this does not work on MySQL (not sure if it would or wouldn't on Postgres or other databases), due to the use of a TextField for the object_pk:

_mysql_exceptions.OperationalError: (1170, "BLOB/TEXT column 'object_pk' used in key specification without a key length")

I have already solved this for my own project's needs in a fork by switching to a CharField, but I suspect the performance hit (of the missing index) is something other users will encounter, sooner or later, so I'd like to push it upstream.

The major risk I can see to this is it would be a breaking change for anyone who has object_pks larger than the max_length of a CharField, but the stated intention for a non-numeric field is to support UUIDs, which could still be supported.

I will submit a PR if this something that would be accepted.

Note that the pull request django/django#9132 is going a step forward allowing indexes on long MySQL fields.

@claudep Good to know that's coming, thanks!

Do you know approximately when that might land in Django? Also, would 1.11 get that that change?

Absolutely no chance for such a patch being backported, sorry.
Don't hesitate to review/comment the patch and weigh on the ticket if you want to speed up things.

Fixed by 7bcc661