Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#23609 closed Uncategorized (fixed)

Migration fails when removing a NULL constraint

Reported by: Markus Holtermann Owned by: Markus Holtermann
Component: Database layer (models, ORM) Version: 1.7
Severity: Normal Keywords:
Cc: Markus Holtermann, Simon Charette Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Markus Holtermann)

If a model has e.g IntegerField(null=True) and a migration changes it to IntegerField(default=42) the migration fails on SQLite due to the way the new table is created (create new table and copy old data into it) and PostgreSQL due to no default being used for existing rows. The respective SQL code Django produces is:

-- SQLite
CREATE TABLE "testapp_foo__new" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "d" integer NOT NULL);
INSERT INTO "testapp_foo__new" ("id", "d") SELECT "id", "d" FROM "testapp_foo";
DROP TABLE "testapp_foo";
ALTER TABLE "testapp_foo__new" RENAME TO "testapp_foo";

-- PostgreSQL
ALTER TABLE "testapp_foo" ALTER COLUMN "d" SET DEFAULT 42, ALTER COLUMN "d" SET NOT NULL;
ALTER TABLE "testapp_foo" ALTER COLUMN "d" DROP DEFAULT;

The first idea for a patch was to do a two-step insert, first all rows w/o NULL values and second those w/ NULL and adding a the default explicitly. This would involve some more code changes. apollo13 came up with the idea to use a CASE WHEN SQL syntax which, as it turns out, seems quite nice. jarshwah came up with coalesce("d", 42).

-- SQLite
CREATE TABLE "testapp_foo__new" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "d" integer NOT NULL);
INSERT INTO "testapp_foo__new" ("id", "d") SELECT "id", coalesc("d", 42) FROM "testapp_foo";
DROP TABLE "testapp_foo";
ALTER TABLE "testapp_foo__new" RENAME TO "testapp_foo";

-- PostgreSQL
ALTER TABLE "testapp_foo" ALTER COLUMN "d" SET DEFAULT 42;
UPDATE "testapp_foo" SET "d" = 42 WHERE "d" IS NULL;
ALTER TABLE "testapp_foo" ALTER COLUMN "d" SET NOT NULL;
ALTER TABLE "testapp_foo" ALTER COLUMN "d" DROP DEFAULT;

Change History (7)

comment:1 Changed 5 years ago by Markus Holtermann

Has patch: set

comment:2 Changed 5 years ago by Simon Charette

Patch needs improvement: set

Great patch. Left some comments on the PR.

comment:3 Changed 5 years ago by Markus Holtermann

Description: modified (diff)
Status: newassigned
Summary: Migration fails on SQLite when removing a NULL constraintMigration fails when removing a NULL constraint

Updated description after discussion with jarshwah and charettes

comment:4 Changed 5 years ago by Simon Charette

Cc: Simon Charette added

It has been suggested to use the non standard USING clause on #postgresql to avoid bloat by rewriting the entire table:

ALTER TABLE "testapp_foo"
    ALTER COLUMN "d" SET DATA TYPE integer USING coalesce("d", 42),
    ALTER COLUMN "d" SET NOT NULL;

However this will lock the table longer than the UPDATE + SET NOT NULL.

Last edited 5 years ago by Simon Charette (previous) (diff)

comment:5 Changed 5 years ago by Markus Holtermann

Since there is no special handling for any database except SQLite in the _alter_field() schema alteration I didn't want to introduce one for PostgreSQL. Thus I ended up with the 4-way-default-alteration 1) Add a default for new incoming writes, 2) Update existing NULL rows with new default, 3) Replace NULL constraint with NOT NULL, 4) Drop the default again.

I also updated the pull requested with the recent changes.

comment:6 Changed 5 years ago by Loic Bistuer <loic.bistuer@…>

Resolution: fixed
Status: assignedclosed

In f633ba778d302c12f4295714a78fa20dff98b39f:

Fixed #23609 -- Fixed IntegrityError that prevented altering a NULL column into a NOT NULL one due to existing rows

Thanks to Simon Charette, Loic Bistuer and Tim Graham for the review.

comment:7 Changed 5 years ago by Loic Bistuer <loic.bistuer@…>

In 71988ed953131908fa2a1ba9fac294cedaa6e6c9:

[1.7.x] Fixed #23609 -- Fixed IntegrityError that prevented altering a NULL column into a NOT NULL one due to existing rows

Thanks to Simon Charette, Loic Bistuer and Tim Graham for the review.

Backport of f633ba778d from master

Note: See TracTickets for help on using tickets.
Back to Top