Opened 9 years ago

Closed 4 years ago

#25253 closed Cleanup/optimization (fixed)

MySQL migrations drop & recreate constraints unnecessarily when changing attributes that don't affect the schema

Reported by: Thomas Recouvreux Owned by: Çağıl Uluşahin
Component: Migrations Version: 2.1
Severity: Normal Keywords: migrations m2m mysql
Cc: Phil Krylov, Adam Johnson 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

When running a MySQL migration to add blank=True on a ManyToManyField, Django drops the FK constraints on the join table and creates them back.

Here is the changeset for models.py:

from django.contrib.auth.models import User
from django.db import models


class Banana(models.Model):
-    users = models.ManyToManyField(User)
+    users = models.ManyToManyField(User, blank=True)

And here the corresponding migration:

..
    operations = [
        migrations.AlterField(
            model_name='banana',
            name='users',
            field=models.ManyToManyField(to=settings.AUTH_USER_MODEL, blank=True),
        ),
    ]
..

The resulting sql for MySQL backend is:

ALTER TABLE `myproject_banana_users` DROP FOREIGN KEY `myproject_banana_users_user_id_93c9ecfd1bc0dc2_fk_auth_user_id`;
ALTER TABLE `myproject_banana_users` ADD CONSTRAINT `myproject_banana_users_user_id_60a2450416dd445f_fk_auth_user_id` FOREIGN KEY (`user_id`) REFERENCES `auth_user` (`id`);
ALTER TABLE `myproject_banana_users` DROP FOREIGN KEY `myproject_bana_banana_id_4be506ef34515098_fk_myproject_banana_id`;
ALTER TABLE `myproject_banana_users` ADD CONSTRAINT `myproject_bana_banana_id_43426c15f7d142f5_fk_myproject_banana_id` FOREIGN KEY (`banana_id`) REFERENCES `myproject_banana` (`id`);

I checked on PostgreSQL the resulting sql is empty, which looks good to me since there is no change to issue to the database.

Change History (23)

comment:1 by Tim Graham, 9 years ago

I couldn't reproduce this on master or the stable/1.8.x branch. I turned on Django's logging and didn't see this queries among the SQL that is executed. Could you provide any additional information?

comment:2 by Thomas Recouvreux, 9 years ago

Hello,

You can find here the test project I built for this ticket : https://github.com/trecouvr/django_25253.

Just run ./manage.py sqlmigrate myproject 0002 to see the mentionned SQL query.

comment:3 by Tim Graham, 9 years ago

Did you try with Django 1.8.3 instead of 1.8 as listed in the requirements of that project? There have been a lot of bug fixes since 1.8 (not to mention security fixes!).

comment:4 by Thomas Recouvreux, 9 years ago

Yes, I tried with Django 1.8, 1.8.2 & 1.8.3

comment:5 by Tim Graham, 9 years ago

Triage Stage: UnreviewedAccepted

I can reproduce using your sample project.

comment:6 by Tim Graham, 9 years ago

Type: BugCleanup/optimization

comment:7 by Tim Graham, 9 years ago

Summary: Migrations do unecessary work when adding `blank=True` to M2M field on MySQLMigrations do unnecessary work when adding `blank=True` to M2M field on MySQL

comment:8 by Tim Graham, 9 years ago

Summary: Migrations do unnecessary work when adding `blank=True` to M2M field on MySQLMySQL migrations drop & recreate constraints unnecessarily when changing attributes that don't affect the schema

#25614 reported the same unnecessary dropping/adding of constraints when changing ForeignKey.on_delete. I assume that's a duplicate of this ticket, but please reopen it if fixing this issue doesn't also fix it.

comment:9 by Benjamin Wohlwend, 9 years ago

For what it's worth, I'm also seeing this on PostgreSQL 9.1, Django 1.9.5 and psycopg2 2.6.1. Specifically, adding an on_delete argument to a ForeignKey (but I was also able to reproduce it with just adding blank=True) and then running sqlmigrate on the generated migration results in this output:

BEGIN;
--
-- Alter field last_occurrence on event
--
ALTER TABLE "event_event" DROP CONSTRAINT "last_occurrence_id_refs_id_9b97c921";
ALTER TABLE "event_event" ADD CONSTRAINT "event_event_last_occurrence_id_a6d9e0d9_fk_event_instance_id" FOREIGN KEY ("last_occurrence_id") REFERENCES "event_instance" ("id") DEFERRABLE INITIALLY DEFERRED;

COMMIT;

It might be closer related to #25614 (which is marked as a duplicate of this bug), since it only drops/adds the constraint, but leaves the foreign key itself alone.

Last edited 9 years ago by Benjamin Wohlwend (previous) (diff)

comment:10 by Cameron Dawson, 7 years ago

I'm still hitting this with MySQL 5.7 and Django 1.11.3:

--
-- Alter field job on textlogstep
--
ALTER TABLE `text_log_step` DROP FOREIGN KEY `text_log_step_job_id_698f3bfb_fk_job_id`;
ALTER TABLE `text_log_step` ADD CONSTRAINT `text_log_step_job_id_698f3bfb_fk_job_id` FOREIGN KEY (`job_id`) REFERENCES `job` (`id`);
Last edited 7 years ago by Cameron Dawson (previous) (diff)

comment:11 by Shun Yu, 7 years ago

Owner: changed from nobody to Shun Yu
Status: newassigned

comment:14 by Mike Edmunds, 7 years ago

Still seems to be an issue in Django 2.0.5.

The source of the problem appears to be BaseDatabaseSchemaEditor._alter_field in django.db.backends.base.schema. It unconditionally drops any existing foreign key constraint(s) and later re-creates it—whether or not anything is changing. (It seems to be more selective about dropping other types of constraints only if the field is changing in a way that impacts them.) The origin of this code is 2012, I think during initial implementation of Django migrations.

Are there non-obvious field changes where the field's FK constraint needs to be dropped and re-created? Or some other reason this code runs unconditionally? Optimization around other field changes?

A fix might be to defer dropping the FK constraint until just before the field's actions/null_actions/post_actions are collected and ready to be applied (around line 650), and then drop it only if there are actually actions to execute.

comment:15 by Israel Saeta Pérez, 6 years ago

We had this problem with Postgres and Django 1.11 when deleting a not-null constraint in a ForeignKey field (adding null=True to a field). Instead of just dropping the not-null constraint which is a safe database operation, it first dropped the ForeignKey constraint, then dropped the not-null constraint, and last re-created the ForeignKey constraint (3 statements).

Re-creating ForeignKey constraints locks tables and can generate downtime. Since we cannot afford the downtime we used a RunSQL migration operation with state_operations as a workaround:

migrations.RunSQL(
           """--
           -- Alter field trip on payment
           --
           ALTER TABLE "app_payment" ALTER COLUMN "trip_id" DROP NOT NULL;
           """,
           state_operations=[
               migrations.AlterField(
                   model_name='payment',
                   name='trip',
                   field=models.ForeignKey(blank=True, null=True,
                                           on_delete=django.db.models.deletion.PROTECT,
                                           related_name='payments', to='app.Trip'),
               ),
           ])

This way we only apply the SQL we want and Django won't complain about the migration state not matching the current model. :)

comment:16 by JR Heard, 5 years ago

Owner: Shun Yu removed
Status: assignednew

The issue in this ticket is still happening for me on Django 2.1.9 with Postgres. Adding blank=True to a ManyToManyField drops the FK constraints on the join table and creates them back, exactly as described.

This ticket hasn't been touched in a couple of years so I'm going to deassign it. I hope this is the right thing to do here, I'm new to the Django community :)

Last edited 5 years ago by JR Heard (previous) (diff)

comment:17 by JR Heard, 5 years ago

Version: 1.82.1

comment:18 by Phil Krylov, 5 years ago

Cc: Phil Krylov added

comment:19 by Adam Johnson, 5 years ago

Cc: Adam Johnson added

comment:20 by Çağıl Uluşahin, 4 years ago

Needs tests: set
Owner: set to Çağıl Uluşahin
Status: newassigned

comment:21 by Çağıl Uluşahin, 4 years ago

Has patch: set
Needs tests: unset
Last edited 4 years ago by Mariusz Felisiak (previous) (diff)

comment:22 by Mariusz Felisiak, 4 years ago

Patch needs improvement: set

comment:23 by Mariusz Felisiak, 4 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:24 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

In 9159d17:

Fixed #25253 -- Made AlterField operation a noop when changing attributes that don't affect the schema.

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