From 89303492b2ed3558b2fbc4dbfb2fc1f492e47c4c Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Wed, 20 Nov 2024 17:58:06 -0800 Subject: [PATCH 01/12] Update docs on how to delete tables and columns This uses the new operations from https://github.com/getsentry/sentry/pull/81063 https://github.com/getsentry/sentry/pull/81098 --- .../database-migrations/index.mdx | 220 +++++++++++------- 1 file changed, 139 insertions(+), 81 deletions(-) diff --git a/develop-docs/api-server/application-domains/database-migrations/index.mdx b/develop-docs/api-server/application-domains/database-migrations/index.mdx index 4fb8d707513cd1..50f4c1dfb251c7 100644 --- a/develop-docs/api-server/application-domains/database-migrations/index.mdx +++ b/develop-docs/api-server/application-domains/database-migrations/index.mdx @@ -145,87 +145,137 @@ This is complicated due to our deploy process. When we deploy, we run migrations To avoid this, follow these steps: -- Mark the column as nullable if it isn't, and create a migration. (ex. `BoundedIntegerField(null=True)`) -- Deploy. -- Remove the column from the model, but in the migration make sure we only mark the state as removed. -- Deploy. -- Finally, create a migration that deletes the column. +- (Optional, but ideal) Make a PR to remove all uses of the column in the codebase in a separate PR. This mostly helps with code cleanliness. This should be merged ahead of the migration prs, but we don't need to worry about whether it is deployed first. +- Make another PR that: + - Checks if the column is either not nullable, or doesn't have a db_default set. If either of these is true, then make it nullable via `null=True`. + - If the column is a foreign key, remove the database level foreign key constraint it by setting `db_constraint=False`. + - Remove the column and in the generated migration use `SafeRemoveField(..., deletion_action=DeletionAction.MOVE_TO_PENDING)` to replace `RemoveField(...)`. This only marks the state for the column as removed. + - Combine these migrations together to save making multiple deploys +- Deploy. It's important that all previous prs are in production before we remove the actual column from the table. +- Make a PR that create a new migration that has the same `SafeRemoveField` operation as before, but set `deletion_action=DeletionAction.DELETE` instead. This deletes the actual column from the table in Postgres. +- Deploy + +Here's an example of removing the `project` column from this model. It is both a foreign key and not null: + +```python +@region_silo_model +class TestModel(Model): + __relocation_scope__ = RelocationScope.Excluded + + project = FlexibleForeignKey("sentry.Project") + name = models.TextField() + + class Meta: + app_label = "uptime" + db_table = "uptime_testmodel" +``` + +First we remove the constraint and make the column nullable: + +```python +# Model change +... +project = FlexibleForeignKey("sentry.Project", db_constraint=False, null=True) +... + +# Migration operations +operations = [ + migrations.AlterField( + model_name="testmodel", + name="project", + field=sentry.db.models.fields.foreignkey.FlexibleForeignKey( + db_constraint=False, + null=True, + on_delete=django.db.models.deletion.CASCADE, + to="sentry.project", + ), + ), +] +``` + +Once we've done this, we can now remove the column from the model and generate the migration to remove it. The generated migration looks like this: + +```python +operations = [ + migrations.RemoveField(model_name="testmodel", name="project"), +] +``` -Here's an example of removing columns that were already nullable. First we remove the columns from the model, and then modify the migration to only update the state and make no database operations. +Deleting this way is unsafe, so we want to replace this with `SafeDeleteModel` instead. So this becomes ```python operations = [ - migrations.SeparateDatabaseAndState( - database_operations=[], - state_operations=[ - migrations.RemoveField(model_name="alertrule", name="alert_threshold"), - migrations.RemoveField(model_name="alertrule", name="resolve_threshold"), - migrations.RemoveField(model_name="alertrule", name="threshold_type"), - ], - ) + SafeRemoveField(model_name="testmodel", name="project", deletion_action=DeletionAction.MOVE_TO_PENDING), ] ``` -Once this is deployed, we can then deploy the actual column deletion. This pr will have only a migration, since Django no longer knows about these fields. Note that the reverse SQL is only for dev, so it's fine to not assign a default or do any sort of backfill: +Using `SafeRemoveField` allows us to remove all the state related to the column, but defer deleting it until a later migration. So now, we can combine the migration to remove the constraints and delete the column state together like so ```python operations = [ - migrations.SeparateDatabaseAndState( - database_operations=[ - migrations.RunSQL( - """ - ALTER TABLE "sentry_alertrule" DROP COLUMN "alert_threshold"; - ALTER TABLE "sentry_alertrule" DROP COLUMN "resolve_threshold"; - ALTER TABLE "sentry_alertrule" DROP COLUMN "threshold_type"; - """, - reverse_sql=""" - ALTER TABLE "sentry_alertrule" ADD COLUMN "alert_threshold" smallint NULL; - ALTER TABLE "sentry_alertrule" ADD COLUMN "resolve_threshold" int NULL; - ALTER TABLE "sentry_alertrule" ADD COLUMN "threshold_type" int NULL; - """, - hints={"tables": ["sentry_alertrule"]}, - ) - ], - state_operations=[], - ) + migrations.AlterField( + model_name="testmodel", + name="project", + field=sentry.db.models.fields.foreignkey.FlexibleForeignKey( + db_constraint=False, + null=True, + on_delete=django.db.models.deletion.CASCADE, + to="sentry.project", + ), + ), + SafeRemoveField(model_name="testmodel", name="project", deletion_action=DeletionAction.MOVE_TO_PENDING), +] +``` + +So now we deploy this migration and move onto the final step. + +In this last step, we will use `SafeRemoveField` again to finally remove the column from the table in Postgres. First, we generate an empty migration using `sentry django makemigrations --empty` to produce an empty migration, and then modify the operations to be like: + +```python +operations = [ + SafeRemoveField(model_name="testmodel", name="project", deletion_action=DeletionAction.DELETE), ] ``` +Then we deploy this and we're done. So to recap the steps here: + - Remove all references to the column in the code in a separate PR and merge. Doesn't matter if this deploys before the next step or not. + - If the column has an fk constraint them remove it. If it's not null and has no db_default then mark it as nullable. Then delete the column using SafeRemoveField(..., deletion_action=DeletionAction.MOVE_TO_PENDING). These operations can be in the same migration to save time. + - Deploy all previous before continuing. + - Remove the column from the table in from Postgres using SafeRemoveField(..., deletion_action=DeletionAction.DELETE), + ### Deleting Tables Extra care is needed here if the table is referenced as a foreign key in other tables. In that case, first remove the foreign key columns in the other tables, then come back to this step. -- Remove any database level foreign key constraints from this table to other tables by setting `db_constraint=False` on the columns. -- Deploy -- Remove the model and all references from the sentry codebase. Make sure that the migration only marks the state as removed. -- Deploy. -- Create a migrations that deletes the table. +- (Optional, but ideal) Make a PR to remove all uses of the model in the codebase in a separate PR. This mostly helps with code cleanliness. This should be merged ahead of the migration prs, but we don't need to worry about whether it is deployed first +- Make another PR to + - Remove any database level foreign key constraints from this table to other tables by setting `db_constraint=False` on the columns. + - Remove the model and in the generated migration use `SafeDeleteModel(..., deletion_action=DeletionAction.MOVE_TO_PENDING)` to replace `DeleteModel(...)`. This only marks the state for the model as removed. +- Deploy. It's important that all previous prs are in production before we remove the actual table. +- Make a PR that create a new migration that has the same `SafeDeleteModel` operation as before, but set `deletion_action=DeletionAction.DELETE` instead. This deletes the actual table from Postgres. - Deploy Here's an example of removing this model: ```python -class AlertRuleTriggerAction(Model): - alert_rule_trigger = FlexibleForeignKey("sentry.AlertRuleTrigger") - integration = FlexibleForeignKey("sentry.Integration", null=True) - type = models.SmallIntegerField() - target_type = models.SmallIntegerField() - # Identifier used to perform the action on a given target - target_identifier = models.TextField(null=True) - # Human readable name to display in the UI - target_display = models.TextField(null=True) - date_added = models.DateTimeField(default=timezone.now) +@region_silo_model +class TestModel(Model): + __relocation_scope__ = RelocationScope.Excluded + + project = FlexibleForeignKey("sentry.Project") + name = models.TextField() class Meta: - app_label = "sentry" - db_table = "sentry_alertruletriggeraction" + app_label = "uptime" + db_table = "uptime_testmodel" ``` -First we checked that it's not referenced by any other models, and it's not. Next we need to remove and db level foreign key constraints. To do this, we change these two columns and generate a migration: +First, we remove all references to this model from the codebase, including making sure that it's not referenced by any other models. This is best done as a separate pr to keep things clean. + +Next we need to remove any db level foreign key constraints. To do this, we change this column and generate a migration: ```python -alert_rule_trigger = FlexibleForeignKey("sentry.AlertRuleTrigger", db_constraint=False) -integration = FlexibleForeignKey("sentry.Integration", null=True, db_constraint=False) +project = FlexibleForeignKey("sentry.Project", db_constraint=False) ``` The operations in the migration look like @@ -233,56 +283,64 @@ The operations in the migration look like ```python operations = [ migrations.AlterField( - model_name='alertruletriggeraction', - name='alert_rule_trigger', - field=sentry.db.models.fields.foreignkey.FlexibleForeignKey(db_constraint=False, on_delete=django.db.models.deletion.CASCADE, to='sentry.AlertRuleTrigger'), - ), - migrations.AlterField( - model_name='alertruletriggeraction', - name='integration', - field=sentry.db.models.fields.foreignkey.FlexibleForeignKey(db_constraint=False, null=True, on_delete=django.db.models.deletion.CASCADE, to='sentry.Integration'), + model_name="testmodel", + name="project", + field=sentry.db.models.fields.foreignkey.FlexibleForeignKey( + db_constraint=False, + on_delete=django.db.models.deletion.CASCADE, + to="sentry.project", + ), ), ] ``` -And we can see the sql it generates just drops the FK constaints +Once we've done this, we can now remove the model from code and generate the migration to remove it. The generated migration looks like this: ```python -BEGIN; -SET CONSTRAINTS "a875987ae7debe6be88869cb2eebcdc5" IMMEDIATE; ALTER TABLE "sentry_alertruletriggeraction" DROP CONSTRAINT "a875987ae7debe6be88869cb2eebcdc5"; -SET CONSTRAINTS "sentry_integration_id_14286d876e86361c_fk_sentry_integration_id" IMMEDIATE; ALTER TABLE "sentry_alertruletriggeraction" DROP CONSTRAINT "sentry_integration_id_14286d876e86361c_fk_sentry_integration_id"; -COMMIT; +operations = [ + migrations.DeleteModel(name="TestModel"), +] ``` -So now we deploy this and move onto the next stage. +Deleting this way is unsafe, so we want to replace this with `SafeDeleteModel` instead. So this becomes -The next stage involves removing all references to the model from the codebase. So we do that, and then we generate a migration that removes the model from the migration state, but not the database. The operations in this migration look like +```python +operations = [ + SafeDeleteModel(name="TestModel", deletion_action=DeletionAction.MOVE_TO_PENDING), +] +``` +Using `SafeDeleteModel` allows us to remove all the state related to the model, but defer deleting it until a later migration. So now, we can combine the migration to remove the constraints and delete the model state together like so ```python operations = [ - migrations.SeparateDatabaseAndState( - state_operations=[migrations.DeleteModel(name="AlertRuleTriggerAction")], - database_operations=[], - ) + migrations.AlterField( + model_name="testmodel", + name="project", + field=sentry.db.models.fields.foreignkey.FlexibleForeignKey( + db_constraint=False, + on_delete=django.db.models.deletion.CASCADE, + to="sentry.project", + ), + ), + SafeDeleteModel(name="TestModel", deletion_action=DeletionAction.MOVE_TO_PENDING), ] ``` -and the generated SQL shows no database changes occurring. So now we deploy this and move into the final step. +So now we deploy this migration and move onto the final step. -In this last step, we just want to manually write DDL to remove the table. So we use `sentry django makemigrations --empty` to produce an empty migration, and then modify the operations to be like: +In this last step, we will use `SafeDeleteModel` again to finally remove the table from Postgres. First, we generate an empty migration using `sentry django makemigrations --empty` to produce an empty migration, and then modify the operations to be like: ```python operations = [ - migrations.RunSQL( - """ - DROP TABLE "sentry_alertruletriggeraction"; - """, - reverse_sql="CREATE TABLE sentry_alertruletriggeraction (fake_col int)", # We just create a fake table here so that the DROP will work if we roll back the migration. - ) + SafeDeleteModel(name="TestModel", deletion_action=DeletionAction.DELETE), ] ``` -Then we deploy this and we're done. +Then we deploy this and we're done. So to recap the steps here: + - Remove all references to the model in the code in a separate PR and merge. Doesn't matter if this deploys before the next step or not. + - Remove any fk constraints and delete the model using SafeDeleteModel(..., deletion_action=DeletionAction.MOVE_TO_PENDING). These operations can be in the same migration to save time. + - Deploy all previous before continuing. + - Remove the table from Postgres using SafeDeleteModel(..., deletion_action=DeletionAction.DELETE), ### Foreign Keys From 8c47ca68d5b52a377e4626d969b0c2c0727e2306 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Wed, 20 Nov 2024 18:18:13 -0800 Subject: [PATCH 02/12] formatting --- .../application-domains/database-migrations/index.mdx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/develop-docs/api-server/application-domains/database-migrations/index.mdx b/develop-docs/api-server/application-domains/database-migrations/index.mdx index 50f4c1dfb251c7..3bdd0e8a38948e 100644 --- a/develop-docs/api-server/application-domains/database-migrations/index.mdx +++ b/develop-docs/api-server/application-domains/database-migrations/index.mdx @@ -147,10 +147,10 @@ To avoid this, follow these steps: - (Optional, but ideal) Make a PR to remove all uses of the column in the codebase in a separate PR. This mostly helps with code cleanliness. This should be merged ahead of the migration prs, but we don't need to worry about whether it is deployed first. - Make another PR that: - - Checks if the column is either not nullable, or doesn't have a db_default set. If either of these is true, then make it nullable via `null=True`. - - If the column is a foreign key, remove the database level foreign key constraint it by setting `db_constraint=False`. - - Remove the column and in the generated migration use `SafeRemoveField(..., deletion_action=DeletionAction.MOVE_TO_PENDING)` to replace `RemoveField(...)`. This only marks the state for the column as removed. - - Combine these migrations together to save making multiple deploys + - Checks if the column is either not nullable, or doesn't have a db_default set. If either of these is true, then make it nullable via `null=True`. + - If the column is a foreign key, remove the database level foreign key constraint it by setting `db_constraint=False`. + - Remove the column and in the generated migration use `SafeRemoveField(..., deletion_action=DeletionAction.MOVE_TO_PENDING)` to replace `RemoveField(...)`. This only marks the state for the column as removed. + - Combine these migrations together to save making multiple deploys - Deploy. It's important that all previous prs are in production before we remove the actual column from the table. - Make a PR that create a new migration that has the same `SafeRemoveField` operation as before, but set `deletion_action=DeletionAction.DELETE` instead. This deletes the actual column from the table in Postgres. - Deploy From c277dd7a4744bbf2d6f318003b16aab84768cc40 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Thu, 21 Nov 2024 14:14:42 -0800 Subject: [PATCH 03/12] Update develop-docs/api-server/application-domains/database-migrations/index.mdx Co-authored-by: Mark Story --- .../application-domains/database-migrations/index.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/develop-docs/api-server/application-domains/database-migrations/index.mdx b/develop-docs/api-server/application-domains/database-migrations/index.mdx index 3bdd0e8a38948e..5674fac164f87a 100644 --- a/develop-docs/api-server/application-domains/database-migrations/index.mdx +++ b/develop-docs/api-server/application-domains/database-migrations/index.mdx @@ -151,7 +151,7 @@ To avoid this, follow these steps: - If the column is a foreign key, remove the database level foreign key constraint it by setting `db_constraint=False`. - Remove the column and in the generated migration use `SafeRemoveField(..., deletion_action=DeletionAction.MOVE_TO_PENDING)` to replace `RemoveField(...)`. This only marks the state for the column as removed. - Combine these migrations together to save making multiple deploys -- Deploy. It's important that all previous prs are in production before we remove the actual column from the table. +- Deploy your migration changes. It's important that all previous pull requests are in production before we remove the actual column from the table. - Make a PR that create a new migration that has the same `SafeRemoveField` operation as before, but set `deletion_action=DeletionAction.DELETE` instead. This deletes the actual column from the table in Postgres. - Deploy From 2dfd97442932d03b116da68fcfcb5290074b2f51 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Thu, 21 Nov 2024 14:14:54 -0800 Subject: [PATCH 04/12] Update develop-docs/api-server/application-domains/database-migrations/index.mdx Co-authored-by: Mark Story --- .../application-domains/database-migrations/index.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/develop-docs/api-server/application-domains/database-migrations/index.mdx b/develop-docs/api-server/application-domains/database-migrations/index.mdx index 5674fac164f87a..d1861aad5ca53a 100644 --- a/develop-docs/api-server/application-domains/database-migrations/index.mdx +++ b/develop-docs/api-server/application-domains/database-migrations/index.mdx @@ -152,7 +152,7 @@ To avoid this, follow these steps: - Remove the column and in the generated migration use `SafeRemoveField(..., deletion_action=DeletionAction.MOVE_TO_PENDING)` to replace `RemoveField(...)`. This only marks the state for the column as removed. - Combine these migrations together to save making multiple deploys - Deploy your migration changes. It's important that all previous pull requests are in production before we remove the actual column from the table. -- Make a PR that create a new migration that has the same `SafeRemoveField` operation as before, but set `deletion_action=DeletionAction.DELETE` instead. This deletes the actual column from the table in Postgres. +- Make a pull request that create a new migration that has the same `SafeRemoveField` operation as before, but set `deletion_action=DeletionAction.DELETE` instead. This deletes the actual column from the table in Postgres. - Deploy Here's an example of removing the `project` column from this model. It is both a foreign key and not null: From db177e1fccba71292b3ebe1f735d1a18b1ccd52b Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Thu, 21 Nov 2024 14:15:04 -0800 Subject: [PATCH 05/12] Update develop-docs/api-server/application-domains/database-migrations/index.mdx Co-authored-by: Mark Story --- .../application-domains/database-migrations/index.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/develop-docs/api-server/application-domains/database-migrations/index.mdx b/develop-docs/api-server/application-domains/database-migrations/index.mdx index d1861aad5ca53a..ba202574399fd6 100644 --- a/develop-docs/api-server/application-domains/database-migrations/index.mdx +++ b/develop-docs/api-server/application-domains/database-migrations/index.mdx @@ -153,7 +153,7 @@ To avoid this, follow these steps: - Combine these migrations together to save making multiple deploys - Deploy your migration changes. It's important that all previous pull requests are in production before we remove the actual column from the table. - Make a pull request that create a new migration that has the same `SafeRemoveField` operation as before, but set `deletion_action=DeletionAction.DELETE` instead. This deletes the actual column from the table in Postgres. -- Deploy +- Deploy the drop column migration. Here's an example of removing the `project` column from this model. It is both a foreign key and not null: From 19c3cb44df358085efe3f8022997824b93226581 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Thu, 21 Nov 2024 14:15:22 -0800 Subject: [PATCH 06/12] Update develop-docs/api-server/application-domains/database-migrations/index.mdx Co-authored-by: Mark Story --- .../application-domains/database-migrations/index.mdx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/develop-docs/api-server/application-domains/database-migrations/index.mdx b/develop-docs/api-server/application-domains/database-migrations/index.mdx index ba202574399fd6..c0515f59c8c982 100644 --- a/develop-docs/api-server/application-domains/database-migrations/index.mdx +++ b/develop-docs/api-server/application-domains/database-migrations/index.mdx @@ -238,10 +238,10 @@ operations = [ ``` Then we deploy this and we're done. So to recap the steps here: - - Remove all references to the column in the code in a separate PR and merge. Doesn't matter if this deploys before the next step or not. - - If the column has an fk constraint them remove it. If it's not null and has no db_default then mark it as nullable. Then delete the column using SafeRemoveField(..., deletion_action=DeletionAction.MOVE_TO_PENDING). These operations can be in the same migration to save time. + - Remove all references to the column in the code in a separate pull request and merge. Doesn't matter if this deploys before the next step or not. + - If the column has an foreign key constraint them remove it. If it's not null and has no db_default then mark it as nullable. Then delete the column using `SafeRemoveField(..., deletion_action=DeletionAction.MOVE_TO_PENDING)`. These operations can be in the same migration to save time. - Deploy all previous before continuing. - - Remove the column from the table in from Postgres using SafeRemoveField(..., deletion_action=DeletionAction.DELETE), + - Remove the column from the table in from Postgres using `SafeRemoveField(..., deletion_action=DeletionAction.DELETE),` ### Deleting Tables From bbc2c93f45fa3ba4d556312a66784530b30e759e Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Thu, 21 Nov 2024 14:15:31 -0800 Subject: [PATCH 07/12] Update develop-docs/api-server/application-domains/database-migrations/index.mdx Co-authored-by: Mark Story --- .../application-domains/database-migrations/index.mdx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/develop-docs/api-server/application-domains/database-migrations/index.mdx b/develop-docs/api-server/application-domains/database-migrations/index.mdx index c0515f59c8c982..e20fc80e50cbb8 100644 --- a/develop-docs/api-server/application-domains/database-migrations/index.mdx +++ b/develop-docs/api-server/application-domains/database-migrations/index.mdx @@ -247,8 +247,8 @@ Then we deploy this and we're done. So to recap the steps here: Extra care is needed here if the table is referenced as a foreign key in other tables. In that case, first remove the foreign key columns in the other tables, then come back to this step. -- (Optional, but ideal) Make a PR to remove all uses of the model in the codebase in a separate PR. This mostly helps with code cleanliness. This should be merged ahead of the migration prs, but we don't need to worry about whether it is deployed first -- Make another PR to +- (Optional, but ideal) Make a pull request to remove all uses of the model in the codebase in a separate pull request. This mostly helps with code cleanliness. This should be merged ahead of the migration pull requests, but we don't need to worry about whether it is deployed first. +- Make another pull request to: - Remove any database level foreign key constraints from this table to other tables by setting `db_constraint=False` on the columns. - Remove the model and in the generated migration use `SafeDeleteModel(..., deletion_action=DeletionAction.MOVE_TO_PENDING)` to replace `DeleteModel(...)`. This only marks the state for the model as removed. - Deploy. It's important that all previous prs are in production before we remove the actual table. From dadbce6ffcd55d9d0ecfde91d0ca5ab38bb52035 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Thu, 21 Nov 2024 14:15:40 -0800 Subject: [PATCH 08/12] Update develop-docs/api-server/application-domains/database-migrations/index.mdx Co-authored-by: Mark Story --- .../application-domains/database-migrations/index.mdx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/develop-docs/api-server/application-domains/database-migrations/index.mdx b/develop-docs/api-server/application-domains/database-migrations/index.mdx index e20fc80e50cbb8..e8a1ad02fc9d4c 100644 --- a/develop-docs/api-server/application-domains/database-migrations/index.mdx +++ b/develop-docs/api-server/application-domains/database-migrations/index.mdx @@ -251,8 +251,8 @@ Extra care is needed here if the table is referenced as a foreign key in other t - Make another pull request to: - Remove any database level foreign key constraints from this table to other tables by setting `db_constraint=False` on the columns. - Remove the model and in the generated migration use `SafeDeleteModel(..., deletion_action=DeletionAction.MOVE_TO_PENDING)` to replace `DeleteModel(...)`. This only marks the state for the model as removed. -- Deploy. It's important that all previous prs are in production before we remove the actual table. -- Make a PR that create a new migration that has the same `SafeDeleteModel` operation as before, but set `deletion_action=DeletionAction.DELETE` instead. This deletes the actual table from Postgres. +- Deploy. It's important that all previous pull requests are in production before we remove the actual table. +- Make a pull request that create a new migration that has the same `SafeDeleteModel` operation as before, but set `deletion_action=DeletionAction.DELETE` instead. This deletes the actual table from Postgres. - Deploy Here's an example of removing this model: From 66b6353e3e8b08804d8b8aaa5ac7ce0ebe2ff8eb Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Thu, 21 Nov 2024 14:15:53 -0800 Subject: [PATCH 09/12] Update develop-docs/api-server/application-domains/database-migrations/index.mdx Co-authored-by: Mark Story --- .../application-domains/database-migrations/index.mdx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/develop-docs/api-server/application-domains/database-migrations/index.mdx b/develop-docs/api-server/application-domains/database-migrations/index.mdx index e8a1ad02fc9d4c..d1d182d1f7862a 100644 --- a/develop-docs/api-server/application-domains/database-migrations/index.mdx +++ b/develop-docs/api-server/application-domains/database-migrations/index.mdx @@ -337,10 +337,10 @@ operations = [ ``` Then we deploy this and we're done. So to recap the steps here: - - Remove all references to the model in the code in a separate PR and merge. Doesn't matter if this deploys before the next step or not. - - Remove any fk constraints and delete the model using SafeDeleteModel(..., deletion_action=DeletionAction.MOVE_TO_PENDING). These operations can be in the same migration to save time. + - Remove all references to the model in the code in a separate pull request and merge. Doesn't matter if this deploys before the next step or not. + - Remove any foreign key constraints and delete the model using `SafeDeleteModel(..., deletion_action=DeletionAction.MOVE_TO_PENDING)`. These operations can be in the same migration to save time. - Deploy all previous before continuing. - - Remove the table from Postgres using SafeDeleteModel(..., deletion_action=DeletionAction.DELETE), + - Remove the table from Postgres using `SafeDeleteModel(..., deletion_action=DeletionAction.DELETE),` ### Foreign Keys From d9c95015d0c370fdc8ff69f8c6468910021e34bf Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Fri, 22 Nov 2024 10:02:29 -0800 Subject: [PATCH 10/12] try to improve the model deletion docs --- .../database-migrations/index.mdx | 66 ++++++++++++++----- 1 file changed, 51 insertions(+), 15 deletions(-) diff --git a/develop-docs/api-server/application-domains/database-migrations/index.mdx b/develop-docs/api-server/application-domains/database-migrations/index.mdx index d1d182d1f7862a..f94347260dd152 100644 --- a/develop-docs/api-server/application-domains/database-migrations/index.mdx +++ b/develop-docs/api-server/application-domains/database-migrations/index.mdx @@ -252,7 +252,7 @@ Extra care is needed here if the table is referenced as a foreign key in other t - Remove any database level foreign key constraints from this table to other tables by setting `db_constraint=False` on the columns. - Remove the model and in the generated migration use `SafeDeleteModel(..., deletion_action=DeletionAction.MOVE_TO_PENDING)` to replace `DeleteModel(...)`. This only marks the state for the model as removed. - Deploy. It's important that all previous pull requests are in production before we remove the actual table. -- Make a pull request that create a new migration that has the same `SafeDeleteModel` operation as before, but set `deletion_action=DeletionAction.DELETE` instead. This deletes the actual table from Postgres. +- Make a pull request that creates a new migration that has the same `SafeDeleteModel` operation as before, but set `deletion_action=DeletionAction.DELETE` instead. This deletes the actual table from Postgres. - Deploy Here's an example of removing this model: @@ -272,14 +272,56 @@ class TestModel(Model): First, we remove all references to this model from the codebase, including making sure that it's not referenced by any other models. This is best done as a separate pr to keep things clean. -Next we need to remove any db level foreign key constraints. To do this, we change this column and generate a migration: +Next we produce two migrations, in individual prs that we will deploy separately. +First PR ```python -project = FlexibleForeignKey("sentry.Project", db_constraint=False) +# + +# First migration +# ... Migration boilerplate ... +operations = [ + migrations.AlterField( + model_name="testmodel", + name="project", + field=sentry.db.models.fields.foreignkey.FlexibleForeignKey( + db_constraint=False, + on_delete=django.db.models.deletion.CASCADE, + to="sentry.project", + ), + ), + SafeDeleteModel(name="TestModel", deletion_action=DeletionAction.MOVE_TO_PENDING), +] +# ... Migration boilerplate ... +``` + +Second PR: +```python +# Second migration +# ... Migration boilerplate ... +operations = [ + SafeDeleteModel(name="TestModel", deletion_action=DeletionAction.DELETE), +] +# ... Migration boilerplate ... ``` -The operations in the migration look like +So once we have these two prs, we merge/deploy the first, and then the second and then the table is fully removed. + +So to recap the steps here: + - Remove all references to the model in the code in a separate pull request and merge. Doesn't matter if this deploys before the next step or not. + - Remove any foreign key constraints and delete the model using `SafeDeleteModel(..., deletion_action=DeletionAction.MOVE_TO_PENDING)`. These operations can be in the same migration to save time. + - Deploy all previous before continuing. + - Remove the table from Postgres using `SafeDeleteModel(..., deletion_action=DeletionAction.DELETE),` + +If you're comfortable producing these prs and deploying them, then stop here. Otherwise, this next section covers how to produce them in more detail. + +To produce the first migration, we need to remove any db level foreign key constraints and remove the table from the codebase. To remove the db level foreign key constraints we add `db_constraint` to this column and generate a migration: +```python +project = FlexibleForeignKey("sentry.Project", db_constraint=False) +``` + +This produces a migration with operations like: ```python operations = [ migrations.AlterField( @@ -294,7 +336,7 @@ operations = [ ] ``` -Once we've done this, we can now remove the model from code and generate the migration to remove it. The generated migration looks like this: +Next, we remove the model from code and generate the migration to remove it. The generated migration looks like this: ```python operations = [ @@ -302,7 +344,7 @@ operations = [ ] ``` -Deleting this way is unsafe, so we want to replace this with `SafeDeleteModel` instead. So this becomes +Django doesn't know about the `SafeDeleteModel` operation, so we replace it with that instead. This allows us to remove all state related to the model, but defer deleting it until a later migration. So this becomes ```python operations = [ @@ -310,7 +352,7 @@ operations = [ ] ``` -Using `SafeDeleteModel` allows us to remove all the state related to the model, but defer deleting it until a later migration. So now, we can combine the migration to remove the constraints and delete the model state together like so +So now as a final step, we can combine these operations into a single migration, which is the first migration we want to deploy. ```python operations = [ migrations.AlterField( @@ -326,9 +368,7 @@ operations = [ ] ``` -So now we deploy this migration and move onto the final step. - -In this last step, we will use `SafeDeleteModel` again to finally remove the table from Postgres. First, we generate an empty migration using `sentry django makemigrations --empty` to produce an empty migration, and then modify the operations to be like: +To produce the second migration we generate an empty migration (`sentry django makemigrations --empty`), then use the same `SafeDeleteModel` command from the previous migration, but change the deletion_action to `DeletionAction.DELETE`. This operation will remove the table from Postgres: ```python operations = [ @@ -336,11 +376,7 @@ operations = [ ] ``` -Then we deploy this and we're done. So to recap the steps here: - - Remove all references to the model in the code in a separate pull request and merge. Doesn't matter if this deploys before the next step or not. - - Remove any foreign key constraints and delete the model using `SafeDeleteModel(..., deletion_action=DeletionAction.MOVE_TO_PENDING)`. These operations can be in the same migration to save time. - - Deploy all previous before continuing. - - Remove the table from Postgres using `SafeDeleteModel(..., deletion_action=DeletionAction.DELETE),` +This second PR will contain only the migration and related boilerplate. ### Foreign Keys From 4762d481bd92d24a9bf10a21ffd7a1e5f7a8c3c4 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Fri, 22 Nov 2024 10:05:18 -0800 Subject: [PATCH 11/12] small fixes --- .../application-domains/database-migrations/index.mdx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/develop-docs/api-server/application-domains/database-migrations/index.mdx b/develop-docs/api-server/application-domains/database-migrations/index.mdx index f94347260dd152..dc9e4c6022ad4c 100644 --- a/develop-docs/api-server/application-domains/database-migrations/index.mdx +++ b/develop-docs/api-server/application-domains/database-migrations/index.mdx @@ -145,9 +145,9 @@ This is complicated due to our deploy process. When we deploy, we run migrations To avoid this, follow these steps: -- (Optional, but ideal) Make a PR to remove all uses of the column in the codebase in a separate PR. This mostly helps with code cleanliness. This should be merged ahead of the migration prs, but we don't need to worry about whether it is deployed first. +- Make a PR to remove all uses of the column in the codebase in a separate PR. This mostly helps with code cleanliness. This should be merged ahead of the migration prs, but we don't need to worry about whether it is deployed first. - Make another PR that: - - Checks if the column is either not nullable, or doesn't have a db_default set. If either of these is true, then make it nullable via `null=True`. + - Checks if the column is either not nullable, or doesn't have a `db_default` set. If either of these is true, then make it nullable via `null=True`. - If the column is a foreign key, remove the database level foreign key constraint it by setting `db_constraint=False`. - Remove the column and in the generated migration use `SafeRemoveField(..., deletion_action=DeletionAction.MOVE_TO_PENDING)` to replace `RemoveField(...)`. This only marks the state for the column as removed. - Combine these migrations together to save making multiple deploys @@ -239,7 +239,7 @@ operations = [ Then we deploy this and we're done. So to recap the steps here: - Remove all references to the column in the code in a separate pull request and merge. Doesn't matter if this deploys before the next step or not. - - If the column has an foreign key constraint them remove it. If it's not null and has no db_default then mark it as nullable. Then delete the column using `SafeRemoveField(..., deletion_action=DeletionAction.MOVE_TO_PENDING)`. These operations can be in the same migration to save time. + - If the column has an foreign key constraint them remove it. If it's not null and has no `db_default` then mark it as nullable. Then delete the column using `SafeRemoveField(..., deletion_action=DeletionAction.MOVE_TO_PENDING)`. These operations can be in the same migration to save time. - Deploy all previous before continuing. - Remove the column from the table in from Postgres using `SafeRemoveField(..., deletion_action=DeletionAction.DELETE),` @@ -247,7 +247,7 @@ Then we deploy this and we're done. So to recap the steps here: Extra care is needed here if the table is referenced as a foreign key in other tables. In that case, first remove the foreign key columns in the other tables, then come back to this step. -- (Optional, but ideal) Make a pull request to remove all uses of the model in the codebase in a separate pull request. This mostly helps with code cleanliness. This should be merged ahead of the migration pull requests, but we don't need to worry about whether it is deployed first. +- Make a pull request to remove all uses of the model in the codebase in a separate pull request. This mostly helps with code cleanliness. This should be merged ahead of the migration pull requests, but we don't need to worry about whether it is deployed first. - Make another pull request to: - Remove any database level foreign key constraints from this table to other tables by setting `db_constraint=False` on the columns. - Remove the model and in the generated migration use `SafeDeleteModel(..., deletion_action=DeletionAction.MOVE_TO_PENDING)` to replace `DeleteModel(...)`. This only marks the state for the model as removed. From 92b26dabfbce51f3eb93fd577bc9ce6ccb7bcd62 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Fri, 22 Nov 2024 11:34:48 -0800 Subject: [PATCH 12/12] improve docs for cols --- .../database-migrations/index.mdx | 64 +++++++++++++++---- 1 file changed, 51 insertions(+), 13 deletions(-) diff --git a/develop-docs/api-server/application-domains/database-migrations/index.mdx b/develop-docs/api-server/application-domains/database-migrations/index.mdx index dc9e4c6022ad4c..6c52a0e5ef4d62 100644 --- a/develop-docs/api-server/application-domains/database-migrations/index.mdx +++ b/develop-docs/api-server/application-domains/database-migrations/index.mdx @@ -170,7 +170,53 @@ class TestModel(Model): db_table = "uptime_testmodel" ``` -First we remove the constraint and make the column nullable: +First, we remove all references to this field from the codebase. This is best done as a separate pr to keep things clean. + +Next we produce two migrations, in individual prs that we will deploy separately. + +First PR +```python +# + +# First migration +# ... Migration boilerplate ... +operations = [ + migrations.AlterField( + model_name="testmodel", + name="project", + field=sentry.db.models.fields.foreignkey.FlexibleForeignKey( + db_constraint=False, + null=True, + on_delete=django.db.models.deletion.CASCADE, + to="sentry.project", + ), + ), + SafeRemoveField(model_name="testmodel", name="project", deletion_action=DeletionAction.MOVE_TO_PENDING), +] +# ... Migration boilerplate ... +``` + +Second PR: +```python +# Second migration +# ... Migration boilerplate ... +operations = [ + SafeRemoveField(model_name="testmodel", name="project", deletion_action=DeletionAction.DELETE), +] +# ... Migration boilerplate ... +``` + +So once we have these two prs, we merge/deploy the first, and then the second and then the table is fully removed. + +So to recap the steps here: + - Remove all references to the column in the code in a separate pull request and merge. Doesn't matter if this deploys before the next step or not. + - If the column has a foreign key constraint them remove it. If it's not null and has no `db_default` then mark it as nullable. Then delete the column using `SafeRemoveField(..., deletion_action=DeletionAction.MOVE_TO_PENDING)`. These operations can be in the same migration to save time. + - Deploy all previous before continuing. + - Remove the column from the table in from Postgres using `SafeRemoveField(..., deletion_action=DeletionAction.DELETE),` + +If you're comfortable producing these prs and deploying them, then stop here. Otherwise, this next section covers how to produce them in more detail. + +To produce the first migration, we need to remove the db level foreign key constraint, make the column nullable and remove the column from the codebase. To remove the db level foreign key constraints and mark the column nullable we add `db_constraint=False, null=True` to this column and generate a migration: ```python # Model change @@ -201,7 +247,7 @@ operations = [ ] ``` -Deleting this way is unsafe, so we want to replace this with `SafeDeleteModel` instead. So this becomes +Django doesn't know about the `SafeRemoveField` operation, so we replace it with that instead. This allows us to remove all state related to the column, but defer deleting it until a later migration. So this becomes ```python operations = [ @@ -209,7 +255,7 @@ operations = [ ] ``` -Using `SafeRemoveField` allows us to remove all the state related to the column, but defer deleting it until a later migration. So now, we can combine the migration to remove the constraints and delete the column state together like so +So now as a final step, we can combine these operations into a single migration, which is the first migration we want to deploy. ```python operations = [ @@ -227,9 +273,7 @@ operations = [ ] ``` -So now we deploy this migration and move onto the final step. - -In this last step, we will use `SafeRemoveField` again to finally remove the column from the table in Postgres. First, we generate an empty migration using `sentry django makemigrations --empty` to produce an empty migration, and then modify the operations to be like: +To produce the second migration we generate an empty migration (`sentry django makemigrations --empty`), then use the same `SafeRemoveField` command from the previous migration, but change the deletion_action to `DeletionAction.DELETE`. This operation will remove the column from the table in Postgres: ```python operations = [ @@ -237,12 +281,6 @@ operations = [ ] ``` -Then we deploy this and we're done. So to recap the steps here: - - Remove all references to the column in the code in a separate pull request and merge. Doesn't matter if this deploys before the next step or not. - - If the column has an foreign key constraint them remove it. If it's not null and has no `db_default` then mark it as nullable. Then delete the column using `SafeRemoveField(..., deletion_action=DeletionAction.MOVE_TO_PENDING)`. These operations can be in the same migration to save time. - - Deploy all previous before continuing. - - Remove the column from the table in from Postgres using `SafeRemoveField(..., deletion_action=DeletionAction.DELETE),` - ### Deleting Tables Extra care is needed here if the table is referenced as a foreign key in other tables. In that case, first remove the foreign key columns in the other tables, then come back to this step. @@ -315,7 +353,7 @@ So to recap the steps here: If you're comfortable producing these prs and deploying them, then stop here. Otherwise, this next section covers how to produce them in more detail. -To produce the first migration, we need to remove any db level foreign key constraints and remove the table from the codebase. To remove the db level foreign key constraints we add `db_constraint` to this column and generate a migration: +To produce the first migration, we need to remove any db level foreign key constraints and remove the table from the codebase. To remove the db level foreign key constraints we add `db_constraint=False` to this column and generate a migration: ```python project = FlexibleForeignKey("sentry.Project", db_constraint=False)