Opened 8 years ago

Last modified 5 hours ago

#29721 new Bug

Migrations are not applied atomically

Reported by: Gavin Wahl Owned by: Sanyam Khurana
Component: Migrations Version: 2.1
Severity: Normal Keywords:
Cc: 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

Migrations run the migration and record that the migration was applied in a different transaction. This results in a broken/inconsistent database when an error or crash happens between those two steps. This is not purely theoretical, it has happened to me.

The executor needs to call migration.apply and record_applied in the same transaction, and then the django_migrations table can't get out of sync with the migrations that have actually been applied. I think this could be done by moving the call to record_applied inside the schema_editor context manager block.

Change History (12)

comment:1 by Simon Charette, 8 years ago

Triage Stage: Unreviewed → Accepted

I guess we could move the recording logic in the context manager to be entirely correct but I'm surprised this has happened to you given the small amount of operations performed between the two blocks.

Are you certain you didn't hit ​the documented caveat of MySQL regarding atomic migrations? In that case this change wouldn't help at all. Was it the django_migrations table creation that failed?

Are you able to provide a patch/PR?

Last edited 8 years ago by Simon Charette (previous) (diff)

comment:2 by Gavin Wahl, 8 years ago

It was on postgres. It was a regular migration adding some tables, and the tables were added but the migration was not recorded in django_migrations.

comment:3 by Simon Charette, 8 years ago

This is really strange, are you sure the migration was not marked as atomic=False?

If not It's unlikely to have happened without an exception being surfaced.

comment:4 by Tim Graham, 8 years ago

Type: Uncategorized → Bug

comment:5 by Sanyam Khurana, 8 years ago

Owner: changed from nobody to Sanyam Khurana
Status: new → assigned

I'm working on this during DjangoCon US sprints and would submit a patch ASAP.

comment:6 by Simon Charette, 8 years ago

Has patch: set
Triage Stage: Accepted → Ready for checkin

​PR. Given the difficulty of adding a test that isn't highly implementation specific and how the new implementation is technically more correct I consider this patch RFC.

Last edited 8 years ago by Tim Graham (previous) (diff)

comment:7 by Simon Charette, 8 years ago

Needs tests: set
Triage Stage: Ready for checkin → Accepted

Removing the RFC status as a test for backends supporting transactional DDL can probably be added.

comment:8 by Simon Charette, 8 years ago

Needs tests: unset
Triage Stage: Accepted → Ready for checkin

comment:9 by Tim Graham <timograham@…>, 8 years ago

Resolution: → fixed
Status: assigned → closed

In c86a3d80:

Fixed #29721 -- Ensured migrations are applied and recorded atomically.

comment:10 by Mariusz Felisiak <felisiak.mariusz@…>, 6 years ago

In 533a583:

Refs #29721 -- Simplified migration used to test atomic recording.

This makes sure atomic recording of migration application is used when
the schema editor doesn't defer any statement.

comment:11 by Mariusz Felisiak <felisiak.mariusz@…>, 6 years ago

In 900b2ce9:

[3.2.x] Refs #29721 -- Simplified migration used to test atomic recording.

This makes sure atomic recording of migration application is used when
the schema editor doesn't defer any statement.

Backport of 533a5835784b95335c8373b6d0b9495b3834e96e from master

comment:12 by Gavin Wahl(vendor), 6 hours ago

Resolution: fixed
Status: closed → new

After #32374, the bug still happens for migrations that use deferred sql (which is a lot, like CreateModel, AlterField...). Maybe BaseDatabaseSchemaEditor needs to take a record_migration callback so it can always be recorded in the atomic block? Alternatively, it expose a execute_deferred_sql() method and allow the executor to execute and reset the deferred queue before committing. The deferred_sql check was kind of deeply coupled.

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.

I've reproduced the issue on postgres with atomic=True migration that uses deferred_sql, and tested this fix
Before: table created but migration not recorded. Migration "half applied" in inconsistent state.
After: The whole transaction rolled back, tables not created and migration not recorded, consistent state

Deferred sql correctly runs before record_migration, as in the fix for #32374, but now also in the transaction.

The issue is clear from the [abbreviated] postgres logs. 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"

After patch:

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

The django_migrations insert happens after the deferred sql and before the COMMIT, so it is atomic.

  • django/db/backends/base/schema.py

    diff --git a/django/db/backends/base/schema.py b/django/db/backends/base/schema.py
    index 353515315c..3930665e86 100644
    a b class BaseDatabaseSchemaEditor:  
    151151    sql_alter_table_comment = "COMMENT ON TABLE %(table)s IS %(comment)s"
    152152    sql_alter_column_comment = "COMMENT ON COLUMN %(table)s.%(column)s IS %(comment)s"
    153153
    154     def __init__(self, connection, collect_sql=False, atomic=True):
     154    def __init__(self, connection, collect_sql=False, atomic=True, pre_commit=None):
    155155        self.connection = connection
    156156        self.collect_sql = collect_sql
    157157        if self.collect_sql:
    … … class BaseDatabaseSchemaEditor:  
    160160            # name in the database, so introspection must target the old name.
    161161            self.collected_table_renames = {}
    162162        self.atomic_migration = self.connection.features.can_rollback_ddl and atomic
     163        self.pre_commit = pre_commit
    163164
    164165    # State-managing methods
    165166
    … … class BaseDatabaseSchemaEditor:  
    174175        if exc_type is None:
    175176            for sql in self.deferred_sql:
    176177                self.execute(sql, None)
     178        if self.pre_commit is not None:
     179            self.pre_commit()
    177180        if self.atomic_migration:
    178181            self.atomic.__exit__(exc_type, exc_value, traceback)
    179182
  • django/db/migrations/executor.py

    diff --git a/django/db/migrations/executor.py b/django/db/migrations/executor.py
    index 074d7b2d28..d5652079a9 100644
    a b class MigrationExecutor:  
    240240
    241241    def apply_migration(self, state, migration, fake=False, fake_initial=False):
    242242        """Run a migration forwards."""
    243         migration_recorded = False
    244243        if self.progress_callback:
    245244            self.progress_callback("apply_start", migration, fake)
    246245        if not fake:
    … … class MigrationExecutor:  
    252251            if not fake:
    253252                # Alright, do it normally
    254253                with self.connection.schema_editor(
    255                     atomic=migration.atomic
     254                    atomic=migration.atomic,
     255                    pre_commit=lambda: self.record_migration(migration.app_label, migration.name)
    256256                ) as schema_editor:
    257257                    state = migration.apply(state, schema_editor)
    258                     if not schema_editor.deferred_sql:
    259                         self.record_migration(migration.app_label, migration.name)
    260                         migration_recorded = True
    261         if not migration_recorded:
    262             self.record_migration(migration.app_label, migration.name)
    263258        # Report progress
    264259        if self.progress_callback:
    265260            self.progress_callback("apply_success", migration, fake)

(This is just a minimal proof of concept of what a fix would look like, it'd have to be applied to unapply_migrations as well, handle exceptions in record_migrations, etc).

Minimal model to get deferred_sql:

from django.db import models

class Foo(models.Model):
    a = models.ForeignKey('self', on_delete=models.CASCADE)
Last edited 5 hours ago by Gavin Wahl(vendor) (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top