Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#27558 closed Bug (fixed)

Setting db_index=False on existing ForeignKey causes constraint to be recreated on MySQL

Reported by: Ed Morley Owned by: Ed Morley
Component: Database layer (models, ORM) Version: 1.10
Severity: Normal Keywords: db-indexes, mysql
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Using:

  • Django 1.10.3
  • MySQL 5.6

STR:
1) Start with this models.py:

class Musician(models.Model):
    first_name = models.CharField(max_length=50)
    last_name = models.CharField(max_length=50)


class Album(models.Model):
    artist = models.ForeignKey(Musician)
    name = models.CharField(max_length=100)

    class Meta:
        unique_together = ('artist', 'name')

2) Run ./manage.py makemigrations && ./manage.py migrate to generate and apply the initial migration.
3) Add db_index=False to the ForeignKey (to work around the duplicate index issue discussed here) - making it:

    artist = models.ForeignKey(Musician, db_index=False)

4) Run ./manage.py makemigrations
5) Run ./manage.py sqlmigrate <foo> 0002 to display the generated SQL

Expected:
Just the index is dropped.

BEGIN;
--
-- Alter field artist on album
--
DROP INDEX `foo_album_ca949605` ON `foo_album`;
COMMIT;

Actual:

The constraint is removed and re-added, which is time consuming since the constraint is an index in itself.

BEGIN;
--
-- Alter field artist on album
--
ALTER TABLE `foo_album` DROP FOREIGN KEY `foo_album_artist_id_66b4953c_fk_foo_musician_id`;
DROP INDEX `foo_album_ca949605` ON `foo_album`;
ALTER TABLE `foo_album` ADD CONSTRAINT `foo_album_artist_id_66b4953c_fk_foo_musician_id` FOREIGN KEY (`artist_id`) REFERENCES `foo_musician` (`id`);
COMMIT;

Additional context:

  • The explicit db_index=False is needed to work around the duplicate index issue discussed here.
  • The duplicate index issue appears to be fixed on master (not sure by what) causing this not to repro there.
  • I'm presuming the duplicate index isn't severe enough to warrant backporting, however dropping and re-adding the constraint seems pretty bad, so perhaps more worthy of a backport to 1.10.x?

Change History (9)

comment:1 Changed 3 years ago by Ed Morley

As a workaround in the meantime, I was thinking of replacing the auto-generated migration with something like:

migrations.RunSQL(
    "DROP INDEX `<name>` ON `foo_album`;",
    state_operations=[
        migrations.AlterField(
            model_name='album',
            name='artist',
            field=models.ForeignKey(db_index=False, on_delete=django.db.models.deletion.CASCADE, to='foo.Musician'),
        ),
    ],
)

However I haven't yet figured out the best way to get the existing index name from Django.

comment:2 Changed 3 years ago by Tim Graham

If it's a big performance problem, we can likely consider a backport as long as the patch isn't too complex. Could you try bisecting to find the commit that fixed it?

comment:3 Changed 3 years ago by Ed Morley

Ah thank you!

Bisecting shows the redundant index issue was fixed as of:
https://github.com/django/django/commit/3f76d1402dac9c2993d588f996dc1c331edbc9a7

Whilst the change to add_field() in django/db/backends/base/schema.py might look like a no-op (since the _field_should_be_indexed() method in that file does the same thing as the lines being replaced), it actually wasn't - since mysql overrides the base self._field_should_be_indexed() with it's own that handles ForeignKey properly:
https://github.com/django/django/blob/3f76d1402dac9c2993d588f996dc1c331edbc9a7/django/db/backends/mysql/schema.py#L54-L67

comment:4 Changed 3 years ago by Tim Graham

Summary: Setting db_index=False on existing ForeignKey causes constraint to be recreatedSetting db_index=False on existing ForeignKey causes constraint to be recreated on MySQL
Triage Stage: UnreviewedAccepted

A less invasive fix for 1.10 is probably needed as the changing and adding of methods in that commit looks a bit risky for a stable release. The regression test can also be added to master.

comment:5 Changed 3 years ago by Ed Morley

Owner: changed from nobody to Ed Morley
Status: newassigned

I have a reduced testcase for master + 1.10.x branch, and a 1 liner to fix the issue on 1.10.x.
Will clean up soon and open PRs.

comment:6 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 82ce55d:

[1.10.x] Fixed #27558 -- Prevented redundant index on InnoDB ForeignKey.

The MySQL backend overrides _field_should_be_indexed() so that it skips
index creation for ForeignKeys when using InnoDB.

comment:7 Changed 3 years ago by Tim Graham <timograham@…>

In dd2e4d7b:

Refs #27558 -- Added test for no index on InnoDB ForeignKey.

The refactor in 3f76d1402dac9c2993d588f996dc1c331edbc9a7 fixed the creation
of redundant indexes.

Forwardport of 82ce55dbbe2d96e8b5d1fcb4a1d52b73e08e7929 from stable/1.10.x

comment:8 Changed 3 years ago by GitHub <noreply@…>

In f94475e:

Refs #27558 -- Isolated indexes test on MySQL.

MySQL schema changes must be done in TransactionTestCase.

comment:9 Changed 3 years ago by Tim Graham <timograham@…>

In 4086ff9e:

[1.10.x] Refs #27558 -- Isolated indexes test on MySQL.

MySQL schema changes must be done in TransactionTestCase.

Backport of f94475e5262abe78d9771bc353f741b20cc3c725 from master

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