#27236 closed Cleanup/optimization (fixed)
Deprecate Model.Meta.index_together in favour of Model.Meta.indexes
Reported by: | Akshesh Doshi | Owned by: | David Wobrock |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Release blocker | Keywords: | index_together |
Cc: | olivier.tabone@…, David Wobrock, Carlton Gibson, Simon Charette, Collin Anderson | Triage Stage: | Ready for checkin |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Anything that index_together
does can be done by indexes
and the Index
class.
Change History (49)
comment:1 by , 8 years ago
comment:2 by , 8 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Version: | → master |
My understanding is that we're keeping the AlterIndexTogether
migration operation around indefinitely for support in historical migrations. Are there any other details from our conversations to add?
comment:3 by , 8 years ago
Keywords: | db-indexes index_together removed |
---|
Yes, since AlterIndexTogether
is currently there in the migrations of so many Django projects we need to keep maintaining it. But the idea is to deprecate the use of index_together
in the models file and the generate_altered_index_together
method of the MigrationAutodetector
.
comment:5 by , 8 years ago
Cc: | added |
---|---|
Owner: | changed from | to
Status: | new → assigned |
working on this ticket during duth sprint
comment:7 by , 8 years ago
Has patch: | set |
---|
comment:9 by , 8 years ago
While this is the one step of the deprecation process of index_together
there is currently no way to migrate an existing from index_together
to indexes
w/o dropping and recreating indexes. Since removing and adding an index can be a quite expensive operation, this is not an option to do so for now.
The next step in the deprecation process needs to treat indexes
and index_together
similarly in the sense that the latter is translated into the former internally (inside the migration framework). Once this is done, the deprecation of actually using index_together
can start.
comment:10 by , 8 years ago
Hi Markus, Thank you for your reply.
Are you referring to #27064 ? or is it something different ?
comment:12 by , 2 years ago
Cc: | added |
---|---|
Owner: | changed from | to
comment:13 by , 2 years ago
Patch needs improvement: | unset |
---|
comment:14 by , 2 years ago
Needs documentation: | set |
---|---|
Patch needs improvement: | set |
comment:15 by , 2 years ago
Needs documentation: | unset |
---|---|
Patch needs improvement: | unset |
Here we go again! With 3 PRs:
The main PR
Inlining test classes
Splitting index/unique together autodetector tests
comment:19 by , 2 years ago
Patch needs improvement: | set |
---|
comment:20 by , 2 years ago
Patch needs improvement: | unset |
---|
comment:25 by , 2 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:27 by , 2 years ago
I'm getting system check error: "AlterIndexTogether is deprecated. Support for it (except in historical migrations) will be removed in Django 5.1.", even though it's only used in historical migrations. Is there something I'm supposed to do? Or why give a warning if I don't need to take any action? Is there any way to clarify this? (Same with release notes deprecation section, very unclear what action I'm supposed to take.)
comment:28 by , 2 years ago
I think you'll want to squash the migrations containing such operations. I'm not sure if the warnings point at doing that but if it's not the case it definitely should as this will be a common issue given AlterIndexTogether
has been around forever.
comment:29 by , 2 years ago
We should probably run check_deprecation_details()
only for not applied migrations. What do you think?
comment:31 by , 2 years ago
We should probably run check_deprecation_details() only for not applied migrations. What do you think?
I think this will go a long way in avoiding to spam users every single time they run a management command that triggers system checks but it's not uncommon for developers to drop their local database and start from scratch.
Ultimately they'll have to squash their migrations before 5.1 if they want to avoid having their migration breaking so I wonder if we are doing them a favour by silencing the check which than can already silence using SILENCED_SYSTEM_CHECKS
.
I think the system check hint should point at the proper procedure of using indexes
and running makemigrations
at least?
Ultimately the only way for all the warnings and check errors to go away before 5.1 where AlterIndexTogether
will likely be marked as elidable = True
since it will be a noop is for users to manually edit their migrations to replace their usage of AlterIndexTogether
by AddIndex
and friends. I'm not sure the current documentation does a good job at explaining how this must be done as it's a relatively complex operation.
Maybe there something that could be done through RenameIndex.reduce(operation: AlterIndexTogether)
to ease the pain when old_fields
is present here and have migration squashing handle most of the cases? Otherwise I foresee a lot of reports coming our way on the 5.1 rollout saying that RenameIndex
is failing to run due to a missing index.
comment:32 by , 2 years ago
"run only on unapplied migrations"
makes sense to me.
"Ultimately they'll have to squash their migrations before 5.1 if they want to avoid having their migration breaking"
The deprecation warning/check says "Support for it (except in historical migrations) will be removed in Django 5.1", so that seems to say to me that it _won't_ be removed for historical migrations, and I as a developer don't need to worry about it for historical migrations.
I'm mostly just providing feedback here on the wording. If Django wants me to do something to my project to fix this issue, it should tell me what I should do (like squash, or is there an alternative migration operation that I should use instead?). Or if I don't need to do anything then there shouldn't be a warning.
comment:33 by , 2 years ago
Cc: | added |
---|
I think we need all three:
- more descriptive release notes for
AlterIndexTogether
, - running checks only on unapplied migrations, and
- reducing
AlterIndexTogether
withRenameIndex
.
Lots of folks don't recreate their databases so they won't have to do anything. As for the rest, we can document possible solutions, e.g.
- squash migrations,
- update the historical
CreateModel()
operation withoptions={index_together=[...]}
with the current state ofindex_together
(as long as all fields existed when the model was created), - replace
AlterIndexTogether
withAddIndex
/RemoveIndex
.
comment:34 by , 2 years ago
Cc: | added |
---|
comment:35 by , 2 years ago
Has patch: | unset |
---|---|
Resolution: | fixed |
Severity: | Normal → Release blocker |
Status: | closed → new |
Triage Stage: | Ready for checkin → Accepted |
comment:36 by , 2 years ago
Alternatively, we can revert 57793b47657ace966ce8ce96d801ac0d85e5efc6, 41019e48bbf082c985e6ba3bad34d118b903bff1, and partly a6385b382e05a614a99e5a5913d8e631823159a2 and leave AlterIndexTogether
fully functional forever.
comment:37 by , 2 years ago
I'm personally always in favor of keeping things fully functional forever, to reduce the amount of work that developers need to do, as long as it's easy enough to maintain ( https://groups.google.com/g/django-developers/c/zImXMKzWN4A/m/4SNu7MyFAQAJ ). If so, then there probably doesn't need to be a warning message at all, but certainly not for applied migrations, and no need for a deprecation timeline. Just discourage its use and move on. Again, as long as it's easy enough to maintain long-term.
If functionality is getting removed or no-op'd in the future, then I'd suggest that yes there should be a warning (not just for un-applied migrations), and in my opinion the warning message should suggest an alternative or at least link to docs about this deprecation and how to fix it (AddIndex/RemoveIndex or something?) And, ideally it would be a type of change that could be automatically applied using django-codemod or django-upgrade, but that might be asking for too much.
comment:38 by , 2 years ago
I'm longer convinced that we have to deprecate and remove it. We can document that is supported only in pre-Django 4.2 migrations and will not receive any bugfixes in the future.
Simon, What do you think?
comment:40 by , 2 years ago
Cc: | added |
---|
comment:42 by , 2 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:47 by , 2 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
This ticket tracks a sub-part of this deprecation process - https://code.djangoproject.com/ticket/27064