Migrations are not applied atomically

[Gavin Wahl]

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.

[Simon Charette]

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?

[Gavin Wahl]

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.

[Simon Charette]

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.

[Sanyam Khurana]

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

[Simon Charette]

​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.

[Simon Charette]

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

[Tim Graham <timograham@…>]

In c86a3d80:

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

[Mariusz Felisiak <felisiak.mariusz@…>]

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.

[Mariusz Felisiak <felisiak.mariusz@…>]

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

[Gavin Wahl(vendor)]

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? 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 with a deferred_sql migration 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. Migration correct "not applied", consistent state.
deferred sql correctly runs before record_migration, as in the fix for #32374, but now also in the transaction.

django/db/backends/base/schema.py

@@ -151 +151 @@
     sql_alter_table_comment = "COMMENT ON TABLE %(table)s IS %(comment)s"
     sql_alter_column_comment = "COMMENT ON COLUMN %(table)s.%(column)s IS %(comment)s"
 
-    def __init__(self, connection, collect_sql=False, atomic=True):
+    def __init__(self, connection, collect_sql=False, atomic=True, pre_commit=None):
         self.connection = connection
         self.collect_sql = collect_sql
         if self.collect_sql:
@@ -160 +160 @@
             # name in the database, so introspection must target the old name.
             self.collected_table_renames = {}
         self.atomic_migration = self.connection.features.can_rollback_ddl and atomic
+        self.pre_commit = pre_commit
 
     # State-managing methods
 
@@ -174 +175 @@
         if exc_type is None:
             for sql in self.deferred_sql:
                 self.execute(sql, None)
+        if self.pre_commit is not None:
+            self.pre_commit()
         if self.atomic_migration:
             self.atomic.__exit__(exc_type, exc_value, traceback)
 

django/db/migrations/executor.py

@@ -240 +240 @@
 
     def apply_migration(self, state, migration, fake=False, fake_initial=False):
         """Run a migration forwards."""
-        migration_recorded = False
         if self.progress_callback:
             self.progress_callback("apply_start", migration, fake)
         if not fake:
@@ -252 +251 @@
             if not fake:
                 # Alright, do it normally
                 with self.connection.schema_editor(
-                    atomic=migration.atomic
+                    atomic=migration.atomic,
+                    pre_commit=lambda: self.record_migration(migration.app_label, migration.name)
                 ) as schema_editor:
                     state = migration.apply(state, schema_editor)
-                    if not schema_editor.deferred_sql:
-                        self.record_migration(migration.app_label, migration.name)
-                        migration_recorded = True
-        if not migration_recorded:
-            self.record_migration(migration.app_label, migration.name)
         # Report progress
         if self.progress_callback:
             self.progress_callback("apply_success", migration, fake)

Minimal model to get deferred_sql:

from django.db import models

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