[Django] #29721: Migrations are not applied atomically
Django
-----------------------------------------+------------------------
Reporter: Gavin Wahl | Owner: nobody
Type: Uncategorized | Status: new
Component: Migrations | Version: 2.1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29721>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Gavin Wahl | Owner: nobody
Type: Uncategorized | Status: new
Component: Migrations | Version: 2.1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* stage: Unreviewed => Accepted
Comment:
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
[https://docs.djangoproject.com/en/2.1/topics/migrations/#mysql 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/29721#comment:1>
Django
Reporter: Gavin Wahl | Owner: nobody
Type: Uncategorized | Status: new
Component: Migrations | Version: 2.1
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29721#comment:2>
Django
Reporter: Gavin Wahl | Owner: nobody
Type: Uncategorized | Status: new
Component: Migrations | Version: 2.1
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29721#comment:3>
Django
Reporter: Gavin Wahl | Owner: nobody
Component: Migrations | Version: 2.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* type: Uncategorized => Bug
--
Ticket URL: <https://code.djangoproject.com/ticket/29721#comment:4>
Django
Reporter: Gavin Wahl | Owner: Sanyam Khurana
Type: Bug | Status: assigned
Component: Migrations | Version: 2.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Sanyam Khurana):
* status: new => assigned
* owner: nobody => Sanyam Khurana
Comment:
I'm working on this during DjangoCon US sprints and would submit a patch
ASAP.
--
Ticket URL: <https://code.djangoproject.com/ticket/29721#comment:5>
Django
Reporter: Gavin Wahl | Owner: Sanyam Khurana
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* has_patch: 0 => 1
* stage: Accepted => Ready for checkin
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29721#comment:6>
Django
Reporter: Gavin Wahl | Owner: Sanyam Khurana
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* needs_tests: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
Removing the RFC status as a test for backends supporting transactional
DDL can probably be added.
--
Ticket URL: <https://code.djangoproject.com/ticket/29721#comment:7>
Django
Reporter: Gavin Wahl | Owner: Sanyam Khurana
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* needs_tests: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/29721#comment:8>
Django
Reporter: Gavin Wahl | Owner: Sanyam Khurana
Component: Migrations | Version: 2.1
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"c86a3d80a25acd1887319198ca21a84c451014ad" c86a3d80]:
{{{
#!CommitTicketReference repository=""
revision="c86a3d80a25acd1887319198ca21a84c451014ad"
Fixed #29721 -- Ensured migrations are applied and recorded atomically.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29721#comment:9>
Django
Reporter: Gavin Wahl | Owner: Sanyam Khurana
Type: Bug | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"533a5835784b95335c8373b6d0b9495b3834e96e" 533a583]:
{{{
#!CommitTicketReference repository=""
revision="533a5835784b95335c8373b6d0b9495b3834e96e"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29721#comment:10>
Django
Reporter: Gavin Wahl | Owner: Sanyam Khurana
Type: Bug | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"900b2ce92bd68fb4b17e923cffdbde30eb24f7d2" 900b2ce9]:
{{{
#!CommitTicketReference repository=""
revision="900b2ce92bd68fb4b17e923cffdbde30eb24f7d2"
[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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29721#comment:11>