#29721 closed Bug (fixed)
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 (11)
comment:1 by , 6 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 6 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 , 6 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 , 6 years ago
Type: | Uncategorized → Bug |
---|
comment:5 by , 6 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
I'm working on this during DjangoCon US sprints and would submit a patch ASAP.
comment:6 by , 6 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.
comment:7 by , 6 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 , 6 years ago
Needs tests: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
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?