django-taggit icon indicating copy to clipboard operation
django-taggit copied to clipboard

Fix Django warnings about index_together with new migration

Open AlanCoding opened this issue 2 years ago • 5 comments

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.

AlanCoding avatar Jul 06 '23 14:07 AlanCoding

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

rtpg avatar Jul 06 '23 17:07 rtpg

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

rtpg avatar Jul 06 '23 17:07 rtpg

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.

AlanCoding avatar Jul 10 '23 14:07 AlanCoding

Things to do on this one:

  • set things up to make RemovedInXWarnings instead be errors for testing on djmain.
  • concretise the existing index names when setting up the IndexTogether relations, making sure to not generate any actual SQL operations

rtpg avatar Jul 24 '23 05:07 rtpg

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"

ganeshprasadrao avatar Sep 08 '23 14:09 ganeshprasadrao