Opened 3 weeks ago

Last modified 12 days ago

#37416 assigned Bug

Migrations are not recorded atomically

Reported by: Gavin Wahl Owned by: Gavin Wahl
Component: Migrations Version: 6.1
Severity: Normal Keywords:
Cc: Simon Charette Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Migrations apply their operations transactionally, and then record that the migration was applied outside of the transaction. If any sort of failure or crash happens between this steps, this results in the migration being run again, despite it have already actually been applied, just not recorded.

There is a narrow window of failure in MigrationExecutor.apply_migration between the call to migration.apply() and self.record_migration(). It doesn't happen often, but there are mulitple ways it could -- the python process crashing, the computer running python crashing, postgres crashing, or a network failure. I believe the most realistic and reliable reproducible reproducer is to simulate a hard crash during migration recording by adding import os; os.kill(os.getpid(), 9) or import sys; sys.exit() to the first line of BaseDatabaseSchemaEditor.record_migration. A unrecoverable hard crash can happen at any time, that's why we have transactions.

This was fixed #29721 but then regressed in #32374. record_migration is now atomic for migrations that don't use deferred sql, but non-atomic for those that do (which is a lot, like CreateModel, AlterField...). The correct order is

  • Migrate
  • Apply deferred sql
  • Record
  • Commit

While 0c42cdf0d moved the Commit before the Record.

It's easy to see what's happening from these [abbreviated] postgres logs from running migrations. The django_migrations insert happens after the COMMIT of the main migration, so the migration isn't really atomic.

Before:

BEGIN
CREATE TABLE "foo_foo" ("id" bigint NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "a_id" bigint NOT NULL)
ALTER TABLE "foo_foo" ADD CONSTRAINT "foo_foo_a_id_f8ce5994_fk_foo_foo_id" FOREIGN KEY ("a_id") REFERENCES "foo_foo" ("id") DEFERRABLE INITIALLY DEFERRED
CREATE INDEX "foo_foo_a_id_f8ce5994" ON "foo_foo" ("a_id")
COMMIT
INSERT INTO "django_migrations" ("app", "name", "applied") VALUES ('foo', '0001_initial', '2026-10-07 07:10:08.085619+00:00'::timestamptz) RETURNING "django_migrations"."id"

Correct behavior:

BEGIN
CREATE TABLE "foo_foo" ("id" bigint NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "a_id" bigint NOT NULL)
ALTER TABLE "foo_foo" ADD CONSTRAINT "foo_foo_a_id_f8ce5994_fk_foo_foo_id" FOREIGN KEY ("a_id") REFERENCES "foo_foo" ("id") DEFERRABLE INITIALLY DEFERRED
CREATE INDEX "foo_foo_a_id_f8ce5994" ON "foo_foo" ("a_id")
INSERT INTO "django_migrations" ("app", "name", "applied") VALUES ('foo', '0001_initial', '2026-10-07 07:13:01.602656+00:00'::timestamptz) RETURNING "django_migrations"."id"
COMMIT

Here's a minimal model to get a deferred sql migration for these logs:
Minimal model to get deferred_sql:

from django.db import models

class Foo(models.Model):
    a = models.ForeignKey('self', on_delete=models.CASCADE)

Change History (8)

comment:1 by Gavin Wahl, 3 weeks ago

I'm considering two ways to fix this:

  • Adding a callback argument to schema_editor that it would call right before exiting its atomic block. apply_migration would pass on_exit=lambda: self.record_migration(migration.app_label, migration.name) to ensure it happens after deferred sql.
  • Adding a flush_deferred_sql method to the schema editor that executes and clears the pending deferred sql. It would be called inside the schema editor wherever that happens today to preserve backwards compatibility. apply_migration when call flush_deferred_sql() then record_migration() inside the schema_editor with block. The schema editor would still call it again, but the pending deferred sql would have been cleared and it would do nothing.

Does anyone have an opinion which would be preferable?

There's also currently a performance impact of recording migrations outside a transaction. I have a squashed migration that replaces=[...] several hundred others. record_migration inserts a django_migrations row for each of those replacements. It's taking about 0.6s to record all of them applied, because the each get their own transaction. In a single transaction, it takes about 0.01s. I'm working on making my tests run faster, and since they migration from scratch every time that's actually a significant part of time to migrate--slower than even the combine DDL time of all the migrations.

Last edited 3 weeks ago by Gavin Wahl (previous) (diff)

comment:2 by lindangulopez, 3 weeks ago

Hi Gavin,

Thank you for the detailed report. I am a Djangonaut in the current Djangonaut Space session 7 and I am learning the migrations and schema editor code, so I want to watch this ticket closely.

I used Claude (Opus 5.5) to read the code with me. In my local clone (commit 569910a42e, 3 Sept 2026), MigrationExecutor.apply_migration calls record_migration() inside the schema_editor block only when schema_editor.deferred_sql is empty, and after the block otherwise, as you describe. I have not run the reproduction yet and would like to do it myself.

Two questions:

(i) Are you planning to submit the patch? I would like to follow how it is done, so I have added myself to Cc.
(ii) Would an independent reproduction help the triage? I can try to run your minimal model on PostgreSQL and on SQLite and post the statement logs here.

If you open a pull request, I would be glad to review it and test it locally.

Kind regards,
Linda

comment:3 by lindangulopez, 3 weeks ago

Type: Uncategorized → Bug

comment:4 by Abheet, 3 weeks ago

Hi, I'm interested in working on ticket #37416, concerning atomic migration recording when deferred SQL is present. I've been investigating the transaction boundaries and preparing a potential fix with regression tests. The ticket is currently unreviewed, and I'd appreciate guidance on whether it is ready to be accepted for contribution. Thank you!

comment:5 by Simon Charette, 3 weeks ago

Cc: Simon Charette added
Component: Database layer (models, ORM) → Migrations

Abheet please avoid eagerly submitting LLM generated patch ripping away reporter work while there is clearly still an open discussion about how to proceed, it might be well intended but it's not helping in the grand scheme of things.


Hey Gavin I looked back at #32374 and from what I could recall we went this way for two reasons at the time

  1. We knew deferred_sql was designed to defer statements until the end of a transaction
  2. We were on a thigh deadline for a release and making sure we never record if deferred SQL failed was the priority

We historically never ran deferred SQL in the same transaction as the rest of the DDL since the introduction of the migration framework so I'm a bit hesitant to change it now. The fact we create unique constraints and indices which can quite long to perform on large tables worries me the most.

If we are considering not deferring some statements until the end of a transaction anymore I wonder if we should instead get rid of it entirely and steer users towards the usage of AddIndexConcurrently and friends instead.

TL;DR if we are considering making deferred_sql not deferred anymore should we instead get rid of it entirely? I wouldn't be sad to get rid of all the deferred statement eliding and renaming logic if we can provide it's safe to do.

comment:6 by Gavin Wahl, 3 weeks ago

Sorry about that regression, sounds stressful.

Unless I'm misunderstanding something deferred SQL has always run in the transaction (and must, if migrations are to be atomic, even this issue notwithstanding)

master: ​https://github.com/django/django/blob/f1d11117c6a3e227ce7cdd263cf0743f0eb73070/django/db/backends/base/schema.py#L176
4.2.x (picked arbitrarily, I'm not sure of the history): ​https://github.com/django/django/blob/stable/4.0.x/django/db/backends/base/schema.py#L157

My understanding of deferred SQL is that its purpose is to resolve circular references, (like two models with FKs to each other), not anything to do with transactions.

I wouldn't want to recommend using CONCURRENTLY routinely because it can "fail but leave behind an “invalid” index. This index will be ignored for querying purposes because it might be incomplete; however it will still consume update overhead."

The actual thing that's frustrating me here is that schema_editor opens its own atomic block, instead of having the caller wrap it if desired. I think this is required for some sqlite weirdness, but it'd be a lot more elegant if they were decoupled.

Last edited 3 weeks ago by Gavin Wahl (previous) (diff)

comment:7 by Simon Charette, 13 days ago

Unless I'm misunderstanding something deferred SQL has always run in the transaction (and must, if migrations are to be atomic, even this issue notwithstanding)

You are right, I mixed up why we made this change in the first place. I interpreted 0c42cdf0d2422f4c080e93594d5d15381d6e955e as deferred SQL is run out the transaction but it was actually made because of backends that don't support transaction DDL (e.g. MySQL) as for them we'd record the migration as applied before the deferred SQL was applied on __exit__ opening the door for a migration marked as applied while it could potentially fail.

The actual thing that's frustrating me here is that schema_editor opens its own atomic block, instead of having the caller wrap it if desired. I think this is required for some sqlite weirdness, but it'd be a lot more elegant if they were decoupled.

I agree, and the fact it conditionnaly do so, based on whether the backend supports transactions DDL, is what threw me off the wrong conclusion and potentially the poor fix I landed in 0c42cdf0d2422f4c080e93594d5d15381d6e955e. It's not something we should address here though as many tests would need to be adjusted and a deprecation period followed but definitely an avenue we should explore.

Please ignore the concerns I raised in comment:5 and feel free to submit a PR.

comment:8 by Sarah Boyce, 12 days ago

Owner: set to Gavin Wahl
Status: new → assigned
Triage Stage: Unreviewed → Accepted

Gavin I have assigned to you as the reporter has the right of first refusal to take on the ticket, please unassign yourself if you are not intending to work on the ticket

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