Opened 5 years ago

Closed 2 years ago

#31335 closed Bug (fixed)

Renaming a ForeignKey included in an index, and removing UniqueConstraint, Index with ForeignKey fails on MySQL.

Reported by: Stephen Finucane Owned by: Sergey Fursov
Component: Migrations Version: 3.0
Severity: Normal Keywords:
Cc: Perry Harrington, David Wobrock Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I have a foreign key field on a model that I wish to rename. Unfortunately that field is included in an index. Attempting to rename this generates the following error.

MySQLdb._exceptions.OperationalError: (1553, "Cannot drop index 'foo_idx': needed in a foreign key constraint")

I've included reproduction steps along with a workaround below. This is an issue on both 3.0.3 and 1.11.28.


Create a sample project and application:

$ virtualenv venv
$ pip install django mysqlclient
$ django-admin startproject test
$ cd test/
$ django-admin startapp core

Configure test/settings.py to use the mysql backend and enable the core app, then create some sample models:

cat <<< EOF > core/models.py
import datetime
from django.db import models


class Article(models.Model):

    name = models.CharField(max_length=255, blank=True, null=False)
    date = models.DateTimeField(default=datetime.datetime.utcnow)
    body = models.TextField(blank=True, null=False)


class Comment(models.Model):

    article = models.ForeignKey(
        Article,
        related_name='comments',
        related_query_name='comment',
        on_delete=models.CASCADE,
    )
    date = models.DateTimeField(default=datetime.datetime.utcnow)
    body = models.TextField(blank=True, null=False)

    class Meta:
        indexes = [
            # This is a covering index for the /list/ query
            models.Index(
                fields=['article', 'date'],
                name='comment_list_covering_idx',
            ),
        ]
EOF

Once done, create and apply the migrations, then create some entries:

$ python manage.py makemigrations
$ python manage.py migrate
$ python manage.py shell
>>> from core.models import Article, Comment
>>> article_a = Article(name='Sample article', body='foo').save()
>>> article_b = Article(name='Another article', body='bar').save()
>>> comment_1 = Comment(article=article_a)
>>> comment_2 = Comment(article=article_b, body='moar stuff')

Rename the foreign key field, for example from article to foo, updating the index in the process, and generate the migration:

$ python manage.py makemigrations
Did you rename comment.article to comment.foo (a ForeignKey)? [y/N] y
Migrations for 'core':
  core/migrations/0002_auto_20200303_1424.py
    - Remove index comment_list_covering_idx from comment
    - Rename field article on comment to foo
    - Create index comment_list_covering_idx on field(s) foo, date of model comment

Attempt to run that migration. It will fail:

$ python manage.py migrate
Operations to perform:
  Apply all migrations: admin, auth, contenttypes, core, sessions
Running migrations:
  Applying core.0002_auto_20200303_1424...Traceback (most recent call last):
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/django/db/backends/utils.py", line 86, in _execute
    return self.cursor.execute(sql, params)
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/django/db/backends/mysql/base.py", line 74, in execute
    return self.cursor.execute(query, args)
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/MySQLdb/cursors.py", line 209, in execute
    res = self._query(query)
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/MySQLdb/cursors.py", line 315, in _query
    db.query(q)
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/MySQLdb/connections.py", line 239, in query
    _mysql.connection.query(self, query)
MySQLdb._exceptions.OperationalError: (1553, "Cannot drop index 'comment_list_covering_idx': needed in a foreign key constraint")
The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "manage.py", line 21, in <module>
    main()
  File "manage.py", line 17, in main
    execute_from_command_line(sys.argv)
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/django/core/management/__init__.py", line 401, in execute_from_command_line
    utility.execute()
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/django/core/management/__init__.py", line 395, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/django/core/management/base.py", line 328, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/django/core/management/base.py", line 369, in execute
    output = self.handle(*args, **options)
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/django/core/management/base.py", line 83, in wrapped
    res = handle_func(*args, **kwargs)
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/django/core/management/commands/migrate.py", line 233, in handle
    fake_initial=fake_initial,
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/django/db/migrations/executor.py", line 117, in migrate
    state = self._migrate_all_forwards(state, plan, full_plan, fake=fake, fake_initial=fake_initial)
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/django/db/migrations/executor.py", line 147, in _migrate_all_forwards
    state = self.apply_migration(state, migration, fake=fake, fake_initial=fake_initial)
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/django/db/migrations/executor.py", line 245, in apply_migration
    state = migration.apply(state, schema_editor)
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/django/db/migrations/migration.py", line 124, in apply
    operation.database_forwards(self.app_label, schema_editor, old_state, project_state)
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/django/db/migrations/operations/models.py", line 783, in database_forwards
    schema_editor.remove_index(model, index)
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/django/db/backends/base/schema.py", line 356, in remove_index
    self.execute(index.remove_sql(model, self))
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/django/db/backends/base/schema.py", line 142, in execute
    cursor.execute(sql, params)
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/django/db/backends/utils.py", line 100, in execute
    return super().execute(sql, params)
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/django/db/backends/utils.py", line 68, in execute
    return self._execute_with_wrappers(sql, params, many=False, executor=self._execute)
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/django/db/backends/utils.py", line 77, in _execute_with_wrappers
    return executor(sql, params, many, context)
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/django/db/backends/utils.py", line 86, in _execute
    return self.cursor.execute(sql, params)
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/django/db/utils.py", line 90, in __exit__
    raise dj_exc_value.with_traceback(traceback) from exc_value
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/django/db/backends/utils.py", line 86, in _execute
    return self.cursor.execute(sql, params)
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/django/db/backends/mysql/base.py", line 74, in execute
    return self.cursor.execute(query, args)
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/MySQLdb/cursors.py", line 209, in execute
    res = self._query(query)
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/MySQLdb/cursors.py", line 315, in _query
    db.query(q)
  File "/tmp/django-bug/venv/lib/python3.7/site-packages/MySQLdb/connections.py", line 239, in query
    _mysql.connection.query(self, query)
django.db.utils.OperationalError: (1553, "Cannot drop index 'comment_list_covering_idx': needed in a foreign key constraint")

The only workaround I've seen is to use AlterField to remove the fk constraint on the article field before doing this work, then use AlterField again after to re-add it. Perhaps that needs to happen under the hood if MySQL can't do this?

Output of pip freeze and the migrations below:

$ pip freeze
asgiref==3.2.3
Django==3.0.3
mysqlclient==1.4.6
pytz==2019.3
sqlparse==0.3.1
$ cat core/migrations/0001_initial.py 
# Generated by Django 3.0.3 on 2020-03-03 14:22

import datetime
from django.db import migrations, models
import django.db.models.deletion


class Migration(migrations.Migration):

    initial = True

    dependencies = [
    ]

    operations = [
        migrations.CreateModel(
            name='Article',
            fields=[
                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
                ('name', models.CharField(blank=True, max_length=255)),
                ('date', models.DateTimeField(default=datetime.datetime.utcnow)),
                ('body', models.TextField(blank=True)),
            ],
        ),
        migrations.CreateModel(
            name='Comment',
            fields=[
                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
                ('date', models.DateTimeField(default=datetime.datetime.utcnow)),
                ('body', models.TextField(blank=True)),
                ('article', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='comments', related_query_name='comment', to='core.Article')),
            ],
        ),
        migrations.AddIndex(
            model_name='comment',
            index=models.Index(fields=['article', 'date'], name='comment_list_covering_idx'),
        ),
    ]
$ cat core/migrations/0002_auto_20200303_1424.py 
# Generated by Django 3.0.3 on 2020-03-03 14:24

from django.db import migrations, models


class Migration(migrations.Migration):

    dependencies = [
        ('core', '0001_initial'),
    ]

    operations = [
        migrations.RemoveIndex(
            model_name='comment',
            name='comment_list_covering_idx',
        ),
        migrations.RenameField(
            model_name='comment',
            old_name='article',
            new_name='foo',
        ),
        migrations.AddIndex(
            model_name='comment',
            index=models.Index(fields=['foo', 'date'], name='comment_list_covering_idx'),
        ),
    ]

Change History (22)

comment:1 by Stephen Finucane, 5 years ago

Here's the modified migration that works around the issue, for completeness' sake:

$ cat core/migrations/0002_auto_20200303_1424.py
# Generated by Django 3.0.3 on 2020-03-03 14:24

from django.db import migrations, models
import django.db.models.deletion


class Migration(migrations.Migration):

    dependencies = [
        ('core', '0001_initial'),
    ]

    operations = [
        migrations.AlterField(
            model_name='comment',
            name='article',
            field=models.ForeignKey(
                db_constraint=False,
                on_delete=django.db.models.deletion.CASCADE,
                related_name='comments',
                related_query_name='comment',
                to='core.Article',
            ),
        ),
        migrations.RemoveIndex(
            model_name='comment',
            name='comment_list_covering_idx',
        ),
        migrations.RenameField(
            model_name='comment',
            old_name='article',
            new_name='foo',
        ),
        migrations.AddIndex(
            model_name='comment',
            index=models.Index(fields=['foo', 'date'], name='comment_list_covering_idx'),
        ),
        migrations.AlterField(
            model_name='comment',
            name='foo',
            field=models.ForeignKey(
                on_delete=django.db.models.deletion.CASCADE,
                related_name='comments',
                related_query_name='comment',
                to='core.Article',
            ),
        ),
    ]

comment:2 by Simon Charette, 5 years ago

Component: UncategorizedMigrations
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

Looks like MySQL will be smart enough to silently drop the index on article_id when comment_list_covering_idx is created but decides to do otherwise when the latter is dropped.

From MySQL's docs

MySQL requires indexes on foreign keys and referenced keys so that foreign key checks can be fast and not require a table scan. In the referencing table, there must be an index where the foreign key columns are listed as the first columns in the same order. Such an index is created on the referencing table automatically if it does not exist. This index might be silently dropped later if you create another index that can be used to enforce the foreign key constraint. index_name, if given, is used as described previously.

So it looks like MySQL's schema editor remove_index logic needs to handle that case by manually creating an index for index.fields[0] if it's a foreign key.

FWIW it looks like the AddIndex and RemoveIndex here are completely unnecessary and that the auto-detector and RenameField field operation should be taught about avoiding it just like they do with index_together. That should be tracked in a different optimization ticket though as this issue is still relevant in cases where an Index is explicitly removed.

Version 0, edited 5 years ago by Simon Charette (next)

comment:3 by Simon Charette, 5 years ago

This is the same issue as #24757 but for Meta.indexes and Meta.constraints (for index backed constraints such as UniqueConstraint).

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

comment:4 by Mariusz Felisiak, 4 years ago

#31746 was marked as a duplicate (removing UniqueConstraint with ForeignKey on MySQL).

comment:5 by Perry Harrington, 4 years ago

There is a variation of this bug that goes to the core of the problem.

You have a model like this:

class Insect....
species = fk to model Species
has_wings = Boolean

You create the initial migration and it generates roughly this:

Table Insect
id
species_id
has_wings bool
PRIMARY_KEY(id)
KEY someconstraint (species_id)
CONSTRAINT someconstraint FK (species_id) REFERENCES species (id)

The CONSTRAINT keyword in the initial sql migration script will automatically create an index named the same as the related name.

Django doesn't know about this magical key, since it didn't create it.

Next you add a new index with the following:

indexes = [
...Index(fields=['species_id','id','has_wings'])
]

The migration then creates something like this:

ALTER TABLE ADD INDEX someindex (species_id,id,has_wings)

Then MySQL silently deletes the supporting FK constraint index that it created, replacing that single column index with the compound index you just specified.

The *crucial* factor in MySQL's decision to drop the automagic index is whether the FK is the first column in the index.

If the FK is the first column of the secondary index, MySQL drops it because the automagic index is a duplicate.

Next, if you instead decide to change the index to:

indexes = [
...Index(fields=['id','has_wings'])
]

When you run the migration, Django generates a DROP index for the secondary index, but because it's used as the FK constrain index, MySQL refuses.

The same would happen if you changed the order of the columns in the index (because column order is critical to access path):

indexes = [
...Index(fields=['id','species_id','has_wings'])
]

This may look like the same index, but it's not, and Django will drop the old one and create the new one in the new order, generating another FK error.

There are a few problems here:

  • Django doesn't know about the shadow index and therefore its dependency calculation does not take into account MySQL automagically creating and dropping the FK dependent index
  • Django DROPs indexes before creating new indexes

First, Django should create indexes before dropping indexes, this will automatically resolve the dependency issue if the new index has the FK as the first column of the index.

Second, if Django is creating a migration for a table where the FK was the first column in a previously seen index, but that index is gone in the new migration and there is no new index with the FK as the first column, then it should automatically generate an FK constraint index with the name of the FK constraint.

Put another way:
if old index set contained FK as first column, but new index set does not have any index with FK as first column, generate a new KEY with just FK as column.
Then you can drop old index and add new indexes.

But, belt and suspenders, you should add indexes before dropping indexes, this can help cover corner cases.

comment:6 by Perry Harrington, 4 years ago

Cc: Perry Harrington added

comment:7 by Jacob Walls, 4 years ago

Has patch: set
Owner: changed from nobody to Sanskar Jaiswal
Status: newassigned

comment:8 by Mariusz Felisiak, 4 years ago

Needs tests: set

comment:9 by Sanskar Jaiswal, 4 years ago

I have a working patch, but I am having trouble writing a clean test case for the same. The test involves removing an index on ForiegnKey, renaming the ForeignKey and adding the index back on the same. Since the schema tests involve using the editor directly, it doesn't affect the ModelState, which leads to a bit of a problem. When we alter the ForeignKey field, the column name is changed, while the field name remains the same. This leads to an inconsistent state when I try to add the index back on the renamed ForiegnKey field.
Since, the fields property of Model._meta is immutable, is it advisable to use migration operations to update the same?

comment:10 by Mariusz Felisiak, 3 years ago

Summary: [mysql] Renaming a foreign key field that's also included in an index fails with "Cannot drop index 'foo_idx': needed in a foreign key constraint"Renaming a ForeignKey included in an index, and removing UniqueConstraint, Index with ForeignKey fails on MySQL.

#33392 was marked as a duplicate (removing UniqueConstraint, Index with ForeignKey on MySQL).

comment:11 by Jacob Walls, 3 years ago

Needs tests: unset
Owner: changed from Sanskar Jaiswal to Sergey Fursov

PR

Not completely certain yet if the PR addresses the entirety of the comments on this thread, but just linking to increase visibility.

comment:12 by Mariusz Felisiak, 3 years ago

Needs tests: set
Patch needs improvement: set

comment:13 by 777GE90, 3 years ago

I ran into this when upgrading Django from 1.X to 2.2.25, essentially we had a historical migration that renamed a foreign key field that was indexed. The newer version of Django seems to create a migration which does RemoveIndex and AddIndex, which doesn't work since the index is already renamed in the database and doesn't exist.

As a workaround I resolved it by modifying the old migration files so that it does not seem like the field was ever renamed (i.e. update the original files to include the latest name).

comment:14 by Sergey Fursov, 3 years ago

Needs tests: unset
Patch needs improvement: unset

comment:15 by Mariusz Felisiak, 2 years ago

Patch needs improvement: set

comment:16 by Mariusz Felisiak, 2 years ago

Cc: David Wobrock added

comment:17 by Sergey Fursov, 2 years ago

Patch needs improvement: unset

comment:18 by Mariusz Felisiak, 2 years ago

Patch needs improvement: set

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

In ec13e801:

Refs #31335 -- Added SchemaEditor._create_missing_fk_index() on MySQL.

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

In 1b08e9b:

Refs #31335 -- Added more tests for removing composed Meta constraints/indexes on foreign keys.

comment:21 by Mariusz Felisiak, 2 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

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

Resolution: fixed
Status: assignedclosed

In b731e88:

Fixed #31335 -- Fixed removing composed composed Meta constraints/indexes on foreign keys on MySQL.

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