From 71d783a62d7ee5624416f9d86fc77e848a8ea987 Mon Sep 17 00:00:00 2001 From: Seiji Chew Date: Tue, 18 Mar 2025 19:01:37 -0700 Subject: [PATCH 1/3] Change adding-not-nullable field rule to ignore postgres history --- docs/docs/adding-field-with-default.md | 4 + docs/docs/adding-not-nullable-field.md | 99 ++++++++++------------- linter/src/rules/adding_not_null_field.rs | 42 ++-------- 3 files changed, 54 insertions(+), 91 deletions(-) diff --git a/docs/docs/adding-field-with-default.md b/docs/docs/adding-field-with-default.md index eae10530..95f96471 100644 --- a/docs/docs/adding-field-with-default.md +++ b/docs/docs/adding-field-with-default.md @@ -7,7 +7,11 @@ title: adding-field-with-default Adding a field with a default can cause table rewrites, which will take an [`ACCESS EXCLUSIVE`](https://www.postgresql.org/docs/10/sql-altertable.html#SQL-ALTERTABLE-NOTES) lock on the table, blocking reads / writes while the statement is running. +:::note Postgres Version + In Postgres version 11 and later, adding a field with a non-`VOLATILE` `DEFAULT` will not require a table rewrite. Adding a field with a [`VOLATILE` `DEFAULT` will cause a table rewrite](https://www.postgresql.org/docs/14/sql-altertable.html#SQL-ALTERTABLE-NOTES). +::: + ## solutions diff --git a/docs/docs/adding-not-nullable-field.md b/docs/docs/adding-not-nullable-field.md index cde5d020..3372dfd9 100644 --- a/docs/docs/adding-not-nullable-field.md +++ b/docs/docs/adding-not-nullable-field.md @@ -5,41 +5,16 @@ title: adding-not-nullable-field Use a check constraint instead of setting a column as `NOT NULL`. -:::note Postgres Version - -In Postgres versions 11 of later, adding a non-null column with a default will complete without a table scan. -::: - ## problem -Adding a column as `NOT NULL` requires a table scan and the `ALTER TABLE` requires -an `ACCESS EXCLUSIVE` lock. Reads and writes will be disabled while this statement is running. - -## solutions - -### adding a non-nullable column - -Add a column as nullable and use a check constraint to verify integrity. The check constraint should be added as `NOT NULL` and then validated. - -Instead of: - -```sql -ALTER TABLE "recipe" ADD COLUMN "view_count" integer DEFAULT 10 NOT NULL; -``` +Adding a column as `NOT NULL` is no longer covered by this rule. See ["adding-required-field (set a non-volatile default)"](adding-required-field.md#set-a-non-volatile-default) for more information on how to add new columns with `NOT NULL`. -Use: -```sql -ALTER TABLE "recipe" ADD COLUMN "view_count" integer DEFAULT 10; -ALTER TABLE "recipe" ADD CONSTRAINT view_count_not_null - CHECK ("view_count" IS NOT NULL) NOT VALID; --- backfill column so it's not null -ALTER TABLE "recipe" VALIDATE CONSTRAINT view_count_not_null; -``` +Modifying a column to be `NOT NULL` may fail if the column contains records with a `NULL` value, requiring a full table scan to check before executing. Old application code may also try to write `NULL` values to the table. -Add the column as nullable, add a check constraint as `NOT VALID` to verify new rows and updates are, backfill the column so it no longer contains null values, validate the constraint to verify existing rows are valid. +`ALTER TABLE` also requires an `ACCESS EXCLUSIVE` lock which will disable reads and writes while this statement is running. -See ["How not valid constraints work"](constraint-missing-not-valid.md#how-not-valid-validate-works) for more information on adding constraints as `NOT VALID`. +## solutions ### setting an existing column as non-nullable @@ -54,11 +29,20 @@ Use: ```sql ALTER TABLE "recipe" ADD CONSTRAINT view_count_not_null CHECK ("view_count" IS NOT NULL) NOT VALID; --- backfill column so it's not null + +-- Backfill column so it's not null +-- Update ... + ALTER TABLE "recipe" VALIDATE CONSTRAINT view_count_not_null; -``` -Add a check constraint as `NOT VALID` to verify new rows and updates are, backfill the column so it no longer contains null values, validate the constraint to verify existing rows are valid. +-- If pg version >= 12, set not null without doing a table scan +ALTER TABLE table_name ALTER COLUMN column_name SET NOT NULL; +``` +For each step, note that: +1. Adding the constraint acquires an `ACCESS EXCLUSIVE` lock, but this is done extremely quickly. +2. You MUST backfill the column before validating the constraint +3. Validating the constraint does require a full table scan, but only acquires a [`SHARE UPDATE EXCLUSIVE`](https://www.postgresql.org/docs/current/sql-altertable.html#SQL-ALTERTABLE-DESC-VALIDATE-CONSTRAINT) lock which allows for normal operations to continue. +4. If using postgres version 12 or later, it forgoes the full table scan by checking the existing constraint. See ["How not valid constraints work"](constraint-missing-not-valid.md#how-not-valid-validate-works) for more information on adding constraints as `NOT VALID`. @@ -68,41 +52,33 @@ See ["How not valid constraints work"](constraint-missing-not-valid.md#how-not-v Instead of: ```python -# models.py +# migrations/*.py +from alembic import op import sqlalchemy as sa -class Recipe(BaseModel): - view_count = sa.Column(sa.BigInteger, default=10, nullable=False) -``` - -Use: - -```python -# models.py -import sqlalchemy as sa +def schema_upgrades(): + op.alter_column( + "recipe", + "view_count", + existing_type=sa.BigInteger(), + nullable=False, + ) -class Recipe(BaseModel): - __table_args__ = ( - sa.CheckConstraint( - "view_count IS NOT NULL", - name="view_count_not_null", - ) +def schema_downgrades(): + op.alter_column( + "recipe", + "view_count", + existing_type=sa.BigInteger(), + nullable=True, ) - view_count = sa.Column(sa.BigInteger, default=10, nullable=True) ``` -Create Alembic migration manually, because Alembic not creates migration for constraints automatically. See the related [GitHub Issue here](https://github.com/sqlalchemy/alembic/issues/508). - ```python # migrations/*.py from alembic import op import sqlalchemy as sa def schema_upgrades(): - op.add_column( - "recipe", - sa.Column("view_count", sa.BigInteger(), nullable=True), - ) op.create_check_constraint( constraint_name="view_count_not_null", table_name="recipe", @@ -112,12 +88,23 @@ def schema_upgrades(): op.execute( sa.text("ALTER TABLE recipe VALIDATE CONSTRAINT view_count_not_null"), ) + op.alter_column( + "recipe", + "view_count", + existing_type=sa.BigInteger(), + nullable=False, + ) def schema_downgrades(): + op.alter_column( + "recipe", + "view_count", + existing_type=sa.BigInteger(), + nullable=True, + ) op.drop_constraint( constraint_name="view_count_not_null", table_name="recipe", ) - op.drop_column("recipe", "view_count") ``` diff --git a/linter/src/rules/adding_not_null_field.rs b/linter/src/rules/adding_not_null_field.rs index afaf012c..0aa6d8f2 100644 --- a/linter/src/rules/adding_not_null_field.rs +++ b/linter/src/rules/adding_not_null_field.rs @@ -7,16 +7,10 @@ use squawk_parser::ast::{AlterTableCmds, AlterTableType, RawStmt, Stmt}; #[must_use] pub fn adding_not_nullable_field( tree: &[RawStmt], - pg_version: Option, + _pg_version: Option, _assume_in_transaction: bool, ) -> Vec { let mut errs = vec![]; - if let Some(pg_version) = pg_version { - let pg_11 = Version::new(11, Some(0), Some(0)); - if pg_version >= pg_11 { - return errs; - } - } for raw_stmt in tree { match &raw_stmt.stmt { @@ -42,32 +36,23 @@ pub fn adding_not_nullable_field( #[cfg(test)] mod test_rules { - use std::str::FromStr; - use crate::{ check_sql_with_rule, - versions::Version, violations::{RuleViolation, RuleViolationKind}, }; - fn lint_sql(sql: &str, pg_version: Option) -> Vec { - check_sql_with_rule( - sql, - &RuleViolationKind::AddingNotNullableField, - pg_version, - false, - ) - .unwrap() + fn lint_sql(sql: &str) -> Vec { + check_sql_with_rule(sql, &RuleViolationKind::AddingNotNullableField, None, false).unwrap() } use insta::assert_debug_snapshot; #[test] fn set_not_null() { - let sql = r#" + let bad_sql = r#" ALTER TABLE "core_recipe" ALTER COLUMN "foo" SET NOT NULL; "#; - assert_debug_snapshot!(lint_sql(sql, None)); + assert_debug_snapshot!(lint_sql(bad_sql)); } #[test] @@ -80,7 +65,7 @@ ALTER TABLE "core_recipe" ADD COLUMN "foo" integer DEFAULT 10 NOT NULL; ALTER TABLE "core_recipe" ALTER COLUMN "foo" DROP DEFAULT; COMMIT; "#; - assert_debug_snapshot!(lint_sql(ok_sql, None)); + assert_debug_snapshot!(lint_sql(ok_sql)); } #[test] @@ -89,19 +74,6 @@ COMMIT; -- This won't work if the table is populated, but that error is caught by adding-required-field. ALTER TABLE "core_recipe" ADD COLUMN "foo" integer NOT NULL; "#; - assert_debug_snapshot!(lint_sql(ok_sql, None)); - } - - #[test] - fn adding_field_that_is_not_nullable_in_version_11() { - let ok_sql = r#" -BEGIN; --- --- Add field foo to recipe --- -ALTER TABLE "core_recipe" ADD COLUMN "foo" integer NOT NULL DEFAULT 10; -COMMIT; - "#; - assert_debug_snapshot!(lint_sql(ok_sql, Some(Version::from_str("11.0.0").unwrap()),)); + assert_debug_snapshot!(lint_sql(ok_sql)); } } From 52e9b6b25edaab936cc3d35c623f5a22d8d95b93 Mon Sep 17 00:00:00 2001 From: Seiji Chew Date: Tue, 18 Mar 2025 20:24:30 -0700 Subject: [PATCH 2/3] add comment into upgrade --- docs/docs/adding-not-nullable-field.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/docs/adding-not-nullable-field.md b/docs/docs/adding-not-nullable-field.md index 3372dfd9..fa31c03b 100644 --- a/docs/docs/adding-not-nullable-field.md +++ b/docs/docs/adding-not-nullable-field.md @@ -85,6 +85,8 @@ def schema_upgrades(): condition="view_count IS NOT NULL", postgresql_not_valid=True, ) + # Backfill existing rows to get rid of any NULL values. You have have to split + # this into two migrations op.execute( sa.text("ALTER TABLE recipe VALIDATE CONSTRAINT view_count_not_null"), ) From 9c857cb4fd088950881ddaff78ac801cde279f58 Mon Sep 17 00:00:00 2001 From: Seiji Chew Date: Tue, 18 Mar 2025 21:09:47 -0700 Subject: [PATCH 3/3] add clarifying comment to alembic migration --- docs/docs/adding-not-nullable-field.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/docs/adding-not-nullable-field.md b/docs/docs/adding-not-nullable-field.md index fa31c03b..b9cf8b4e 100644 --- a/docs/docs/adding-not-nullable-field.md +++ b/docs/docs/adding-not-nullable-field.md @@ -90,7 +90,7 @@ def schema_upgrades(): op.execute( sa.text("ALTER TABLE recipe VALIDATE CONSTRAINT view_count_not_null"), ) - op.alter_column( + op.alter_column( # only include this step on pg version >= 12 "recipe", "view_count", existing_type=sa.BigInteger(), @@ -99,7 +99,7 @@ def schema_upgrades(): def schema_downgrades(): - op.alter_column( + op.alter_column( # only include this step on pg version >= 12 "recipe", "view_count", existing_type=sa.BigInteger(),