From f18d393afd53b71854ee7a71f5bf9a5ba2abf7b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pave=C5=82=20Ty=C5=9Blacki?= Date: Wed, 29 May 2024 14:30:42 +0100 Subject: [PATCH] update defaults handling --- CHANGES.md | 8 + README.md | 130 +++++++---- .../backends/postgres/schema.py | 215 +++++++++++++----- tests/unit/test_schema.py | 155 ++++++++++++- 4 files changed, 394 insertions(+), 114 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 4b067ae..ad3c561 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,5 +1,13 @@ # django-pg-zero-downtime-migrations changelog +## 0.16 + - changed `ADD COLUMN DEFAULT NULL` to safe operation for code default + - changed `ADD COLUMN DEFAULT NOT NULL` to safe operation for `db_default` in django 5.0+ + - added `ZERO_DOWNTIME_MIGRATIONS_KEEP_DEFAULT` settings and changed `ADD COLUMN DEFAULT NOT NULL` with this settings to safe operation for django<5.0 + - updated unsafe migrations links to documentation + - updated patched code to latest django version + - improved README + ## 0.15 - added idempotent mode and `ZERO_DOWNTIME_MIGRATIONS_IDEMPOTENT_SQL` setting - fixed django 3.2 degradation with missing `skip_default_on_alter` method diff --git a/README.md b/README.md index b5ae700..e75385b 100644 --- a/README.md +++ b/README.md @@ -38,9 +38,9 @@ To enable zero downtime migrations for postgres just setup django backend provid ### Differences with standard django backend -This backend provides same result state, but different way and with additional guarantees for avoiding stuck table locks. +This backend provides same result state (except of `ZERO_DOWNTIME_MIGRATIONS_KEEP_DEFAULT=True` usage for django < 5.0), but different way and with additional guarantees for avoiding stuck table locks. -This backend doesn't use transactions for migrations (except `RunPython` operation), because not all SQL fixes can be run in transaction and it allows to avoid deadlocks for complex migration. So when your migration will down in middle of transaction you need fix it manually (instead potential downtime). For that reason good practice to make migration modules small as possible. +This backend doesn't use transactions for migrations (except `RunPython` operation), because not all SQL fixes can be run in transaction and it allows to avoid deadlocks for complex migration. So when your migration will down in the middle of migration file operations you need to fix db state manually (instead potential downtime). For that reason good practice to make migration modules small as possible. Also `ZERO_DOWNTIME_MIGRATIONS_IDEMPOTENT_SQL=True` allows to automate manual db state fixing. ### Deployment flow @@ -124,9 +124,21 @@ To avoid manual schema manipulation idempotent mode allows to rerun failed migra > _NOTE:_ idempotent mode checks rely only on name and index and constraint valid state, so it can ignore name collisions and recommended do not use it for CI checks. +#### ZERO_DOWNTIME_MIGRATIONS_KEEP_DEFAULT + +Define way keep or drop code defaults on database level when adding new column, default `False`: + + ZERO_DOWNTIME_MIGRATIONS_KEEP_DEFAULT = False + +Allowed values: +- `True` - after adding column with code default this default will not be dropped, this option allows to use `ALTER TABLE ADD COLUMN SET DEFAULT NOT NULL` as safe operation that much more simple and efficient than creating column without default on database level and populating column next +- `False` - after adding column with code default this default will be dropped, this is standard django behaviour + +> _NOTE:_ this option works only for django < 5.0, in django 5.0+ explicit [`db_default`](https://docs.djangoproject.com/en/dev/ref/models/fields/#db-default) should be used instead. + #### PgBouncer and timeouts -In case you using [PgBouncer](https://www.pgbouncer.org/) and expect timeouts will work as expected you need make sure that run migrations using [session pool_mode](https://www.pgbouncer.org/config.html#pool_mode). +In case you using [PgBouncer](https://www.pgbouncer.org/) and expect timeouts will work as expected you need make sure that run migrations using [session pool_mode](https://www.pgbouncer.org/config.html#pool_mode) or use direct database connection. ## How it works @@ -199,7 +211,7 @@ Found same diagram in interesting article http://pankrat.github.io/2015/django-m In this diagram we can extract several metrics: -1. operation time - time spent changing schema, in the case of long running operations on many rows tables like `CREATE INDEX` or `ALTER TABLE ADD COLUMN SET DEFAULT`, so you need a safe equivalent. +1. operation time - time spent changing schema, in the case of long running operations on many rows tables like `CREATE INDEX` or `ALTER TABLE ADD CONSTRAINT`, so you need a safe equivalent. 2. waiting time - your migration will wait until all transactions complete, so there is issue for long running operations/transactions like analytic, so you need avoid it or disable during migration. 3. queries per second + execution time and connections pool - if executing many queries, especially long running ones, they can consume all available database connections until the lock is released, so you need different optimizations there: run migrations when least busy, decrease query count and execution time, split data. 4. too many operations in one transaction - you have issues in all previous points for one operation so if you have many operations in one transaction then you have more likelihood to get this issue, so you need avoid too many simultaneous operations in a single transaction (or even not run it in a transaction at all but being careful when an operation fails). @@ -228,40 +240,40 @@ Regarding documentation https://www.postgresql.org/docs/current/static/mvcc-intr Any schema changes can be processed with creation of new table and copy data to it, but it can take significant time. -| # | name | safe | safe alternative | description | -| --: | --------------------------------------------- |:----:|:-----------------------------:|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| 1 | `CREATE SEQUENCE` | X | | safe operation, because your business logic shouldn't operate with new sequence on migration time \* | -| 2 | `DROP SEQUENCE` | X | | safe operation, because your business logic shouldn't operate with this sequence on migration time \* | -| 3 | `CREATE TABLE` | X | | safe operation, because your business logic shouldn't operate with new table on migration time \* | -| 4 | `DROP TABLE` | X | | safe operation, because your business logic shouldn't operate with this table on migration time \* | -| 5 | `ALTER TABLE RENAME TO` | | use updatable view | **unsafe operation**, because it's too hard write business logic that operate with two tables simultaneously, so propose to use temporary updatable view and switch names in transaction \* | -| 6 | `ALTER TABLE SET TABLESPACE` | | add new table and copy data | **unsafe operation**, but probably you don't need it at all or often \* | -| 7 | `ALTER TABLE ADD COLUMN` | X | | safe operation if without `SET NOT NULL`, `SET DEFAULT`, `PRIMARY KEY`, `UNIQUE` \* | -| 8 | `ALTER TABLE ADD COLUMN SET DEFAULT` | | add column and set default | **unsafe operation**, because you spend time in migration to populate all values in table, so propose `ALTER TABLE ADD COLUMN` and then populate column and then `SET DEFAULT` \* | -| 9 | `ALTER TABLE ADD COLUMN SET NOT NULL` | | +/- | **unsafe operation**, because doesn't work without `SET DEFAULT` or after migration old code can insert rows without new column and raise exception, so propose `ALTER TABLE ADD COLUMN` and then populate column and then `ALTER TABLE ALTER COLUMN SET NOT NULL` \* and \*\* | -| 10 | `ALTER TABLE ADD COLUMN PRIMARY KEY` | | add index and add constraint | **unsafe operation**, because you spend time in migration to `CREATE INDEX`, so propose `ALTER TABLE ADD COLUMN` and then `CREATE INDEX CONCURRENTLY` and then `ALTER TABLE ADD CONSTRAINT PRIMARY KEY USING INDEX` \*\*\* | -| 11 | `ALTER TABLE ADD COLUMN UNIQUE` | | add index and add constraint | **unsafe operation**, because you spend time in migration to `CREATE INDEX`, so propose `ALTER TABLE ADD COLUMN` and then `CREATE INDEX CONCURRENTLY` and then `ALTER TABLE ADD CONSTRAINT UNIQUE USING INDEX` \*\*\* | -| 12 | `ALTER TABLE ALTER COLUMN TYPE` | | +/- | **unsafe operation**, because you spend time in migration to check that all items in column valid or to change type, but some operations can be safe \*\*\*\* | -| 13 | `ALTER TABLE ALTER COLUMN SET NOT NULL` | | add check constraint before | **unsafe operation**, because you spend time in migration to check that all items in column `NOT NULL`, so propose `ALTER TABLE ADD CONSTRAINT CHECK` and then `ALTER TABLE VALIDATE CONSTRAINT` and then `ALTER TABLE ALTER COLUMN SET NOT NULL` *\* | -| 14 | `ALTER TABLE ALTER COLUMN DROP NOT NULL` | X | | safe operation | -| 15 | `ALTER TABLE ALTER COLUMN SET DEFAULT` | X | | safe operation | -| 16 | `ALTER TABLE ALTER COLUMN DROP DEFAULT` | X | | safe operation | -| 17 | `ALTER TABLE DROP COLUMN` | X | | safe operation, because your business logic shouldn't operate with this column on migration time, however better `ALTER TABLE ALTER COLUMN DROP NOT NULL`, `ALTER TABLE DROP CONSTRAINT` and `DROP INDEX` before \* and \*\*\*\*\* | -| 18 | `ALTER TABLE RENAME COLUMN` | | use updatable view | **unsafe operation**, because it's too hard write business logic that operate with two columns simultaneously, so propose to use temporary updatable view and switch names in transaction \* | -| 19 | `ALTER TABLE ADD CONSTRAINT CHECK` | | add as not valid and validate | **unsafe operation**, because you spend time in migration to check constraint | -| 20 | `ALTER TABLE DROP CONSTRAINT` (`CHECK`) | X | | safe operation | -| 21 | `ALTER TABLE ADD CONSTRAINT FOREIGN KEY` | | add as not valid and validate | **unsafe operation**, because you spend time in migration to check constraint, lock two tables | -| 22 | `ALTER TABLE DROP CONSTRAINT` (`FOREIGN KEY`) | X | | safe operation, lock two tables | -| 23 | `ALTER TABLE ADD CONSTRAINT PRIMARY KEY` | | add index and add constraint | **unsafe operation**, because you spend time in migration to create index \*\*\* | -| 24 | `ALTER TABLE DROP CONSTRAINT` (`PRIMARY KEY`) | X | | safe operation \*\*\* | -| 25 | `ALTER TABLE ADD CONSTRAINT UNIQUE` | | add index and add constraint | **unsafe operation**, because you spend time in migration to create index \*\*\* | -| 26 | `ALTER TABLE DROP CONSTRAINT` (`UNIQUE`) | X | | safe operation \*\*\* | -| 27 | `ALTER TABLE ADD CONSTRAINT EXCLUDE` | | add new table and copy data | -| 28 | `ALTER TABLE DROP CONSTRAINT (EXCLUDE)` | X | | -| 29 | `CREATE INDEX` | | `CREATE INDEX CONCURRENTLY` | **unsafe operation**, because you spend time in migration to create index | -| 30 | `DROP INDEX` | X | `DROP INDEX CONCURRENTLY` | safe operation \*\*\* | -| 31 | `CREATE INDEX CONCURRENTLY` | X | | safe operation | -| 32 | `DROP INDEX CONCURRENTLY` | X | | safe operation \*\*\* | +| # | name | safe | safe alternative | description | +| --: |-----------------------------------------------|:----:|:-----------------------------:|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| 1 | `CREATE SEQUENCE` | X | | safe operation, because your business logic shouldn't operate with new sequence on migration time \* | +| 2 | `DROP SEQUENCE` | X | | safe operation, because your business logic shouldn't operate with this sequence on migration time \* | +| 3 | `CREATE TABLE` | X | | safe operation, because your business logic shouldn't operate with new table on migration time \* | +| 4 | `DROP TABLE` | X | | safe operation, because your business logic shouldn't operate with this table on migration time \* | +| 5 | `ALTER TABLE RENAME TO` | | use updatable view | **unsafe operation**, because it's too hard write business logic that operate with two tables simultaneously, so propose to use temporary updatable view and switch names in transaction \* | +| 6 | `ALTER TABLE SET TABLESPACE` | | add new table and copy data | **unsafe operation**, but probably you don't need it at all or often \* | +| 7 | `ALTER TABLE ADD COLUMN` | X | | safe operation if without `SET NOT NULL`, `PRIMARY KEY`, `UNIQUE` \* | +| 8 | `ALTER TABLE ADD COLUMN SET DEFAULT` | X | | safe operation, however it can be unsafe if code default used within `NOT NULL`, for `db_default` or `NULL` there are no issue \* | +| 9 | `ALTER TABLE ADD COLUMN SET NOT NULL` | | +/- | **unsafe operation**, because doesn't work without `SET DEFAULT` or after migration old code can insert rows without new column and raise exception, so propose to use `ALTER TABLE ADD COLUMN SET DEFAULT` with `db_default` or `ALTER TABLE ADD COLUMN` and then populate column and then `ALTER TABLE ALTER COLUMN SET NOT NULL` \* and \*\* | +| 10 | `ALTER TABLE ADD COLUMN PRIMARY KEY` | | add index and add constraint | **unsafe operation**, because you spend time in migration to `CREATE INDEX`, so propose `ALTER TABLE ADD COLUMN` and then `CREATE INDEX CONCURRENTLY` and then `ALTER TABLE ADD CONSTRAINT PRIMARY KEY USING INDEX` \*\*\* | +| 11 | `ALTER TABLE ADD COLUMN UNIQUE` | | add index and add constraint | **unsafe operation**, because you spend time in migration to `CREATE INDEX`, so propose `ALTER TABLE ADD COLUMN` and then `CREATE INDEX CONCURRENTLY` and then `ALTER TABLE ADD CONSTRAINT UNIQUE USING INDEX` \*\*\* | +| 12 | `ALTER TABLE ALTER COLUMN TYPE` | | +/- | **unsafe operation**, because you spend time in migration to check that all items in column valid or to change type, but some operations can be safe \*\*\*\* | +| 13 | `ALTER TABLE ALTER COLUMN SET NOT NULL` | | add check constraint before | **unsafe operation**, because you spend time in migration to check that all items in column `NOT NULL`, so propose `ALTER TABLE ADD CONSTRAINT CHECK` and then `ALTER TABLE VALIDATE CONSTRAINT` and then `ALTER TABLE ALTER COLUMN SET NOT NULL` *\* | +| 14 | `ALTER TABLE ALTER COLUMN DROP NOT NULL` | X | | safe operation | +| 15 | `ALTER TABLE ALTER COLUMN SET DEFAULT` | X | | safe operation | +| 16 | `ALTER TABLE ALTER COLUMN DROP DEFAULT` | X | | safe operation | +| 17 | `ALTER TABLE DROP COLUMN` | X | | safe operation, because your business logic shouldn't operate with this column on migration time, however better `ALTER TABLE ALTER COLUMN DROP NOT NULL`, `ALTER TABLE DROP CONSTRAINT` and `DROP INDEX` before \* and \*\*\*\*\* | +| 18 | `ALTER TABLE RENAME COLUMN` | | use updatable view | **unsafe operation**, because it's too hard write business logic that operate with two columns simultaneously, so propose to use temporary updatable view and switch names in transaction \* | +| 19 | `ALTER TABLE ADD CONSTRAINT CHECK` | | add as not valid and validate | **unsafe operation**, because you spend time in migration to check constraint | +| 20 | `ALTER TABLE DROP CONSTRAINT` (`CHECK`) | X | | safe operation | +| 21 | `ALTER TABLE ADD CONSTRAINT FOREIGN KEY` | | add as not valid and validate | **unsafe operation**, because you spend time in migration to check constraint, lock two tables | +| 22 | `ALTER TABLE DROP CONSTRAINT` (`FOREIGN KEY`) | X | | safe operation, lock two tables | +| 23 | `ALTER TABLE ADD CONSTRAINT PRIMARY KEY` | | add index and add constraint | **unsafe operation**, because you spend time in migration to create index \*\*\* | +| 24 | `ALTER TABLE DROP CONSTRAINT` (`PRIMARY KEY`) | X | | safe operation \*\*\* | +| 25 | `ALTER TABLE ADD CONSTRAINT UNIQUE` | | add index and add constraint | **unsafe operation**, because you spend time in migration to create index \*\*\* | +| 26 | `ALTER TABLE DROP CONSTRAINT` (`UNIQUE`) | X | | safe operation \*\*\* | +| 27 | `ALTER TABLE ADD CONSTRAINT EXCLUDE` | | add new table and copy data | | +| 28 | `ALTER TABLE DROP CONSTRAINT (EXCLUDE)` | X | | | +| 29 | `CREATE INDEX` | | `CREATE INDEX CONCURRENTLY` | **unsafe operation**, because you spend time in migration to create index | +| 30 | `DROP INDEX` | X | `DROP INDEX CONCURRENTLY` | safe operation \*\*\* | +| 31 | `CREATE INDEX CONCURRENTLY` | X | | safe operation | +| 32 | `DROP INDEX CONCURRENTLY` | X | | safe operation \*\*\* | \*: main point with migration on production without downtime that your old and new code should correctly work before and after migration, lets look this point closely in [Dealing with logic that should work before and after migration](#dealing-with-logic-that-should-work-before-and-after-migration) section. @@ -312,19 +324,45 @@ For this migration too hard implement logic that will work correctly for all ins 1. create new table, copy exist data, drop old table 2. downtime -##### Create column with default +##### Create column not null + +Migrations: `ALTER TABLE ADD COLUMN NOT NULL`. + +Postgres doesn't allow to create column with `NOT NULL` if table not empty and `DEFAULT` is not provided. So you want to `ALTER TABLE ADD COLUMN DEFAULT NOT NULL`. +Django has two ways to create column default: [code `default`](https://docs.djangoproject.com/en/dev/ref/models/fields/#default) and [`db_default` for django 5.0+](https://docs.djangoproject.com/en/dev/ref/models/fields/#db-default). +Main difference between them for us in operations they do for migration and old code inserts handling after migration: + +Code `default` migration and business logic SQL: +```sql +-- migration +ALTER TABLE tbl ADD COLUMN new_col integer DEFAULT 0 NOT NULL; +ALTER TABLE tbl ALTER COLUMN new_col DROP DEFAULT; + +-- business logic +INSERT INTO tbl (old_col) VALUES (1); -- old code inserts fail +INSERT INTO tbl (old_col, new_col) VALUES (1, 1); -- new code inserts work fine +``` -Migrations: `ALTER TABLE ADD COLUMN SET DEFAULT`. +`db_default` migration and business logic SQL: +```sql +-- migration +ALTER TABLE tbl ADD COLUMN new_col integer DEFAULT 0 NOT NULL; -Standard django's behaviour for creation column with default is populate all values with default. Django don't use database defaults permanently, so when you add new column with default django will create column with default and drop this default at once, eg. new default will come from django code. In this case you can have a gap when migration applied by not all instances has updated and at this moment new rows in table will be without default and probably you need update nullable values after that. So to avoid this case best way is avoid creation column with default and split column creation (with default for new rows) and data population to two migrations (with deployments). +-- business logic +INSERT INTO tbl (old_col) VALUES (1); -- old code inserts work fine with default +INSERT INTO tbl (old_col, new_col) VALUES (1, 1); -- new code inserts work fine +``` -#### Dealing with `NOT NULL` constraint +`db_default` is most robust way to apply default and it's works fine with `NOT NULL` constraints too. +In django<5.0 you can use `ZERO_DOWNTIME_MIGRATIONS_KEEP_DEFAULT=True` to emulate `db_default` behaviour for `default` field. -Postgres check that all column items `NOT NULL` when you applying `NOT NULL` constraint, for postgres 12 and newest it doesn't make this check if appropriate `CHECK CONSTRAINT` exists, but for older versions you can't defer this check as for `NOT VALID`. Fortunately we have some hacks and alternatives there for old postgres versions. +#### Dealing with `NOT NULL` column constraint -1. Run migrations when load minimal to avoid negative affect of locking. -2. `SET statement_timeout` and try to set `NOT NULL` constraint for small tables. -3. Use `CHECK (column IS NOT NULL)` constraint instead that support `NOT VALID` option with next `VALIDATE CONSTRAINT`, see article for details https://medium.com/doctolib-engineering/adding-a-not-null-constraint-on-pg-faster-with-minimal-locking-38b2c00c4d1c. There are additionally can be applied `NOT NULL` constraint via direct `pg_catalog.pg_attribute` `attnotnull` update, but it require superuser permissions. +Postgres checks that all column values `NOT NULL` (full table scan) when you are applying `ALTER TABLE ALTER COLUMN SET NOT NULL`, this check skipped if appropriate valid `CHECK CONSTRAINT` exists for postgres 12+. So to make existing column `NOT NULL` safe way you can follow next steps: +- `ALTER TABLE ADD CONSTRAINT CHECK (column IS NOT NULL) NOT VALID` - create invalid check constraint for column, this operation takes `ACCESS EXCLUSIVE` lock only for table metadata update +- `ALTER TABLE VALIDATE CONSTRAINT` - validate constraint, at this moment all column values should be `NOT NULL`, this operation takes `SHARE UPDATE EXCLUSIVE` lock until full table scan will be completed +- `ALTER TABLE ALTER COLUMN SET NOT NULL` - set column `NOT NULL` don't check column values if appropriate valid `CHECK CONSTRAINT` exists, in this case this operation takes `ACCESS EXCLUSIVE` lock only for table metadata update +- `ALTER TABLE DROP CONSTRAINT` - clean up `CHECK CONSTRAINT` that duplicates column `NOT NULL`, this operation takes `ACCESS EXCLUSIVE` lock only for table metadata update #### Dealing with `UNIQUE` constraint diff --git a/django_zero_downtime_migrations/backends/postgres/schema.py b/django_zero_downtime_migrations/backends/postgres/schema.py index ce2461b..c4a408f 100644 --- a/django_zero_downtime_migrations/backends/postgres/schema.py +++ b/django_zero_downtime_migrations/backends/postgres/schema.py @@ -10,18 +10,14 @@ DatabaseSchemaEditor as PostgresDatabaseSchemaEditor ) from django.db.backends.utils import strip_quotes +from django.db.models import NOT_PROVIDED class Unsafe: - ADD_COLUMN_DEFAULT = ( - "ADD COLUMN DEFAULT is unsafe operation\n" - "See details for safe alternative " - "https://github.com/tbicr/django-pg-zero-downtime-migrations#create-column-with-default" - ) ADD_COLUMN_NOT_NULL = ( "ADD COLUMN NOT NULL is unsafe operation\n" "See details for safe alternative " - "https://github.com/tbicr/django-pg-zero-downtime-migrations#dealing-with-not-null-constraint" + "https://github.com/tbicr/django-pg-zero-downtime-migrations#create-column-not-null" ) ALTER_COLUMN_TYPE = ( "ALTER COLUMN TYPE is unsafe operation\n" @@ -36,7 +32,7 @@ class Unsafe: ALTER_TABLE_RENAME = ( "ALTER TABLE RENAME is unsafe operation\n" "See details for save alternative " - "https://github.com/tbicr/django-pg-zero-downtime-migrations#changes-for-working-logic" + "https://github.com/tbicr/django-pg-zero-downtime-migrations#rename-models" ) ALTER_TABLE_SET_TABLESPACE = ( "ALTER TABLE SET TABLESPACE is unsafe operation\n" @@ -46,7 +42,7 @@ class Unsafe: ALTER_TABLE_RENAME_COLUMN = ( "ALTER TABLE RENAME COLUMN is unsafe operation\n" "See details for save alternative " - "https://github.com/tbicr/django-pg-zero-downtime-migrations#changes-for-working-logic" + "https://github.com/tbicr/django-pg-zero-downtime-migrations#rename-columns" ) @@ -472,6 +468,18 @@ class DatabaseSchemaEditorMixin: _varchar_type_regexp = re.compile(r'^varchar\((?P\d+)\)$') _numeric_type_regexp = re.compile(r'^numeric\((?P\d+), *(?P\d+)\)$') + @property + def sql_alter_column_no_default_null(self): + if self.KEEP_DEFAULT: + return DUMMY_SQL + return super().sql_alter_column_no_default_null + + @property + def sql_alter_column_no_default(self): + if self.KEEP_DEFAULT: + return DUMMY_SQL + return super().sql_alter_column_no_default + def __init__(self, connection, collect_sql=False, atomic=True): # Disable atomic transactions as it can be reason of downtime or deadlock # in case if you combine many operation in one migration module. @@ -488,6 +496,14 @@ def __init__(self, connection, collect_sql=False, atomic=True): self.RAISE_FOR_UNSAFE = getattr(settings, "ZERO_DOWNTIME_MIGRATIONS_RAISE_FOR_UNSAFE", False) self.DEFERRED_SQL = getattr(settings, "ZERO_DOWNTIME_MIGRATIONS_DEFERRED_SQL", True) self.IDEMPOTENT_SQL = getattr(settings, "ZERO_DOWNTIME_MIGRATIONS_IDEMPOTENT_SQL", False) + self.KEEP_DEFAULT = getattr(settings, "ZERO_DOWNTIME_MIGRATIONS_KEEP_DEFAULT", False) + if django.VERSION[:2] >= (5, 0) and hasattr(settings, 'ZERO_DOWNTIME_MIGRATIONS_KEEP_DEFAULT'): + warnings.warn( + 'settings.ZERO_DOWNTIME_MIGRATIONS_KEEP_DEFAULT is not applicable for django 5.0+. ' + 'Please remove this setting.', + DeprecationWarning, + ) + self.KEEP_DEFAULT = False def execute(self, sql, params=()): if sql is DUMMY_SQL: @@ -657,19 +673,20 @@ def _rename_field_sql(self, table, old_field, new_field, new_type): warnings.warn(UnsafeOperationWarning(Unsafe.ALTER_TABLE_RENAME_COLUMN)) return super()._rename_field_sql(table, old_field, new_field, new_type) - def _add_column_default(self): - if self.RAISE_FOR_UNSAFE: - raise UnsafeOperationException(Unsafe.ADD_COLUMN_DEFAULT) - else: - warnings.warn(UnsafeOperationWarning(Unsafe.ADD_COLUMN_DEFAULT)) - return " DEFAULT %s" + def _has_db_default(self, field): + if django.VERSION < (5, 0): + if self.KEEP_DEFAULT: + return field.default is not NOT_PROVIDED + return False + return field.db_default is not NOT_PROVIDED def _add_column_not_null(self, model, field): - if self.RAISE_FOR_UNSAFE: - raise UnsafeOperationException(Unsafe.ADD_COLUMN_NOT_NULL) - else: - warnings.warn(UnsafeOperationWarning(Unsafe.ADD_COLUMN_NOT_NULL)) - return " NOT NULL" + if not self._has_db_default(field): + if self.RAISE_FOR_UNSAFE: + raise UnsafeOperationException(Unsafe.ADD_COLUMN_NOT_NULL) + else: + warnings.warn(UnsafeOperationWarning(Unsafe.ADD_COLUMN_NOT_NULL)) + return "NOT NULL" def _add_column_primary_key(self, model, field): self.deferred_sql.append(self.sql_create_pk % { @@ -694,31 +711,56 @@ def skip_default_on_alter(self, field): """ return False - def column_sql(self, model, field, include_default=False): - """ - Take a field and return its column definition. - The field must already have had set_attributes_from_name() called. - """ - if not include_default: - return super().column_sql(model, field, include_default) - - # Get the column's type and use that as the basis of the SQL - db_params = field.db_parameters(connection=self.connection) - sql = db_params['type'] - params = [] - # Check for fields that aren't actually columns (e.g. M2M) - if sql is None: - return None, None - # Collation. - collation = getattr(field, 'db_collation', None) - if collation: - sql += self._collate_sql(collation) - # Work out nullability + def column_sql(self, model, field, include_default=False): + """ + Return the column definition for a field. The field must already have + had set_attributes_from_name() called. + """ + if not include_default: + return super().column_sql(model, field, include_default) + + # Get the column's type and use that as the basis of the SQL. + field_db_params = field.db_parameters(connection=self.connection) + column_db_type = field_db_params["type"] + # Check for fields that aren't actually columns (e.g. M2M). + if column_db_type is None: + return None, None + params = [] + return ( + " ".join( + # This appends to the params being returned. + self._iter_column_sql( + column_db_type, + params, + model, + field, + include_default, + ) + ), + params, + ) + + def _patched_iter_column_sql( + self, column_db_type, params, model, field, field_db_params, include_default + ): + yield column_db_type + if field_db_params.get("collation"): + yield self._collate_sql(field_db_params.get("collation")) + if django.VERSION >= (4, 2) and self.connection.features.supports_comments_inline and field.db_comment: + yield self._comment_sql(field.db_comment) + # Work out nullability. null = field.null - # If we were told to include a default value, do so + # Add database default. + if django.VERSION >= (5, 0) and field.db_default is not NOT_PROVIDED: + default_sql, default_params = self.db_default_sql(field) + yield f"DEFAULT {default_sql}" + params.extend(default_params) + include_default = False + # Include a default value, if requested. include_default = ( - include_default and - not self.skip_default(field) and + include_default + and not self.skip_default(field) + and # Don't include a default value if it's a nullable field and the # default cannot be dropped in the ALTER COLUMN statement (e.g. # MySQL longtext and longblob). @@ -727,28 +769,87 @@ def column_sql(self, model, field, include_default=False): if include_default: default_value = self.effective_default(field) if default_value is not None: - sql += self._add_column_default() - params += [default_value] + column_default = "DEFAULT " + self._column_default_sql(field) + if self.connection.features.requires_literal_defaults: + # Some databases can't take defaults as a parameter + # (Oracle, SQLite). If this is the case, the individual + # schema backend should implement prepare_default(). + yield column_default % self.prepare_default(default_value) + else: + yield column_default + params.append(default_value) # Oracle treats the empty string ('') as null, so coerce the null # option whenever '' is a possible value. - if (field.empty_strings_allowed and not field.primary_key and - self.connection.features.interprets_empty_strings_as_nulls): + if ( + field.empty_strings_allowed + and not field.primary_key + and self.connection.features.interprets_empty_strings_as_nulls + ): null = True - if null and not self.connection.features.implied_column_null: - sql += " NULL" + if django.VERSION >= (5, 0) and field.generated: + generated_sql, generated_params = self._column_generated_sql(field) + params.extend(generated_params) + yield generated_sql elif not null: - sql += self._add_column_not_null(model, field) - # Primary key/unique outputs + yield self._add_column_not_null(model, field) # different to origin method + elif not self.connection.features.implied_column_null: + yield "NULL" if field.primary_key: - sql += self._add_column_primary_key(model, field) + self._add_column_primary_key(model, field) # different to origin method elif field.unique: - sql += self._add_column_unique(model, field) - # Optionally add the tablespace if it's an implicitly indexed column + self._add_column_unique(model, field) # different to origin method + # Optionally add the tablespace if it's an implicitly indexed column. tablespace = field.db_tablespace or model._meta.db_tablespace - if tablespace and self.connection.features.supports_tablespaces and field.unique: - sql += " %s" % self.connection.ops.tablespace_sql(tablespace, inline=True) - # Return the sql - return sql, params + if ( + tablespace + and self.connection.features.supports_tablespaces + and field.unique + ): + yield self.connection.ops.tablespace_sql(tablespace, inline=True) + + if django.VERSION >= (4, 1): + def _iter_column_sql( + self, column_db_type, params, model, field, field_db_params, include_default + ): + if not include_default: + yield from super()._iter_column_sql( + column_db_type, + params, + model, + field, + field_db_params, + include_default, + ) + else: + yield from self._patched_iter_column_sql( + column_db_type, + params, + model, + field, + field_db_params, + include_default, + ) + else: + def _iter_column_sql( + self, column_db_type, params, model, field, include_default + ): + if not include_default: + yield from super()._iter_column_sql( + column_db_type, + params, + model, + field, + include_default, + ) + else: + yield from self._patched_iter_column_sql( + column_db_type, + params, + model, + field, + {}, + include_default, + ) def _alter_column_set_not_null(self, model, new_field): self.deferred_sql.append(self._sql_column_not_null % { diff --git a/tests/unit/test_schema.py b/tests/unit/test_schema.py index 7c69524..448c28e 100644 --- a/tests/unit/test_schema.py +++ b/tests/unit/test_schema.py @@ -212,37 +212,134 @@ def test_add_field__ok(): @pytest.mark.django_db -def test_add_field_with_default__warning(): +@override_settings(ZERO_DOWNTIME_MIGRATIONS_RAISE_FOR_UNSAFE=True) +def test_add_field_with_code_default_null__ok(): with cmp_schema_editor() as editor: - with pytest.warns(UnsafeOperationWarning, match='ADD COLUMN DEFAULT is unsafe operation'): - field = models.CharField(max_length=40, default='test', null=True) + field = models.CharField(max_length=40, default='test', null=True) + field.set_attributes_from_name('field') + editor.add_field(Model, field) + assert editor.collected_sql == timeouts( + 'ALTER TABLE "tests_model" ADD COLUMN "field" varchar(40) DEFAULT \'test\' NULL;', + ) + timeouts( + 'ALTER TABLE "tests_model" ALTER COLUMN "field" DROP DEFAULT;', + ) + assert editor.django_sql == [ + 'ALTER TABLE "tests_model" ADD COLUMN "field" varchar(40) DEFAULT \'test\' NULL;', + 'ALTER TABLE "tests_model" ALTER COLUMN "field" DROP DEFAULT;', + ] + + +@pytest.mark.django_db +def test_add_field_with_code_default_not_null__warning(): + with cmp_schema_editor() as editor: + with pytest.warns(UnsafeOperationWarning, match='ADD COLUMN NOT NULL is unsafe operation'): + field = models.CharField(max_length=40, default='test', null=False) field.set_attributes_from_name('field') editor.add_field(Model, field) assert editor.collected_sql == timeouts( - 'ALTER TABLE "tests_model" ADD COLUMN "field" varchar(40) DEFAULT \'test\' NULL;' + 'ALTER TABLE "tests_model" ADD COLUMN "field" varchar(40) DEFAULT \'test\' NOT NULL;' ) + timeouts( 'ALTER TABLE "tests_model" ALTER COLUMN "field" DROP DEFAULT;' ) assert editor.django_sql == [ - 'ALTER TABLE "tests_model" ADD COLUMN "field" varchar(40) DEFAULT \'test\' NULL;', + 'ALTER TABLE "tests_model" ADD COLUMN "field" varchar(40) DEFAULT \'test\' NOT NULL;', 'ALTER TABLE "tests_model" ALTER COLUMN "field" DROP DEFAULT;', ] @pytest.mark.django_db @override_settings(ZERO_DOWNTIME_MIGRATIONS_RAISE_FOR_UNSAFE=True) -def test_add_field_with_default__raise(): +def test_add_field_with_code_default_not_null__raise(): with cmp_schema_editor() as editor: - with pytest.raises(UnsafeOperationException, match='ADD COLUMN DEFAULT is unsafe operation'): - field = models.CharField(max_length=40, default='test', null=True) + with pytest.raises(UnsafeOperationException, match='ADD COLUMN NOT NULL is unsafe operation'): + field = models.CharField(max_length=40, default='test', null=False) field.set_attributes_from_name('field') editor.add_field(Model, field) assert editor.django_sql == [ - 'ALTER TABLE "tests_model" ADD COLUMN "field" varchar(40) DEFAULT \'test\' NULL;', + 'ALTER TABLE "tests_model" ADD COLUMN "field" varchar(40) DEFAULT \'test\' NOT NULL;', + 'ALTER TABLE "tests_model" ALTER COLUMN "field" DROP DEFAULT;', + ] + + +@pytest.mark.django_db +@pytest.mark.skipif(django.VERSION[:2] >= (5, 0), reason='setting deprecated for django 5.0') +@override_settings(ZERO_DOWNTIME_MIGRATIONS_RAISE_FOR_UNSAFE=True, + ZERO_DOWNTIME_MIGRATIONS_KEEP_DEFAULT=True) +def test_add_field_with_code_default_not_null__keep_default__ok(): + with cmp_schema_editor() as editor: + field = models.CharField(max_length=40, default='test', null=False) + field.set_attributes_from_name('field') + editor.add_field(Model, field) + assert editor.collected_sql == timeouts( + 'ALTER TABLE "tests_model" ADD COLUMN "field" varchar(40) DEFAULT \'test\' NOT NULL;' + ) + assert editor.django_sql == [ + 'ALTER TABLE "tests_model" ADD COLUMN "field" varchar(40) DEFAULT \'test\' NOT NULL;', 'ALTER TABLE "tests_model" ALTER COLUMN "field" DROP DEFAULT;', ] +@pytest.mark.django_db +@pytest.mark.skipif(django.VERSION[:2] < (5, 0), reason='setting deprecated in django 5.0') +@override_settings(ZERO_DOWNTIME_MIGRATIONS_RAISE_FOR_UNSAFE=True, + ZERO_DOWNTIME_MIGRATIONS_KEEP_DEFAULT=True) +def test_add_field_with_code_default_not_null__keep_default__raise(): + with cmp_schema_editor() as editor: + with pytest.raises(UnsafeOperationException, match='ADD COLUMN NOT NULL is unsafe operation'): + field = models.CharField(max_length=40, default='test', null=False) + field.set_attributes_from_name('field') + editor.add_field(Model, field) + assert editor.django_sql == [ + 'ALTER TABLE "tests_model" ADD COLUMN "field" varchar(40) DEFAULT \'test\' NOT NULL;', + 'ALTER TABLE "tests_model" ALTER COLUMN "field" DROP DEFAULT;', + ] + + +@pytest.mark.django_db +@pytest.mark.skipif(django.VERSION[:2] < (5, 0), reason='functionality provided in django 5.0') +@override_settings(ZERO_DOWNTIME_MIGRATIONS_RAISE_FOR_UNSAFE=True) +def test_add_field_with_db_default_null__ok(): + with cmp_schema_editor() as editor: + field = models.CharField(max_length=40, db_default='test', null=True) + field.set_attributes_from_name('field') + field.model = Model + editor.add_field(Model, field) + assert editor.collected_sql == timeouts(editor.django_sql) + assert editor.django_sql == [ + 'ALTER TABLE "tests_model" ADD COLUMN "field" varchar(40) DEFAULT \'test\' NULL;', + ] + + +@pytest.mark.django_db +@pytest.mark.skipif(django.VERSION[:2] < (5, 0), reason='functionality provided in django 5.0') +@override_settings(ZERO_DOWNTIME_MIGRATIONS_RAISE_FOR_UNSAFE=True) +def test_add_field_with_db_default_not_null__ok(): + with cmp_schema_editor() as editor: + field = models.CharField(max_length=40, db_default='test', null=False) + field.set_attributes_from_name('field') + field.model = Model + editor.add_field(Model, field) + assert editor.collected_sql == timeouts(editor.django_sql) + assert editor.django_sql == [ + 'ALTER TABLE "tests_model" ADD COLUMN "field" varchar(40) DEFAULT \'test\' NOT NULL;', + ] + + +@pytest.mark.django_db +@pytest.mark.skipif(django.VERSION[:2] < (5, 0), reason='functionality provided in django 5.0') +@override_settings(ZERO_DOWNTIME_MIGRATIONS_RAISE_FOR_UNSAFE=True) +def test_add_field_with_code_default_db_default_not_null__ok(): + with cmp_schema_editor() as editor: + field = models.CharField(max_length=40, db_default='test', default='test2', null=False) + field.set_attributes_from_name('field') + field.model = Model + editor.add_field(Model, field) + assert editor.collected_sql == timeouts(editor.django_sql) + assert editor.django_sql == [ + 'ALTER TABLE "tests_model" ADD COLUMN "field" varchar(40) DEFAULT \'test\' NOT NULL;', + ] + + @pytest.mark.django_db def test_add_field_with_not_null__warning(): with cmp_schema_editor() as editor: @@ -962,7 +1059,7 @@ def test_alter_filed_drop_not_null__ok(cursor, mocker): @pytest.mark.django_db @override_settings(ZERO_DOWNTIME_MIGRATIONS_RAISE_FOR_UNSAFE=True) -def test_alter_field_set_default__ok(): +def test_alter_field_set_code_default__ok(): with cmp_schema_editor() as editor: old_field = models.CharField(max_length=40) old_field.set_attributes_from_name('field') @@ -976,7 +1073,7 @@ def test_alter_field_set_default__ok(): @pytest.mark.django_db @override_settings(ZERO_DOWNTIME_MIGRATIONS_RAISE_FOR_UNSAFE=True) -def test_alter_field_drop_default__ok(): +def test_alter_field_drop_code_default__ok(): with cmp_schema_editor() as editor: old_field = models.CharField(max_length=40, default='test') old_field.set_attributes_from_name('field') @@ -988,6 +1085,42 @@ def test_alter_field_drop_default__ok(): assert editor.django_sql == [] +@pytest.mark.django_db +@pytest.mark.skipif(django.VERSION[:2] < (5, 0), reason='functionality provided in django 5.0') +@override_settings(ZERO_DOWNTIME_MIGRATIONS_RAISE_FOR_UNSAFE=True) +def test_alter_field_set_db_default__ok(): + with cmp_schema_editor() as editor: + old_field = models.CharField(max_length=40) + old_field.set_attributes_from_name('field') + old_field.model = Model + new_field = models.CharField(max_length=40, db_default='test') + new_field.set_attributes_from_name('field') + new_field.model = Model + editor.alter_field(Model, old_field, new_field) + assert editor.collected_sql == timeouts(editor.django_sql) + assert editor.django_sql == [ + 'ALTER TABLE "tests_model" ALTER COLUMN "field" SET DEFAULT \'test\';', + ] + + +@pytest.mark.django_db +@pytest.mark.skipif(django.VERSION[:2] < (5, 0), reason='functionality provided in django 5.0') +@override_settings(ZERO_DOWNTIME_MIGRATIONS_RAISE_FOR_UNSAFE=True) +def test_alter_field_drop_db_default__ok(): + with cmp_schema_editor() as editor: + old_field = models.CharField(max_length=40, db_default='test') + old_field.set_attributes_from_name('field') + old_field.model = Model + new_field = models.CharField(max_length=40) + new_field.set_attributes_from_name('field') + new_field.model = Model + editor.alter_field(Model, old_field, new_field) + assert editor.collected_sql == timeouts(editor.django_sql) + assert editor.django_sql == [ + 'ALTER TABLE "tests_model" ALTER COLUMN "field" DROP DEFAULT;', + ] + + @pytest.mark.django_db def test_rename_field__warning(): with cmp_schema_editor() as editor: