Opened 4 years ago

Last modified 11 months ago

#34417 assigned Cleanup/optimization

AlterField migration on ForeignKey field re-creates foreign key constraints unnecessarily

Reported by: Ömer Faruk Abacı Owned by: Bhuvnesh
Component: Migrations Version: 4.1
Severity: Normal Keywords:
Cc: Adam Johnson, Dmytro Litvinov Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Let's assume that we have the following models:

class Bar(models.Model):
    pass

class Foo(models.Model):
    bar = models.ForeignKey(Bar, related_name="foos", on_delete=models.CASCADE)

The migration that creates these models will add an index to the bar field of Foo model, this is well-documented and there is no problem up until now. But when we do the following change to drop the index on the ForeignKey field:

class Foo(models.Model):
    bar = models.ForeignKey(..., db_index=False)

Then we have a migration that results in the following SQL statement:

BEGIN;
--
-- Alter field bar on foo
--
SET CONSTRAINTS "foos_foo_bar_id_efb062ad_fk_foos_bar_id" IMMEDIATE;
ALTER TABLE "foos_foo"
    DROP CONSTRAINT "foos_foo_bar_id_efb062ad_fk_foos_bar_id";

DROP INDEX IF EXISTS "foos_foo_bar_id_efb062ad";

ALTER TABLE "foos_foo" ADD CONSTRAINT "foos_foo_bar_id_efb062ad_fk_foos_bar_id"
    FOREIGN KEY ("bar_id")
    REFERENCES "bars_bar" ("id")
    DEFERRABLE INITIALLY DEFERRED;
COMMIT;

I believe that we shouldn't be needing to re-create the FK constraint, so the following SQL should suffice:

BEGIN;
--
-- Alter field bar on foo
--
DROP INDEX IF EXISTS "foos_foo_bar_id_efb062ad";
COMMIT;

Actually the main problem here is that dropping & adding constraints locks the entire table, and it might be catastrophic for some live production databases. IMHO this should either be documented or refactored. I look forward to hear your opinions on this, thank you in advance!

Change History (17)

comment:1 by Simon Charette, 4 years ago

Triage Stage: Unreviewed → Accepted

Dropping the foreign key and recreating it ​is effectively not necessary on PostgreSQL but ​it is on MySQL as this backend requires an index on the referencing side of the relation and will silently default to using an existing one if its present (see #31335 for more details).

Given the contention issues associated with dropping a foreign key on PostgreSQL I think it's a worthy investment to try to make things better on databases that are not affected by this quirk.

Would you like ​to work on a patch? The ​alter_field method is where you want to start looking and you'll likely need a feature flag to denote that foreign keys must be dropped or not based on the backend.

comment:2 by Ömer Faruk Abacı, 4 years ago

Hello Simon, thank you for your review! If that's OK for you one of my colleagues encountered this issue will work on a patch for this on Monday.

comment:3 by Simon Charette, 4 years ago

Totally okay Ömer, thanks to you and your coworker for taking the time to submit a patch.

comment:4 by Deniz Altun, 4 years ago

Hello Simon, just letting you know that I will be working on this patch.

comment:5 by Simon Charette, 4 years ago

Sounds good, you'll want to assign the ticket to you then.

comment:6 by Deniz Altun, 4 years ago

Owner: changed from nobody to Deniz Altun
Status: new → assigned

comment:7 by Mariusz Felisiak, 3 years ago

Has patch: set
Patch needs improvement: set

comment:8 by Bhuvnesh, 2 years ago

Owner: changed from Deniz Altun to Bhuvnesh
Patch needs improvement: unset

comment:9 by Sarah Boyce, 2 years ago

Patch needs improvement: set

comment:10 by Adam Johnson, 12 months ago

Cc: Adam Johnson added

comment:11 by Dmytro Litvinov, 12 months ago

Cc: Dmytro Litvinov added

comment:12 by Jonathan Biemond, 11 months ago

Owner: changed from Bhuvnesh to Jonathan Biemond

comment:13 by Bhuvnesh, 11 months ago

Hi Jonathan,
I'm still working on it! 🙂

comment:14 by Jacob Walls, 11 months ago

Owner: changed from Jonathan Biemond to Bhuvnesh

comment:15 by Jonathan Biemond, 11 months ago

Hi Bhuvnesh, my bad! I didn't look closely enough at the PR activity. Thanks for letting me know

comment:16 by Jacob Walls, 11 months ago

Patch needs improvement: unset

comment:17 by Jacob Walls, 11 months ago

Patch needs improvement: set
Note: See TracTickets for help on using tickets.
Back to Top