diff --git a/docs/docs/ban-alter-domain-with-add-constraint.md b/docs/docs/ban-alter-domain-with-add-constraint.md new file mode 100644 index 00000000..b0866270 --- /dev/null +++ b/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) diff --git a/docs/docs/ban-create-domain-with-constraint.md b/docs/docs/ban-create-domain-with-constraint.md new file mode 100644 index 00000000..74110877 --- /dev/null +++ b/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) diff --git a/docs/sidebars.js b/docs/sidebars.js index 1f5e6a0a..83612aa1 100644 --- a/docs/sidebars.js +++ b/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 ], }, diff --git a/docs/src/pages/index.js b/docs/src/pages/index.js index 81ee46ec..fbd9d750 100644 --- a/docs/src/pages/index.js +++ b/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 ] diff --git a/linter/src/lib.rs b/linter/src/lib.rs index 174edeb2..33db56a7 100644 --- a/linter/src/lib.rs +++ b/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, diff --git a/linter/src/rules/ban_alter_domain_with_add_constraint.rs b/linter/src/rules/ban_alter_domain_with_add_constraint.rs new file mode 100644 index 00000000..8227e94d --- /dev/null +++ b/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, + _assume_in_transaction: bool, +) -> Vec { + 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 { + 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)); + } +} diff --git a/linter/src/rules/ban_create_domain_with_constraint.rs b/linter/src/rules/ban_create_domain_with_constraint.rs new file mode 100644 index 00000000..77b614e2 --- /dev/null +++ b/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, + _assume_in_transaction: bool, +) -> Vec { + 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 { + 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)); + } +} diff --git a/linter/src/rules/mod.rs b/linter/src/rules/mod.rs index e4d11ce7..1dc3fbc9 100644 --- a/linter/src/rules/mod.rs +++ b/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::*; diff --git a/linter/src/rules/snapshots/squawk_linter__rules__ban_alter_domain_with_add_constraint__test_rules__ban_alter_domain_with_add_constraint_works.snap b/linter/src/rules/snapshots/squawk_linter__rules__ban_alter_domain_with_add_constraint__test_rules__ban_alter_domain_with_add_constraint_works.snap new file mode 100644 index 00000000..7adad1c2 --- /dev/null +++ b/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", + ), + ], + }, +] diff --git a/linter/src/rules/snapshots/squawk_linter__rules__ban_create_domain_with_constraint__test_rules__ban_create_domain_with_constraint_works.snap b/linter/src/rules/snapshots/squawk_linter__rules__ban_create_domain_with_constraint__test_rules__ban_create_domain_with_constraint_works.snap new file mode 100644 index 00000000..bb1fd486 --- /dev/null +++ b/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", + ), + ], + }, +] diff --git a/linter/src/snapshots/squawk_linter__test_rules__rule_names_debug_snap.snap b/linter/src/snapshots/squawk_linter__test_rules__rule_names_debug_snap.snap index 83322f6d..ad4cfe31 100644 --- a/linter/src/snapshots/squawk_linter__test_rules__rule_names_debug_snap.snap +++ b/linter/src/snapshots/squawk_linter__test_rules__rule_names_debug_snap.snap @@ -1,6 +1,7 @@ --- source: linter/src/lib.rs expression: rule_names + --- [ "adding-field-with-default", @@ -8,8 +9,10 @@ expression: rule_names "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", diff --git a/linter/src/snapshots/squawk_linter__test_rules__rule_names_display_snap.snap b/linter/src/snapshots/squawk_linter__test_rules__rule_names_display_snap.snap index 12a30380..555d224f 100644 --- a/linter/src/snapshots/squawk_linter__test_rules__rule_names_display_snap.snap +++ b/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 diff --git a/linter/src/violations.rs b/linter/src/violations.rs index 0a753ebd..d20f499e 100644 --- a/linter/src/violations.rs +++ b/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 } diff --git a/parser/src/ast.rs b/parser/src/ast.rs index 66805572..70aa995d 100644 --- a/parser/src/ast.rs +++ b/parser/src/ast.rs @@ -429,6 +429,26 @@ pub struct AlterTableStmt { pub missing_ok: bool, } +#[derive(Debug, Deserialize, Serialize)] +pub struct CreateDomainStmt { + #[serde(rename = "domainname")] + pub domain_name: Vec, + #[serde(rename = "typeName")] + pub typename: Value, + #[serde(default)] + pub constraints: Vec, +} + +#[derive(Debug, Deserialize, Serialize)] +pub struct AlterDomainStmt { + pub behavior: DropBehavior, + pub name: Option, + pub subtype: String, + #[serde(rename = "typeName")] + pub typename: Value, + pub def: Option, +} + /// Source: #[derive(Serialize, Deserialize, PartialEq, Debug)] pub enum DropBehavior { @@ -888,7 +908,7 @@ pub enum Stmt { UpdateStmt(Value), DeleteStmt(Value), CreateSchemaStmt(Value), - AlterDomainStmt(Value), + AlterDomainStmt(AlterDomainStmt), GrantStmt(Value), GrantRoleStmt(Value), AlterDefaultPrivilegesStmt(Value), @@ -928,7 +948,7 @@ pub enum Stmt { CreateSeqStmt(Value), AlterSeqStmt(Value), DefineStmt(Value), - CreateDomainStmt(Value), + CreateDomainStmt(CreateDomainStmt), CreateOpClassStmt(Value), CreateOpFamilyStmt(Value), AlterOpFamilyStmt(Value), diff --git a/parser/src/snapshots/squawk_parser__parse__tests__create_domain_stmt.snap b/parser/src/snapshots/squawk_parser__parse__tests__create_domain_stmt.snap index ff0eba6f..bd69cad7 100644 --- a/parser/src/snapshots/squawk_parser__parse__tests__create_domain_stmt.snap +++ b/parser/src/snapshots/squawk_parser__parse__tests__create_domain_stmt.snap @@ -1,13 +1,38 @@ --- source: parser/src/parse.rs expression: res + --- Ok( [ RawStmt { stmt: CreateDomainStmt( - Object({ - "constraints": Array([ + CreateDomainStmt { + domain_name: [ + QualifiedName { + string: PGString { + sval: "us_postal_code", + }, + }, + ], + typename: Object({ + "location": Number( + 33, + ), + "names": Array([ + Object({ + "String": Object({ + "sval": String( + "text", + ), + }), + }), + ]), + "typemod": Number( + -1, + ), + }), + constraints: [ Object({ "Constraint": Object({ "contype": String( @@ -127,34 +152,8 @@ Ok( }), }), }), - ]), - "domainname": Array([ - Object({ - "String": Object({ - "sval": String( - "us_postal_code", - ), - }), - }), - ]), - "typeName": Object({ - "location": Number( - 33, - ), - "names": Array([ - Object({ - "String": Object({ - "sval": String( - "text", - ), - }), - }), - ]), - "typemod": Number( - -1, - ), - }), - }), + ], + }, ), stmt_location: 0, stmt_len: Some( diff --git a/parser/src/snapshots/squawk_parser__parse__tests__parse_alter_domain_stmt.snap b/parser/src/snapshots/squawk_parser__parse__tests__parse_alter_domain_stmt.snap index eb919edb..426705ea 100644 --- a/parser/src/snapshots/squawk_parser__parse__tests__parse_alter_domain_stmt.snap +++ b/parser/src/snapshots/squawk_parser__parse__tests__parse_alter_domain_stmt.snap @@ -1,19 +1,17 @@ --- source: parser/src/parse.rs expression: res + --- Ok( [ RawStmt { stmt: AlterDomainStmt( - Object({ - "behavior": String( - "DROP_RESTRICT", - ), - "subtype": String( - "O", - ), - "typeName": Array([ + AlterDomainStmt { + behavior: Restrict, + name: None, + subtype: "O", + typename: Array([ Object({ "String": Object({ "sval": String( @@ -22,7 +20,8 @@ Ok( }), }), ]), - }), + def: None, + }, ), stmt_location: 0, stmt_len: Some(