[Django] #27558: Setting db_index=False on existing ForeignKey causes constraint to be recreated
Django
recreated
-------------------------------------+-------------------------------------
Reporter: Ed Morley | Owner: nobody
Type: Bug | Status: new
Component: Database | Version: 1.10
layer (models, ORM) |
Severity: Normal | Keywords: db-indexes, mysql
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Using:
* Django 1.10.3
* MySQL 5.6
STR:
1) Start with this models.py:
{{{#!python
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 [https://groups.google.com/forum/#!topic/django-
developers/3ywugkcaxqs here]) - making it:
{{{#!python
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.
{{{#!sql
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.
{{{#!sql
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 [https://groups.google.com/forum/#!topic/django-
developers/3ywugkcaxqs 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/27558>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
recreated
-------------------------------------+-------------------------------------
Reporter: Ed Morley | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: db-indexes, mysql | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Ed Morley):
As a workaround in the meantime, I was thinking of replacing the auto-
generated migration with something like:
{{{#!python
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27558#comment:1>
Django
recreated
-------------------------------------+-------------------------------------
Reporter: Ed Morley | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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?
--
Ticket URL: <https://code.djangoproject.com/ticket/27558#comment:2>
Django
recreated
-------------------------------------+-------------------------------------
Reporter: Ed Morley | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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
--
Ticket URL: <https://code.djangoproject.com/ticket/27558#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Ed Morley | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27558#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Ed Morley | Owner: Ed Morley
Type: Bug | Status: assigned
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => assigned
* owner: nobody => Ed Morley
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27558#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Ed Morley | Owner: Ed Morley
Component: Database layer | Version: 1.10
(models, ORM) |
Keywords: db-indexes, mysql | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"82ce55dbbe2d96e8b5d1fcb4a1d52b73e08e7929" 82ce55d]:
{{{
#!CommitTicketReference repository=""
revision="82ce55dbbe2d96e8b5d1fcb4a1d52b73e08e7929"
[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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27558#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Ed Morley | Owner: Ed Morley
Type: Bug | Status: closed
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution: fixed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"dd2e4d7b5d6f24f33c2805b0bfb97a08e27b2125" dd2e4d7b]:
{{{
#!CommitTicketReference repository=""
revision="dd2e4d7b5d6f24f33c2805b0bfb97a08e27b2125"
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27558#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Ed Morley | Owner: Ed Morley
Type: Bug | Status: closed
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution: fixed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by GitHub <noreply@…>):
In [changeset:"f94475e5262abe78d9771bc353f741b20cc3c725" f94475e]:
{{{
#!CommitTicketReference repository=""
revision="f94475e5262abe78d9771bc353f741b20cc3c725"
Refs #27558 -- Isolated indexes test on MySQL.
MySQL schema changes must be done in TransactionTestCase.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27558#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Ed Morley | Owner: Ed Morley
Type: Bug | Status: closed
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution: fixed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"4086ff9ecbf4824cc5c0cc94e4c13ef474a22b9b" 4086ff9e]:
{{{
#!CommitTicketReference repository=""
revision="4086ff9ecbf4824cc5c0cc94e4c13ef474a22b9b"
[1.10.x] Refs #27558 -- Isolated indexes test on MySQL.
MySQL schema changes must be done in TransactionTestCase.
Backport of f94475e5262abe78d9771bc353f741b20cc3c725 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27558#comment:9>