add rules to ban domains with constraints

docs/docs/ban-alter-domain-with-add-constraint.md

@@ -0,0 +1,41 @@
+---
+id: ban-alter-domain-with-add-constraint
+title: ban-alter-domain-with-add-constraint
+---
+
+## problem
+
+Postgres [domains][], which associate a data type with an optional check constraint, have poor support for online migrations
+when associated with a check constraint.
+
+[domains]: https://www.postgresql.org/docs/current/sql-createdomain.html
+
+The purpose of domains is to make the named type-plus-constraint reusable, but this means that any change to the domain's constraint
+requires _all_ columns that use the domain to be revalidated. And, because Postgres can't reason well about arbitrary constraints,
+they increase the chances of a change requiring an expensive table rewrite.
+
+A couple relevant quotes from a Postgres developer include:
+
+> No, that's not going to work: coercing to a domain that has any
+> constraints is considered to require a rewrite.
+
+And:
+
+> In any case, the point remains that domains are pretty inefficient
+> compared to native types like varchar(12); partly because the system
+> can’t reason very well about arbitrary check constraints as compared
+> to simple length constraints, and partly because the whole feature
+> just isn’t implemented very completely or efficiently.  So you’ll be
+> paying *a lot* for some hypothetical future savings.
+
+
+## solution
+
+Either avoid domains altogether, or (most importantly) avoid adding constraints to domains. Instead, put the [constraint][]
+on the desired column(s) directly.
+
+[constraint]: https://www.postgresql.org/docs/current/sql-createdomain.html
+
+## links
+
+[The mailing list thread from which the above quotes are sourced](https://www.postgresql.org/message-id/flat/CADVWZZKjhV9fLpewPdQMZx7V6kvGJViwMEDrPAv9m50rGeK9UA%40mail.gmail.com)

docs/docs/ban-create-domain-with-constraint.md

@@ -0,0 +1,41 @@
+---
+id: ban-create-domain-with-constraint
+title: ban-create-domain-with-constraint
+---
+
+## problem
+
+Postgres [domains][], which associate a data type with an optional check constraint, have poor support for online migrations
+when associated with a check constraint.
+
+[domains]: https://www.postgresql.org/docs/current/sql-createdomain.html
+
+The purpose of domains is to make the named type-plus-constraint reusable, but this means that any change to the domain's constraint
+requires _all_ columns that use the domain to be revalidated. And, because Postgres can't reason well about arbitrary constraints,
+they increase the chances of a change requiring an expensive table rewrite.
+
+A couple relevant quotes from a Postgres developer include:
+
+> No, that's not going to work: coercing to a domain that has any
+> constraints is considered to require a rewrite.
+
+And:
+
+> In any case, the point remains that domains are pretty inefficient
+> compared to native types like varchar(12); partly because the system
+> can’t reason very well about arbitrary check constraints as compared
+> to simple length constraints, and partly because the whole feature
+> just isn’t implemented very completely or efficiently.  So you’ll be
+> paying *a lot* for some hypothetical future savings.
+
+
+## solution
+
+Either avoid domains altogether, or (most importantly) avoid adding constraints to domains. Instead, put the [constraint][]
+on the desired column(s) directly.
+
+[constraint]: https://www.postgresql.org/docs/current/sql-createdomain.html
+
+## links
+
+[The mailing list thread from which the above quotes are sourced](https://www.postgresql.org/message-id/flat/CADVWZZKjhV9fLpewPdQMZx7V6kvGJViwMEDrPAv9m50rGeK9UA%40mail.gmail.com)

docs/sidebars.js

@@ -29,6 +29,8 @@ module.exports = {
       "require-concurrent-index-creation",
       "require-concurrent-index-deletion",
       "transaction-nesting",
+      "ban-create-domain-with-constraint",
+      "ban-alter-domain-with-add-constraint",
       // generator::new-rule-above
     ],
   },

docs/src/pages/index.js

@@ -183,6 +183,16 @@ const rules = [
     tags: ["schema"],
     description: "Prevent forbidden use of transactions during concurrent index creation.",
   },
+  {
+    name: "ban-create-domain-with-constraint",
+    tags: ["schema", "locking"],
+    description: "Domains with constraints have poor support for online migrations",
+  },
+  {
+    name: "ban-alter-domain-with-add-constraint",
+    tags: ["schema", "locking"],
+    description: "Domains with constraints have poor support for online migrations",
+  },
   // generator::new-rule-above
 ]
 

linter/src/lib.rs

@@ -9,7 +9,9 @@ extern crate lazy_static;
 
 use crate::errors::CheckSqlError;
 use crate::rules::adding_required_field;
+use crate::rules::ban_alter_domain_with_add_constraint;
 use crate::rules::ban_concurrent_index_creation_in_transaction;
+use crate::rules::ban_create_domain_with_constraint;
 use crate::rules::ban_drop_not_null;
 use crate::rules::prefer_big_int;
 use crate::rules::prefer_identity;
@@ -98,6 +100,15 @@ lazy_static! {
 
         ],
     },
+    SquawkRule {
+        name: RuleViolationKind::BanAlterDomainWithAddConstraint,
+        func: ban_alter_domain_with_add_constraint,
+        messages: vec![
+            ViolationMessage::Note(
+                "Domains with constraints have poor support for online migrations".into()
+            ),
+        ],
+    },
     SquawkRule {
         name: RuleViolationKind::BanCharField,
         func: ban_char_type,
@@ -119,6 +130,15 @@ lazy_static! {
             ),
         ],
     },
+    SquawkRule {
+        name: RuleViolationKind::BanCreateDomainWithConstraint,
+        func: ban_create_domain_with_constraint,
+        messages: vec![
+            ViolationMessage::Note(
+                "Domains with constraints have poor support for online migrations".into()
+            ),
+        ],
+    },
     SquawkRule {
         name: RuleViolationKind::BanDropColumn,
         func: ban_drop_column,

linter/src/rules/ban_alter_domain_with_add_constraint.rs

@@ -0,0 +1,71 @@
+use crate::{
+    versions::Version,
+    violations::{RuleViolation, RuleViolationKind},
+};
+
+use squawk_parser::ast::{RawStmt, Stmt};
+
+#[must_use]
+pub fn ban_alter_domain_with_add_constraint(
+    tree: &[RawStmt],
+    _pg_version: Option<Version>,
+    _assume_in_transaction: bool,
+) -> Vec<RuleViolation> {
+    let mut errs = vec![];
+    for raw_stmt in tree {
+        match &raw_stmt.stmt {
+            Stmt::AlterDomainStmt(stmt) if stmt.subtype == "C" => {
+                errs.push(RuleViolation::new(
+                    RuleViolationKind::BanAlterDomainWithAddConstraint,
+                    raw_stmt.into(),
+                    None,
+                ));
+            }
+            _ => continue,
+        }
+    }
+    errs
+}
+
+#[cfg(test)]
+mod test_rules {
+    use crate::{
+        check_sql_with_rule,
+        violations::{RuleViolation, RuleViolationKind},
+    };
+
+    use insta::assert_debug_snapshot;
+
+    fn lint_sql(sql: &str) -> Vec<RuleViolation> {
+        check_sql_with_rule(
+            sql,
+            &RuleViolationKind::BanAlterDomainWithAddConstraint,
+            None,
+            false,
+        )
+        .unwrap()
+    }
+
+    #[test]
+    fn ban_alter_domain_without_add_constraint_is_ok() {
+        let sql = r"
+     ALTER DOMAIN domain_name_1 SET DEFAULT 1;
+     ALTER DOMAIN domain_name_2 SET NOT NULL;
+     ALTER DOMAIN domain_name_3 DROP CONSTRAINT other_domain_name;
+     ALTER DOMAIN domain_name_4 RENAME CONSTRAINT constraint_name TO other_constraint_name;
+     ALTER DOMAIN domain_name_5 RENAME TO other_domain_name;
+     ALTER DOMAIN domain_name_6 VALIDATE CONSTRAINT constraint_name;
+     ALTER DOMAIN domain_name_7 OWNER TO you;
+     ALTER DOMAIN domain_name_8 SET SCHEMA foo;
+       ";
+        assert_eq!(lint_sql(sql), vec![]);
+    }
+
+    #[test]
+    fn ban_alter_domain_with_add_constraint_works() {
+        let sql = r"
+     ALTER DOMAIN domain_name ADD CONSTRAINT constraint_name CHECK (value > 0);
+       ";
+        assert_debug_snapshot!(lint_sql(sql));
+    }
+}

linter/src/rules/ban_create_domain_with_constraint.rs

@@ -0,0 +1,64 @@
+use crate::{
+    versions::Version,
+    violations::{RuleViolation, RuleViolationKind},
+};
+
+use squawk_parser::ast::{RawStmt, Stmt};
+
+#[must_use]
+pub fn ban_create_domain_with_constraint(
+    tree: &[RawStmt],
+    _pg_version: Option<Version>,
+    _assume_in_transaction: bool,
+) -> Vec<RuleViolation> {
+    let mut errs = vec![];
+    for raw_stmt in tree {
+        match &raw_stmt.stmt {
+            Stmt::CreateDomainStmt(stmt) if !stmt.constraints.is_empty() => {
+                errs.push(RuleViolation::new(
+                    RuleViolationKind::BanCreateDomainWithConstraint,
+                    raw_stmt.into(),
+                    None,
+                ));
+            }
+            _ => continue,
+        }
+    }
+    errs
+}
+
+#[cfg(test)]
+mod test_rules {
+    use crate::{
+        check_sql_with_rule,
+        violations::{RuleViolation, RuleViolationKind},
+    };
+    use insta::assert_debug_snapshot;
+
+    fn lint_sql(sql: &str) -> Vec<RuleViolation> {
+        check_sql_with_rule(
+            sql,
+            &RuleViolationKind::BanCreateDomainWithConstraint,
+            None,
+            false,
+        )
+        .unwrap()
+    }
+
+    #[test]
+    fn ban_create_domain_without_constraint_is_ok() {
+        let sql = r"
+    CREATE DOMAIN domain_name_1 AS TEXT;
+    CREATE DOMAIN domain_name_2 AS CHARACTER VARYING;
+      ";
+        assert_eq!(lint_sql(sql), vec![]);
+    }
+
+    #[test]
+    fn ban_create_domain_with_constraint_works() {
+        let sql = r"
+    CREATE DOMAIN domain_name_3 AS NUMERIC(15,5) CHECK (value > 0);
+      ";
+        assert_debug_snapshot!(lint_sql(sql));
+    }
+}

linter/src/rules/mod.rs

@@ -52,3 +52,7 @@ pub mod adding_required_field;
 pub use adding_required_field::*;
 pub mod ban_concurrent_index_creation_in_transaction;
 pub use ban_concurrent_index_creation_in_transaction::*;
+pub mod ban_create_domain_with_constraint;
+pub use ban_create_domain_with_constraint::*;
+pub mod ban_alter_domain_with_add_constraint;
+pub use ban_alter_domain_with_add_constraint::*;

linter/src/rules/snapshots/squawk_linter__rules__ban_alter_domain_with_add_constraint__test_rules__ban_alter_domain_with_add_constraint_works.snap

@@ -0,0 +1,21 @@
+---
+source: linter/src/rules/ban_alter_domain_with_add_constraint.rs
+expression: lint_sql(sql)
+
+---
+[
+    RuleViolation {
+        kind: BanAlterDomainWithAddConstraint,
+        span: Span {
+            start: 0,
+            len: Some(
+                79,
+            ),
+        },
+        messages: [
+            Note(
+                "Domains with constraints have poor support for online migrations",
+            ),
+        ],
+    },
+]

linter/src/rules/snapshots/squawk_linter__rules__ban_create_domain_with_constraint__test_rules__ban_create_domain_with_constraint_works.snap

@@ -0,0 +1,21 @@
+---
+source: linter/src/rules/ban_create_domain_with_constraint.rs
+expression: lint_sql(sql)
+
+---
+[
+    RuleViolation {
+        kind: BanCreateDomainWithConstraint,
+        span: Span {
+            start: 0,
+            len: Some(
+                67,
+            ),
+        },
+        messages: [
+            Note(
+                "Domains with constraints have poor support for online migrations",
+            ),
+        ],
+    },
+]

linter/src/snapshots/squawk_linter__test_rules__rule_names_debug_snap.snap

@@ -1,15 +1,18 @@
 ---
 source: linter/src/lib.rs
 expression: rule_names
+
 ---
 [
     "adding-field-with-default",
     "adding-foreign-key-constraint",
     "adding-not-nullable-field",
     "adding-required-field",
     "adding-serial-primary-key-field",
+    "ban-alter-domain-with-add-constraint",
     "ban-char-field",
     "ban-concurrent-index-creation-in-transaction",
+    "ban-create-domain-with-constraint",
     "ban-drop-column",
     "ban-drop-database",
     "ban-drop-not-null",

linter/src/snapshots/squawk_linter__test_rules__rule_names_display_snap.snap

@@ -1,14 +1,17 @@
 ---
 source: linter/src/lib.rs
 expression: "rule_names.join(\"\\n\")"
+
 ---
 adding-field-with-default
 adding-foreign-key-constraint
 adding-not-nullable-field
 adding-required-field
 adding-serial-primary-key-field
+ban-alter-domain-with-add-constraint
 ban-char-field
 ban-concurrent-index-creation-in-transaction
+ban-create-domain-with-constraint
 ban-drop-column
 ban-drop-database
 ban-drop-not-null

linter/src/violations.rs

@@ -61,6 +61,10 @@ pub enum RuleViolationKind {
     AddingRequiredField,
     #[serde(rename = "ban-concurrent-index-creation-in-transaction")]
     BanConcurrentIndexCreationInTransaction,
+    #[serde(rename = "ban-create-domain-with-constraint")]
+    BanCreateDomainWithConstraint,
+    #[serde(rename = "ban-alter-domain-with-add-constraint")]
+    BanAlterDomainWithAddConstraint,
     // generator::new-rule-above
 }