[Django] #28273: Have a way to prevent adding columns with defaults in migrations
Django
-------------------------------------+-------------------------------------
Reporter: Raphael | Owner: nobody
Gaschignard |
Type: | Status: new
Uncategorized |
Component: Database | Version: 1.10
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
So normally the Django documentation says that it doesn't use database
defaults.
If you have a model where you add a {{{ NullBooleanField}}} with a model-
side default and generate a migration then you end up with the following
SQL being executed (at least for Postgres)
{{{
BEGIN;
--
-- Add field some_field to table
--
ALTER TABLE "appname_model" ADD COLUMN "some_field" boolean DEFAULT true
NULL;
ALTER TABLE "appname_model" ALTER COLUMN "some_field" DROP DEFAULT;
COMMIT;
}}}
This actually backfills the value for you in the DB during the ADD COLUMN
command
But from the PostgreSQL documentation:
> When a column is added with ADD COLUMN, all existing rows in the table
are initialized with the column's default value (NULL if no DEFAULT clause
is specified).
>Adding a column with a non-null default or changing the type of an
existing column will require the entire table and indexes to be rewritten.
So if you're adding a column to a big enough table, the migration run
itself will be very painful, because it will update _every_ row of your
table to set the default.
Usually you want to set the {{{default}}} value model-side to help with
the future backfill. So the way to handle this is to usually remove the
automatically inserted default from the migration. But it can be easy to
miss this (or to have custom fields that auto-set defaults).
But the generated SQL is something you basically never want to run on a
big table. I would love to have a way to just prevent a migration from
ever generating an ADD COLUMN with a DEFAULT clause.
I think there would be a way to handle this through simple changes in the
schema editor but I couldn't figure out how the default code was
generated.
--
Ticket URL: <https://code.djangoproject.com/ticket/28273>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Uncategorized | Status: new
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
Can't you accomplish this by adding the column as `null=True`, populating
the values, then changing the column to `null=False` (similar to the
[https://docs.djangoproject.com/en/dev/howto/writing-migrations
/#migrations-that-add-unique-fields Migrations that add unique fields]
howto)? I believe defaults should only be added when adding non-nullable
columns -- if that behavior were changed, how would the non-null
constraint be avoided?
--
Ticket URL: <https://code.djangoproject.com/ticket/28273#comment:1>
Django
-------------------------------------+-------------------------------------
Type: Uncategorized | Status: new
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Raphael Gaschignard):
Replying to [comment:1 Tim Graham]:
> Can't you accomplish this by adding the column as `null=True`,
populating the values, then changing the column to `null=False` (similar
to the [https://docs.djangoproject.com/en/dev/howto/writing-migrations
/#migrations-that-add-unique-fields Migrations that add unique fields]
howto)? I believe defaults should only be added when adding non-nullable
columns -- if that behavior were changed, how would the non-null
constraint be avoided?
Because we want to deploy without downtime, we already deploy fields with
{{{null=True}}}, and add a backfill later. The issue I'm trying to point
out here is that with `default=a_value` set (even with `null=True`) we
still hit an issue because Postgres backfills defaults into existing rows
when a column is added (even if the column is nullable). I am not aware of
how other DB backends handle adding a column with `DEFAULT` + `NULL` set.
Our conclusion is that any `AddField` with a `default` set cannot be
accomplished without downtime in a production system (similar to a non-
null field), so we've written the following check for our migrations:
{{{
from django.db.migrations.operations.fields import AddField
from django.db.models.fields import NOT_PROVIDED
from django.db.models.signals import pre_migrate
def check_migration_safety(**kwargs):
# `plan` is not part of the public API and is subject to change in
later
# versions of Django.
plan = kwargs['plan']
for migration_cls, rolled_back in plan:
# Check if this is going forwards or rolling back a migration.
if not rolled_back:
for operation in migration_cls.operations:
if isinstance(operation, AddField):
description = "{migration!r} model '{model}' field
'{field}': ".format(
migration=migration_cls,
model=operation.model_name,
field=operation.name,
)
if not operation.field.null:
raise ValueError(description + "do not add new
non-nullable columns")
if operation.field.default != NOT_PROVIDED:
raise ValueError(description + "do not set a
default when adding columns")
pre_migrate.connect(
check_migration_safety,
dispatch_uid="operations.check_migration_safety",
)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28273#comment:2>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Migrations | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* type: Uncategorized => Cleanup/optimization
* component: Database layer (models, ORM) => Migrations
Comment:
If a model field `default` is added after adding a nullable field, does it
have the same issue? For example:
migration one:
adding `foo = models.NullBooleanField()`
ALTER TABLE "t28273_test" ADD COLUMN "foo" boolean NULL;
migration two:
changing to `foo = models.NullBooleanField(default=True)`
ALTER TABLE "t28273_test" ALTER COLUMN "foo" SET DEFAULT true; # does
this cause the backfill problem?
ALTER TABLE "t28273_test" ALTER COLUMN "foo" DROP DEFAULT;
I'm trying to think of how to solve this by structuring migrations
appropriately because I'm not seeing another elegant way forward.
--
Ticket URL: <https://code.djangoproject.com/ticket/28273#comment:3>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Migrations | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Raphael Gaschignard):
Adding a default after the column was added does not seem to trigger a
backfill. Like you're saying, creating two migrations doesn't cause this
issue to appear (except when custom fields add a default, for example)
{{{
testing=# select * from t;
a | b
----+---
hi | 3
hi | 4
hi | 5
(3 rows)
testing=# alter table t add column c integer null;
ALTER TABLE
testing=# select * from t;
a | b | c
----+---+---
hi | 3 |
hi | 4 |
hi | 5 |
(3 rows)
testing=# alter table t alter COLUMN c set default 3;
ALTER TABLE
testing=# select * from t;
a | b | c
----+---+---
hi | 3 |
hi | 4 |
hi | 5 |
(3 rows)
testing=# alter table t add column d integer null default 10;
ALTER TABLE
testing=# select * from t;
a | b | c | d
----+---+---+----
hi | 3 | | 10
hi | 4 | | 10
hi | 5 | | 10
(3 rows)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28273#comment:4>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Migrations | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
Would documenting this solution be sufficient?
--
Ticket URL: <https://code.djangoproject.com/ticket/28273#comment:5>
Django
--------------------------------------+------------------------------------
Reporter: Raphael Gaschignard | Owner: nobody
Component: Documentation | Version: 1.10
Severity: Normal | Resolution:
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 Tim Graham):
* stage: Unreviewed => Accepted
* component: Migrations => Documentation
--
Ticket URL: <https://code.djangoproject.com/ticket/28273#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: Caio
Type: | Ariede
Cleanup/optimization | Status: assigned
Component: Documentation | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Caio Ariede
* status: new => assigned
Comment:
Replying to [comment:5 Tim Graham]:
> Would documenting this solution be sufficient?
Where do you think the documentation for this could go?
--
Ticket URL: <https://code.djangoproject.com/ticket/28273#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: Caio
Type: | Ariede
Cleanup/optimization | Status: assigned
Component: Documentation | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
PR [https://github.com/django/django/pull/11871]
--
Ticket URL: <https://code.djangoproject.com/ticket/28273#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: Caio
Type: | Ariede
Component: Documentation | Version: 1.10
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"46a05e10a420deae3158c502ce58cf25ab59ccb1" 46a05e10]:
{{{
#!CommitTicketReference repository=""
revision="46a05e10a420deae3158c502ce58cf25ab59ccb1"
[2.2.x] Fixed #28273 -- Doc'd fast nullable column creation with defaults.
Backport of 06909fe084f87a65459a83bd69d7cdbe4fce9a7c from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28273#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: Caio
Type: | Ariede
Cleanup/optimization | Status: closed
Component: Documentation | Version: 1.10
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"06909fe084f87a65459a83bd69d7cdbe4fce9a7c" 06909fe0]:
{{{
#!CommitTicketReference repository=""
revision="06909fe084f87a65459a83bd69d7cdbe4fce9a7c"
Fixed #28273 -- Doc'd fast nullable column creation with defaults.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28273#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: Caio
Type: | Ariede
Cleanup/optimization | Status: closed
Component: Documentation | Version: 1.10
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"282138a7bbfc19681327042b9d462e038d88dda6" 282138a]:
{{{
#!CommitTicketReference repository=""
revision="282138a7bbfc19681327042b9d462e038d88dda6"
[3.0.x] Fixed #28273 -- Doc'd fast nullable column creation with defaults.
Backport of 06909fe084f87a65459a83bd69d7cdbe4fce9a7c from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28273#comment:10>