Opened 8 years ago

Closed 2 years ago

Last modified 10 months ago

#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 Akshesh Doshi, 8 years ago

This ticket tracks a sub-part of this deprecation process - https://code.djangoproject.com/ticket/27064

comment:2 by Tim Graham, 8 years ago

Triage Stage: UnreviewedAccepted
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 Akshesh Doshi, 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:4 by Akshesh Doshi, 8 years ago

Keywords: index_together added

Changed the keywords by mistake.

comment:5 by Olivier Tabone, 8 years ago

Cc: olivier.tabone@… added
Owner: changed from nobody to Olivier Tabone
Status: newassigned

working on this ticket during duth sprint

comment:6 by Olivier Tabone, 8 years ago

see https://github.com/django/django/pull/7509

any comment appreciated

comment:7 by Olivier Tabone, 8 years ago

Has patch: set

comment:8 by Tim Graham, 8 years ago

Patch needs improvement: set

I left comments for improvement.

comment:9 by Markus Holtermann, 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 Olivier Tabone, 8 years ago

Hi Markus, Thank you for your reply.

Are you referring to #27064 ? or is it something different ?

comment:11 by Tim Graham, 8 years ago

I think that's the correct ticket.

comment:12 by David Wobrock, 2 years ago

Cc: David Wobrock added
Owner: changed from Olivier Tabone to David Wobrock

Since #27064 is done, I'll try to tackle this issue.
Here is a new PR based a bit off the previous one.

I changed the assignee, since it was set about 6 years ago :)

Last edited 2 years ago by Tim Graham (previous) (diff)

comment:13 by David Wobrock, 2 years ago

Patch needs improvement: unset

comment:14 by Mariusz Felisiak, 2 years ago

Needs documentation: set
Patch needs improvement: set

comment:15 by David Wobrock, 2 years ago

Needs documentation: unset
Patch needs improvement: unset

comment:16 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In a3a1290d:

Refs #27236 -- Moved models with Meta.index_together inside of test methods.

comment:17 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In 457cfd6f:

Refs #27236 -- Added test_autodetector.BaseAutodetectorTests.

comment:18 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In db588d4f:

Refs #27236 -- Split index_together and unique_together autodetector tests.

comment:19 by Mariusz Felisiak, 2 years ago

Patch needs improvement: set

comment:20 by David Wobrock, 2 years ago

Patch needs improvement: unset

comment:21 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In 57793b47:

Refs #27236 -- Refactored out DeprecationForHistoricalMigrationMixin.

comment:22 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In 41019e48:

Refs #27236 -- Added generic mechanism to handle the deprecation of migration operations.

comment:23 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In f8f16b3c:

Refs #27236 -- Removed usage of Meta.index_together from indexes/introspection test models.

comment:24 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In 4f284115:

Refs #27236 -- Split RenameField() tests with unique/index_together.

comment:25 by Mariusz Felisiak, 2 years ago

Triage Stage: AcceptedReady for checkin

comment:26 by David Wobrock <david.wobrock@…>, 2 years ago

Resolution: fixed
Status: assignedclosed

In a6385b3:

Fixed #27236 -- Deprecated Meta.index_together in favor of Meta.indexes.

This also deprecates AlterIndexTogether migration operation.

comment:27 by Collin Anderson, 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 Simon Charette, 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 Mariusz Felisiak, 2 years ago

We should probably run check_deprecation_details() only for not applied migrations. What do you think?

comment:30 by Mariusz Felisiak, 2 years ago

comment:31 by Simon Charette, 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.

Last edited 2 years ago by Simon Charette (previous) (diff)

comment:32 by Collin Anderson, 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 Mariusz Felisiak, 2 years ago

Cc: Carlton Gibson added

I think we need all three:

  • more descriptive release notes for AlterIndexTogether,
  • running checks only on unapplied migrations, and
  • reducing AlterIndexTogether with RenameIndex.

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 with options={index_together=[...]} with the current state of index_together (as long as all fields existed when the model was created),
  • replace AlterIndexTogether with AddIndex/RemoveIndex.

comment:34 by Simon Charette, 2 years ago

Cc: Simon Charette added

comment:35 by Mariusz Felisiak, 2 years ago

Has patch: unset
Resolution: fixed
Severity: NormalRelease blocker
Status: closednew
Triage Stage: Ready for checkinAccepted

comment:36 by Mariusz Felisiak, 2 years ago

Alternatively, we can revert 57793b47657ace966ce8ce96d801ac0d85e5efc6, 41019e48bbf082c985e6ba3bad34d118b903bff1, and partly a6385b382e05a614a99e5a5913d8e631823159a2 and leave AlterIndexTogether fully functional forever.

comment:37 by Collin Anderson, 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 Mariusz Felisiak, 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:39 by Mariusz Felisiak, 2 years ago

Alternative PR.

comment:40 by Collin Anderson, 2 years ago

Cc: Collin Anderson added

comment:41 by GitHub <noreply@…>, 2 years ago

In f810325:

Refs #27236 -- Made cosmetic edits to Meta.index_together deprecation.

This should make it more straightforward to move forward when
deprecation ends.

comment:42 by Carlton Gibson, 2 years ago

Triage Stage: AcceptedReady for checkin

comment:43 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In c773d579:

Refs #27236 -- Reverted AlterIndexTogether deprecation.

This partly reverts a6385b382e05a614a99e5a5913d8e631823159a2.

comment:44 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In a1e9e9ab:

Refs #27236 -- Reverted "Refs #27236 -- Added generic mechanism to handle the deprecation of migration operations."

This reverts commit 41019e48bbf082c985e6ba3bad34d118b903bff1.

comment:45 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In 66f30dbf:

Refs #27236 -- Reverted "Refs #27236 -- Refactored out DeprecationForHistoricalMigrationMixin."

This reverts commit 57793b47657ace966ce8ce96d801ac0d85e5efc6.

comment:46 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In 7e3c9c32:

Refs #27236 -- Doc'd that AlterIndexTogether is no longer officially supported for Django 4.2+ migration files.

comment:47 by Mariusz Felisiak, 2 years ago

Resolution: fixed
Status: newclosed

comment:48 by GitHub <noreply@…>, 15 months ago

In ca5d3c9:

Refs #27236 -- Made more cosmetic edits to Meta.index_together deprecation.

comment:49 by Mariusz Felisiak <felisiak.mariusz@…>, 10 months ago

In 2abf417c:

Refs #27236 -- Removed Meta.index_together per deprecation timeline.

Note: See TracTickets for help on using tickets.
Back to Top