tbicr / django-pg-zero-downtime-migrations

Django postgresql backend that apply migrations with respect to database locks
MIT License
525 stars 25 forks source link

Add column and set default may be a safe action #62

Closed kevan26 closed 5 months ago

kevan26 commented 6 months ago

Under the "Django Migration Hacks" section the README on Row 8, it considers ALTER TABLE ADD COLUMN SET DEFAULT to be unsafe.

However, since Postgres 11, it could be considered safe (so long as the default value is not volatile) as suggested here: https://www.postgresql.org/docs/current/ddl-alter.html (under the "Tip" section).

Can this be investigated and updated, if needed? Alternatively, if it is still considered unsafe, could clarification be added?

Thank you in advance!

tbicr commented 6 months ago

Add column with default looks next https://github.com/tbicr/django-pg-zero-downtime-migrations/blob/master/tests/unit/test_schema.py#L241-L242:

ALTER TABLE "tests_model" ADD COLUMN "field" varchar(40) DEFAULT 'test' NULL;
ALTER TABLE "tests_model" ALTER COLUMN "field" DROP DEFAULT;

Lets consider two cases:

  1. add column with default and null in this case inserts between migration and deployment will be reason of null values that can be not expected

  2. add column with default and not null in this case all inserts between migration and deployment will be reason of fails that can be not accepted

so this is reason as in this lib adding column with default considered unsafe

in django 5 you can use https://docs.djangoproject.com/en/5.0/ref/models/fields/#db-default that is better way to use default, I'd strongly recommend

default explanation mentioned there https://github.com/tbicr/django-pg-zero-downtime-migrations/tree/master?tab=readme-ov-file#create-column-with-default

however option keeping default after migration and considered this operation as safe is interesting option

kevan26 commented 6 months ago

Thank you for your quick and thorough response - I understand it better now. I have 2 follow-up questions:

  1. I am wondering if the operation could still be considered safe, from a downtime prespective? Yes - there is still the possibility of NULL values in the code (in the case of between migration and deployment) but would it still be unsafe with respect to downtime? Or, is somehow having NULL values considered unsafe and could cause downtime?
  2. The reason in row 8 for why it is unsafe:

    because you spend time in migration to populate all values in table

Is this still true? I was thinking that since Postgres 11, it had been addressed:

adding a column with a constant default value no longer means that each row of the table needs to be updated when the ALTER TABLE statement is executed.

So since each row doesn't need to be updated when ALTER TABLE is executed, this would mean that the migration that generates this SQL would not need time to populate the values in the table, during the migration:

Instead, the default value will be returned the next time the row is accessed, and applied when the table is rewritten

Thank you in advance for your time and response!

tbicr commented 5 months ago

I use next definition for safe migration: no select, insert, update, delete operation will be affected (fail or timeout) during migration and deployment.

In case of ADD COLUMN DEFAULT NULL:

NULL means column values can be nullable that semantically correct, so ADD COLUMN DEFAULT NULL should be considered as safe migration.

In case of ADD COLUMN DEFAULT NOT NULL:

So ADD COLUMN DEFAULT NOT NULL is unsafe migration.

Looks previously this two cases were considered as same and was handled same way, so they should be separated.

You correct that in postgres 11+ no table rewrite happens and only schema changes is safe operation, this library documentation should be fixed.

kevan26 commented 5 months ago

Ok - great. The explanation provided makes sense to me, thank you.

I can leave this issue open, and the update can be created against it to track? Or if a new issue is preferred, feel free to close this out.

Thank you again for your response and explanations.

tbicr commented 5 months ago

defaults was reworked in 0.16