Opened 6 years ago

Closed 2 weeks 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 (47)

comment:1 Changed 6 years ago by Akshesh Doshi

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

comment:2 Changed 6 years ago by Tim Graham

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 Changed 6 years ago by Akshesh Doshi

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 Changed 6 years ago by Akshesh Doshi

Keywords: index_together added

Changed the keywords by mistake.

comment:5 Changed 6 years ago by Olivier Tabone

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

working on this ticket during duth sprint

comment:6 Changed 6 years ago by Olivier Tabone

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

any comment appreciated

comment:7 Changed 6 years ago by Olivier Tabone

Has patch: set

comment:8 Changed 6 years ago by Tim Graham

Patch needs improvement: set

I left comments for improvement.

comment:9 Changed 6 years ago by Markus Holtermann

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 Changed 6 years ago by Olivier Tabone

Hi Markus, Thank you for your reply.

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

comment:11 Changed 6 years ago by Tim Graham

I think that's the correct ticket.

comment:12 Changed 3 months ago by David Wobrock

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 3 months ago by Tim Graham (previous) (diff)

comment:13 Changed 3 months ago by David Wobrock

Patch needs improvement: unset

comment:14 Changed 2 months ago by Mariusz Felisiak

Needs documentation: set
Patch needs improvement: set

comment:15 Changed 2 months ago by David Wobrock

Needs documentation: unset
Patch needs improvement: unset

comment:16 Changed 2 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In a3a1290d:

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

comment:17 Changed 2 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 457cfd6f:

Refs #27236 -- Added test_autodetector.BaseAutodetectorTests.

comment:18 Changed 2 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In db588d4f:

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

comment:19 Changed 2 months ago by Mariusz Felisiak

Patch needs improvement: set

comment:20 Changed 8 weeks ago by David Wobrock

Patch needs improvement: unset

comment:21 Changed 5 weeks ago by Mariusz Felisiak <felisiak.mariusz@…>

In 57793b47:

Refs #27236 -- Refactored out DeprecationForHistoricalMigrationMixin.

comment:22 Changed 5 weeks ago by Mariusz Felisiak <felisiak.mariusz@…>

In 41019e48:

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

comment:23 Changed 4 weeks ago by Mariusz Felisiak <felisiak.mariusz@…>

In f8f16b3c:

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

comment:24 Changed 4 weeks ago by Mariusz Felisiak <felisiak.mariusz@…>

In 4f284115:

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

comment:25 Changed 4 weeks ago by Mariusz Felisiak

Triage Stage: AcceptedReady for checkin

comment:26 Changed 4 weeks ago by David Wobrock <david.wobrock@…>

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 Changed 4 weeks ago by Collin Anderson

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 Changed 4 weeks ago by Simon Charette

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 Changed 4 weeks ago by Mariusz Felisiak

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

comment:30 Changed 4 weeks ago by Mariusz Felisiak

comment:31 Changed 4 weeks ago by Simon Charette

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 4 weeks ago by Simon Charette (previous) (diff)

comment:32 Changed 4 weeks ago by Collin Anderson

"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 Changed 3 weeks ago by Mariusz Felisiak

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 Changed 3 weeks ago by Simon Charette

Cc: Simon Charette added

comment:35 Changed 3 weeks ago by Mariusz Felisiak

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

comment:36 Changed 3 weeks ago by Mariusz Felisiak

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

comment:37 Changed 3 weeks ago by Collin Anderson

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 Changed 3 weeks ago by Mariusz Felisiak

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 Changed 3 weeks ago by Mariusz Felisiak

Alternative PR.

comment:40 Changed 3 weeks ago by Collin Anderson

Cc: Collin Anderson added

comment:41 Changed 3 weeks ago by GitHub <noreply@…>

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 Changed 2 weeks ago by Carlton Gibson

Triage Stage: AcceptedReady for checkin

comment:43 Changed 2 weeks ago by Mariusz Felisiak <felisiak.mariusz@…>

In c773d579:

Refs #27236 -- Reverted AlterIndexTogether deprecation.

This partly reverts a6385b382e05a614a99e5a5913d8e631823159a2.

comment:44 Changed 2 weeks ago by Mariusz Felisiak <felisiak.mariusz@…>

In a1e9e9ab:

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

This reverts commit 41019e48bbf082c985e6ba3bad34d118b903bff1.

comment:45 Changed 2 weeks ago by Mariusz Felisiak <felisiak.mariusz@…>

In 66f30dbf:

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

This reverts commit 57793b47657ace966ce8ce96d801ac0d85e5efc6.

comment:46 Changed 2 weeks ago by Mariusz Felisiak <felisiak.mariusz@…>

In 7e3c9c32:

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

comment:47 Changed 2 weeks ago by Mariusz Felisiak

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top