[Django] #23610: Removing a null constraint can lead to race conditions with migrations
Django
-------------------------------+------------------------
Reporter: jarshwah | Owner: nobody
Type: Uncategorized | Status: new
Component: Migrations | Version: 1.7
Severity: Normal | Keywords: migrations
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------
I think a race condition exists when removing a null constraint that
doesn't contain a default. This discussion started with regards to #23609.
The flow here was changing a column from having a null to having a
default:
`IntegerField(null=True) -> IntegerField(default=42)`
And the race condition:
- Deploy migration
- Update App Server 1 code
- App Server 2 tries to write to the table, and gets a write error (since
it doesn't yet have the default in its models file)
- Update App Server 2 code
For a write heavy table, this could result in many failed writes.
I think we should be clear, and document, that removing a null on a column
should always be done in two steps:
1. Add a default to the column. Deploy migration. Deploy app code.
2. Remove the null and the default from the column. Deploy migration.
Deploy app code.
Perhaps there could be a check that ensures that removing a null always
requires an existing default to exist.
--
Ticket URL: <https://code.djangoproject.com/ticket/23610>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jarshwah | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.7
Severity: Normal | Resolution:
Keywords: migrations | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by loic):
* needs_better_patch: => 0
* component: Migrations => Documentation
* needs_tests: => 0
* needs_docs: => 0
* type: Uncategorized => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
There are many cases where old app code won't work after a migration
applied (e.g. adding a new column with `NOT NULL`, removing a column,
etc.). I reckon we should just document that people are expected to run
the new app code right after applying a migration.
There are certainly ways around it, but it's important people understand
that nothing special is done at the framework level.
--
Ticket URL: <https://code.djangoproject.com/ticket/23610#comment:1>
Django
Reporter: Josh Smeaton | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.7
Severity: Normal | Resolution:
Keywords: migrations | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/14740 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/23610#comment:2>
Django
Reporter: Josh Smeaton | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: Documentation | Version: 1.7
Severity: Normal | Resolution:
Keywords: migrations | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* owner: nobody => Jacob Walls
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/23610#comment:3>
Django
Reporter: Josh Smeaton | Owner: Jacob
Type: | Walls
Component: Documentation | Version: 1.7
Severity: Normal | Resolution: wontfix
Keywords: migrations | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* status: assigned => closed
* resolution: => wontfix
Comment:
A short note is not enough to explain it without creating confusion. On
the other hand I don't think we want to create an extra section for this.
--
Ticket URL: <https://code.djangoproject.com/ticket/23610#comment:4>
Django
Reporter: Josh Smeaton | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: closed
Component: Documentation | Version: 1.7
Severity: Normal | Resolution: wontfix
Keywords: migrations | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
FWIW I've been working on a third-party application that resolves this
problem by introducing a concept of pre- and post-deployment migrations.
We've been using it in production for a few months now.
It has a built-in notion of quorum which allows the following sequence of
operation to be taken across multiple instances (e.g. multiple k8s
clusters)
1. Run the `migrate --pre-deploy` command to alter the column to have it
`NULL SET DEFAULT 42`
2. Wait for the N clusters to have completed their application rollout
3. Run the `migrate` command to have it `NOT NULL` and `DROP DEFAULT`
I created [https://github.com/charettes/django-syzygy/issues/24 an issue]
to add support for this particular case as null constraint removal is not
explicitly tested but it should only be matter of adjusting the auto-
detector to have a `IntegerField(null=True) -> IntegerField(default=42)`
change generate an extra `PreAlterField` operation meant to run on
`migrate --pre-deploy`.
--
Ticket URL: <https://code.djangoproject.com/ticket/23610#comment:5>