Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Django warnings about index_together with new migration #862

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AlanCoding
Copy link

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
Copy link
Contributor

rtpg commented Jul 6, 2023

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

@rtpg
Copy link
Contributor

rtpg commented Jul 6, 2023

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
Copy link
Author

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
Copy link
Contributor

rtpg commented Jul 24, 2023

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

@mister-rao
Copy link

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"

Comment on lines +12 to +16
migrations.RenameIndex(
model_name="taggeditem",
new_name="taggit_tagg_content_8fc721_idx",
old_fields=("content_type", "object_id"),
),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using migrations.SeparateDatabaseAndState might do the trick to do this with zero downtime as explained in this article: https://adamj.eu/tech/2020/07/27/how-to-modernize-your-django-index-definitions/

@rtpg rtpg mentioned this pull request Sep 20, 2023
@igorsimb
Copy link

Why is this not merged yet?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants