Opened 6 years ago

Closed 7 months ago

#27064 closed New feature (fixed)

Implement RenameIndex in a backwards compatible way

Reported by: Markus Holtermann Owned by: David Wobrock
Component: Migrations Version: dev
Severity: Normal Keywords:
Cc: Andrew Godwin, Akshesh Doshi, Adam Johnson, Ravi Teja P, David Wobrock Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In order to eventually deprecate index_together we need a way to deal with old projects that have unnamed indexes. This proves to be a non-trivial problem. Andrew and I came up with these things to consider.

  • RenameIndex(model, new_name, old_name=None, old_fields=None) where exactly one of old_name and old_field is given (old_name ^ old_fields)
  • If the old_name is given we use RENAME INDEX if available
  • Otherwise look at the state and drop existing indexes and create new the index with new name
  • On MySQL (or other DBs) that don't support RENAME INDEX, provide SQL query to look up index name from information_schema by field names and pass in to DROP INDEX.
  • If more than one index is found while identifying with field names, migrations must error out with an AmbiguityError
  • If the autodetector finds an old, unnamed index and a new, named one matching field signature, issue a RenameIndex operation
  • For backwards operations with unnamed old indexes, RenameIndex is a noop.

Change History (24)

comment:1 Changed 6 years ago by Markus Holtermann

Type: UncategorizedNew feature

comment:2 Changed 6 years ago by Markus Holtermann

Cc: Akshesh Doshi added

comment:3 Changed 6 years ago by Markus Holtermann

Cc: Andrew Godwin added; anr removed

comment:4 Changed 6 years ago by Akshesh Doshi

Is there a reason why we cannot just simply deprecate index_together.

My understanding is that the models file of the app will get modified from this

    class Meta:
        index_together = (('a', 'b'))

to this

    class Meta:
        indexes = [models.Index(fields=['a', 'b'])]

and everything should work fine (or not? Am I missing something?)

I need some help to see where RenameIndex comes into picture here.

comment:5 Changed 6 years ago by Markus Holtermann

I just noticed that I actually never commented here. So, the reason is explained here: https://code.djangoproject.com/ticket/27236#comment:9

comment:6 Changed 3 years ago by Mariusz Felisiak

On MySQL (or other DBs) that don't support RENAME INDEX, provide SQL query to look up index name from information_schema by field names and pass in to DROP INDEX.

RENAME INDEX is now supported on MySQL 5.7+ and MariaDB 10.5.2+.

comment:7 Changed 3 years ago by Adam Johnson

Cc: Adam Johnson added

comment:8 Changed 2 years ago by Ravi Teja P

Cc: Ravi Teja P added

comment:9 Changed 7 months ago by David Wobrock

Cc: David Wobrock added
Has patch: set
Needs documentation: unset
Needs tests: unset
Owner: changed from nobody to David Wobrock
Status: newassigned

Hi there,

I tried to tackle this change in this PR.

Feel free to review it!
And maybe we'll be able to deprecate index_together in a next major release, as we wanted to do here https://code.djangoproject.com/ticket/27236 a few years back :)

comment:10 Changed 7 months ago by Mariusz Felisiak

Patch needs improvement: set

comment:11 Changed 7 months ago by David Wobrock

Patch needs improvement: unset

Integrated the review comments and split the PR into 2 parts:

  1. Adding the RenameIndex operation https://github.com/django/django/pull/15677
  2. Using the RenameIndex in the autodetector https://github.com/django/django/pull/15651

comment:12 Changed 7 months ago by Mariusz Felisiak

Needs tests: set
Patch needs improvement: set

comment:13 Changed 7 months ago by David Wobrock

Needs tests: unset
Patch needs improvement: unset

Integrated review changes into the first PR implementing RenameIndex operation.

comment:14 Changed 7 months ago by Mariusz Felisiak

Triage Stage: AcceptedReady for checkin

comment:15 Changed 7 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In eacd4977:

Refs #27064 -- Added RenameIndex migration operation.

comment:16 Changed 7 months ago by Mariusz Felisiak

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

comment:17 Changed 7 months ago by David Wobrock

Patch needs improvement: unset

Rebased PR, ready for some review again :)
Even if it (most probably) won't be included to the coming 4.1 release.

comment:18 Changed 7 months ago by Mariusz Felisiak

Patch needs improvement: set

It's blocked by #33710.

comment:19 Changed 7 months ago by Mariusz Felisiak

Patch needs improvement: unset

comment:20 Changed 7 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In c6cec3c2:

Refs #27064 -- Made migrations generate RenameIndex operations when renaming Meta.indexes.

comment:21 Changed 7 months ago by Mariusz Felisiak

Triage Stage: AcceptedReady for checkin

comment:22 Changed 7 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In a098cde9:

Refs #27064 -- Refactored out MigrationAutodetector.create_renamed_fields().

comment:23 Changed 7 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 97f124f:

Refs #27064 -- Made migrations generate RenameIndex operations when moving indexes from index_together to Meta.indexes.

comment:24 Changed 7 months ago by Mariusz Felisiak

Resolution: fixed
Status: assignedclosed
Triage Stage: Ready for checkinAccepted
Note: See TracTickets for help on using tickets.
Back to Top