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:2 by , 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 , 3 weeks ago
| Type: | Uncategorized → Bug |
|---|
comment:4 by , 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 , 3 weeks ago
| Cc: | 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
- We knew
deferred_sqlwas designed to defer statements until the end of a transaction - 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 , 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.
comment:7 by , 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 , 12 days ago
| Owner: | set to |
|---|---|
| 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
I'm considering two ways to fix this:
apply_migrationwould passon_exit=lambda: self.record_migration(migration.app_label, migration.name)to ensure it happens after deferred sql.flush_deferred_sqlmethod 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_migrationwhen callflush_deferred_sql()thenrecord_migration()inside theschema_editorwithblock. 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_migrationinserts adjango_migrationsrow 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.