Ignore postgres version in adding-not-nullable-field (#412)

### Summary
Hey there, I noticed in #301 that the logic for handling adding new fields as null was split out from [adding-not-nullable-field](https://squawkhq.com/docs/adding-not-nullable-field) into [adding-required-field](https://squawkhq.com/docs/adding-required-field).

The logic for performing the pg version check wasn't removed however, so this causes the rule to fail to detect changing existing columns to be not null. All the logic for checking not null for adding cols was extracted to `adding-required-field` or covered by `adding-required-field`, so I think we can remove the version check altogether.

### Examples
#301 has nice examples showing how the other rules cover the adding column with not null. I've included the part below to show what changes with this PR.

Previously with --pg-version >=12:
```SQL
ALTER TABLE foo ALTER COLUMN d SET NOT NULL; -- no warning,
```

This PR with --pg-version >=12:
```SQL
ALTER TABLE foo ALTER COLUMN d SET NOT NULL; -- triggers adding-not-nullable-field
```

### Documentation
I updated `adding-not-nullable-field` to explicitly mention that it no longer covers adding cols, and added to the problem+solution sections as well.

I think ideally it'd be best to maybe re-name this rule to clarify the fact that it only covers modifying existing cols now, but that may be hard to do

---
Please let me know if this change may not be the best approach, or if there are any concerns!

Author: schew2381

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
 

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,56 +52,61 @@ 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",
         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"),
     )
+    op.alter_column( # only include this step on pg version >= 12
+        "recipe",
+        "view_count",
+        existing_type=sa.BigInteger(),
+        nullable=False,
+    )
 
 
 def schema_downgrades():
+    op.alter_column( # only include this step on pg version >= 12
+        "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")
 ```

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<Version>,
+    _pg_version: Option<Version>,
     _assume_in_transaction: bool,
 ) -> Vec<RuleViolation> {
     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<Version>) -> Vec<RuleViolation> {
-        check_sql_with_rule(
-            sql,
-            &RuleViolationKind::AddingNotNullableField,
-            pg_version,
-            false,
-        )
-        .unwrap()
+    fn lint_sql(sql: &str) -> Vec<RuleViolation> {
+        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));
     }
 }