jazzband / django-taggit

Simple tagging for django
https://django-taggit.readthedocs.io
BSD 3-Clause "New" or "Revised" License
3.27k stars 623 forks source link

Fix Django warnings about index_together with new migration #862

Open AlanCoding opened 1 year ago

AlanCoding commented 1 year ago

This is triggering a Django warning

RemovedInDjango51Warning: 'index_together' is deprecated. Use 'Meta.indexes' in 'taggit.TaggedItem' instead.

This updates the index_together use to resolve this, hopefully painlessly.

rtpg commented 1 year ago

something to check, whether index renaming is cheap or not in the major SQL vendors (locking etc)

rtpg commented 1 year ago

also whether the index renaming is actually safe? we want to make sure that deployments including this don't cause downtime. I'm also partial to outright not having the rename by hardcoding the existing index name

AlanCoding commented 12 months ago

Right, let's look at the details of what this migration does. For this I'm using a local sqlite3 database for testing.

$ python -m django sqlmigrate taggit 0006 --settings=tests.settings
BEGIN;
--
-- Rename unnamed index for ('content_type', 'object_id') on taggeditem to taggit_tagg_content_8fc721_idx
--
DROP INDEX "taggit_taggeditem_content_type_id_object_id_196cc965_idx";
CREATE INDEX "taggit_tagg_content_8fc721_idx" ON "taggit_taggeditem" ("content_type_id", "object_id");
COMMIT;

This confirms your hunch, it does delete the old index and re-create it. This could have a performance penalty. I believe this is called out by the Django docs

On databases that don’t support an index renaming statement (SQLite and MariaDB < 10.5.2), the operation will drop and recreate the index, which can be expensive.

And this case is sqlite3. For other databases like postgres, I believe it should use the rename SQL.

Also, the checks and the docs are telling me that this would have to be Django 4.1 or higher, which is a problematic requirement for migrations.

Also, the "test" app has unmigrated changes when testing with Django 4.2. Those don't seem to have anything to do with the index changes in Django.

rtpg commented 11 months ago

Things to do on this one:

mister-rao commented 10 months ago

Getting this error while running tests:

django.utils.deprecation.RemovedInDjango51Warning: 'index_together' is deprecated. Use 'Meta.indexes' in 'taggit.TaggedItem' instead. 

Versions: Django = "4.2.2" django-taggit = "4.0.0"