From 69c04c029441d53283ce27f7ebe135ce0532afcd Mon Sep 17 00:00:00 2001 From: John Mastro Date: Wed, 26 Mar 2025 16:20:21 -0400 Subject: [PATCH 1/4] ban creating domains with constraints --- .../docs/ban-create-domain-with-constraint.md | 16 +++++ docs/sidebars.js | 1 + docs/src/pages/index.js | 5 ++ linter/src/lib.rs | 10 +++ .../ban_create_domain_with_constraint.rs | 64 +++++++++++++++++++ linter/src/rules/mod.rs | 2 + ...n_create_domain_with_constraint_works.snap | 21 ++++++ ...er__test_rules__rule_names_debug_snap.snap | 2 + ...__test_rules__rule_names_display_snap.snap | 2 + linter/src/violations.rs | 2 + parser/src/ast.rs | 12 +++- ...ser__parse__tests__create_domain_stmt.snap | 59 +++++++++-------- 12 files changed, 165 insertions(+), 31 deletions(-) create mode 100644 docs/docs/ban-create-domain-with-constraint.md create mode 100644 linter/src/rules/ban_create_domain_with_constraint.rs create mode 100644 linter/src/rules/snapshots/squawk_linter__rules__ban_create_domain_with_constraint__test_rules__ban_create_domain_with_constraint_works.snap 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..41f293cf --- /dev/null +++ b/docs/docs/ban-create-domain-with-constraint.md @@ -0,0 +1,16 @@ +--- +id: ban-create-domain-with-constraint +title: ban-create-domain-with-constraint +--- + +## problem + + + +## solution + + + +## links + + \ No newline at end of file diff --git a/docs/sidebars.js b/docs/sidebars.js index 1f5e6a0a..81b1db1f 100644 --- a/docs/sidebars.js +++ b/docs/sidebars.js @@ -29,6 +29,7 @@ module.exports = { "require-concurrent-index-creation", "require-concurrent-index-deletion", "transaction-nesting", + "ban-create-domain-with-constraint", // generator::new-rule-above ], }, diff --git a/docs/src/pages/index.js b/docs/src/pages/index.js index 81ee46ec..1bada8ca 100644 --- a/docs/src/pages/index.js +++ b/docs/src/pages/index.js @@ -183,6 +183,11 @@ const rules = [ tags: ["schema"], description: "Prevent forbidden use of transactions during concurrent index creation.", }, + { + name: "ban-create-domain-with-constraint", + tags: ["TODO"], + description: "TODO", + }, // generator::new-rule-above ] diff --git a/linter/src/lib.rs b/linter/src/lib.rs index 174edeb2..60b20062 100644 --- a/linter/src/lib.rs +++ b/linter/src/lib.rs @@ -10,6 +10,7 @@ extern crate lazy_static; use crate::errors::CheckSqlError; use crate::rules::adding_required_field; 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; @@ -119,6 +120,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_create_domain_with_constraint.rs b/linter/src/rules/ban_create_domain_with_constraint.rs new file mode 100644 index 00000000..1d73b192 --- /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.len() > 0 => { + 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..d1a756ab 100644 --- a/linter/src/rules/mod.rs +++ b/linter/src/rules/mod.rs @@ -52,3 +52,5 @@ 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::*; \ No newline at end of file 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..ba050f46 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", @@ -10,6 +11,7 @@ expression: rule_names "adding-serial-primary-key-field", "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..8b74ab82 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,6 +1,7 @@ --- source: linter/src/lib.rs expression: "rule_names.join(\"\\n\")" + --- adding-field-with-default adding-foreign-key-constraint @@ -9,6 +10,7 @@ adding-required-field adding-serial-primary-key-field 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..4661442c 100644 --- a/linter/src/violations.rs +++ b/linter/src/violations.rs @@ -61,6 +61,8 @@ pub enum RuleViolationKind { AddingRequiredField, #[serde(rename = "ban-concurrent-index-creation-in-transaction")] BanConcurrentIndexCreationInTransaction, + #[serde(rename = "ban-create-domain-with-constraint")] + BanCreateDomainWithConstraint, // generator::new-rule-above } diff --git a/parser/src/ast.rs b/parser/src/ast.rs index 66805572..ebb5337d 100644 --- a/parser/src/ast.rs +++ b/parser/src/ast.rs @@ -429,6 +429,16 @@ 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, +} + /// Source: #[derive(Serialize, Deserialize, PartialEq, Debug)] pub enum DropBehavior { @@ -928,7 +938,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( From 7b502f0fd814de288474f0f7b9612b8167c563f0 Mon Sep 17 00:00:00 2001 From: John Mastro Date: Mon, 31 Mar 2025 16:35:14 -0400 Subject: [PATCH 2/4] ban adding constraints to existing domains --- .../ban-alter-domain-with-add-constraint.md | 16 +++++ docs/sidebars.js | 1 + docs/src/pages/index.js | 5 ++ linter/src/lib.rs | 10 +++ .../ban_alter_domain_with_add_constraint.rs | 71 +++++++++++++++++++ linter/src/rules/mod.rs | 3 +- ...lter_domain_with_add_constraint_works.snap | 21 ++++++ ...er__test_rules__rule_names_debug_snap.snap | 1 + ...__test_rules__rule_names_display_snap.snap | 1 + linter/src/violations.rs | 2 + parser/src/ast.rs | 12 +++- ...parse__tests__parse_alter_domain_stmt.snap | 17 +++-- 12 files changed, 149 insertions(+), 11 deletions(-) create mode 100644 docs/docs/ban-alter-domain-with-add-constraint.md create mode 100644 linter/src/rules/ban_alter_domain_with_add_constraint.rs create mode 100644 linter/src/rules/snapshots/squawk_linter__rules__ban_alter_domain_with_add_constraint__test_rules__ban_alter_domain_with_add_constraint_works.snap 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..35c40113 --- /dev/null +++ b/docs/docs/ban-alter-domain-with-add-constraint.md @@ -0,0 +1,16 @@ +--- +id: ban-alter-domain-with-add-constraint +title: ban-alter-domain-with-add-constraint +--- + +## problem + + + +## solution + + + +## links + + \ No newline at end of file diff --git a/docs/sidebars.js b/docs/sidebars.js index 81b1db1f..83612aa1 100644 --- a/docs/sidebars.js +++ b/docs/sidebars.js @@ -30,6 +30,7 @@ module.exports = { "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 1bada8ca..cff53b14 100644 --- a/docs/src/pages/index.js +++ b/docs/src/pages/index.js @@ -188,6 +188,11 @@ const rules = [ tags: ["TODO"], description: "TODO", }, + { + name: "ban-alter-domain-with-add-constraint", + tags: ["TODO"], + description: "TODO", + }, // generator::new-rule-above ] diff --git a/linter/src/lib.rs b/linter/src/lib.rs index 60b20062..33db56a7 100644 --- a/linter/src/lib.rs +++ b/linter/src/lib.rs @@ -9,6 +9,7 @@ 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; @@ -99,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, 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..e32d27b5 --- /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/mod.rs b/linter/src/rules/mod.rs index d1a756ab..9ee64c58 100644 --- a/linter/src/rules/mod.rs +++ b/linter/src/rules/mod.rs @@ -53,4 +53,5 @@ 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::*; \ No newline at end of file +pub use ban_create_domain_with_constraint::*;pub mod ban_alter_domain_with_add_constraint; +pub use ban_alter_domain_with_add_constraint::*; \ No newline at end of file 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/snapshots/squawk_linter__test_rules__rule_names_debug_snap.snap b/linter/src/snapshots/squawk_linter__test_rules__rule_names_debug_snap.snap index ba050f46..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 @@ -9,6 +9,7 @@ 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", 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 8b74ab82..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 @@ -8,6 +8,7 @@ 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 diff --git a/linter/src/violations.rs b/linter/src/violations.rs index 4661442c..d20f499e 100644 --- a/linter/src/violations.rs +++ b/linter/src/violations.rs @@ -63,6 +63,8 @@ pub enum RuleViolationKind { 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 ebb5337d..70aa995d 100644 --- a/parser/src/ast.rs +++ b/parser/src/ast.rs @@ -439,6 +439,16 @@ pub struct CreateDomainStmt { 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 { @@ -898,7 +908,7 @@ pub enum Stmt { UpdateStmt(Value), DeleteStmt(Value), CreateSchemaStmt(Value), - AlterDomainStmt(Value), + AlterDomainStmt(AlterDomainStmt), GrantStmt(Value), GrantRoleStmt(Value), AlterDefaultPrivilegesStmt(Value), 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( From 816d21ec8b8628976d871d2ea2c7990b9e5c549d Mon Sep 17 00:00:00 2001 From: John Mastro Date: Mon, 31 Mar 2025 16:53:27 -0400 Subject: [PATCH 3/4] docs --- .../ban-alter-domain-with-add-constraint.md | 31 +++++++++++++++++-- .../docs/ban-create-domain-with-constraint.md | 31 +++++++++++++++++-- docs/src/pages/index.js | 8 ++--- 3 files changed, 60 insertions(+), 10 deletions(-) diff --git a/docs/docs/ban-alter-domain-with-add-constraint.md b/docs/docs/ban-alter-domain-with-add-constraint.md index 35c40113..b0866270 100644 --- a/docs/docs/ban-alter-domain-with-add-constraint.md +++ b/docs/docs/ban-alter-domain-with-add-constraint.md @@ -5,12 +5,37 @@ 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 - \ No newline at end of file +[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 index 41f293cf..74110877 100644 --- a/docs/docs/ban-create-domain-with-constraint.md +++ b/docs/docs/ban-create-domain-with-constraint.md @@ -5,12 +5,37 @@ 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 - \ No newline at end of file +[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/src/pages/index.js b/docs/src/pages/index.js index cff53b14..fbd9d750 100644 --- a/docs/src/pages/index.js +++ b/docs/src/pages/index.js @@ -185,13 +185,13 @@ const rules = [ }, { name: "ban-create-domain-with-constraint", - tags: ["TODO"], - description: "TODO", + tags: ["schema", "locking"], + description: "Domains with constraints have poor support for online migrations", }, { name: "ban-alter-domain-with-add-constraint", - tags: ["TODO"], - description: "TODO", + tags: ["schema", "locking"], + description: "Domains with constraints have poor support for online migrations", }, // generator::new-rule-above ] From 93c984b9a99868ef5b8e852069c0851fc91d6c6a Mon Sep 17 00:00:00 2001 From: John Mastro Date: Tue, 1 Apr 2025 12:52:08 -0400 Subject: [PATCH 4/4] fix linter errors --- .../src/rules/ban_alter_domain_with_add_constraint.rs | 8 ++++---- linter/src/rules/ban_create_domain_with_constraint.rs | 10 +++++----- linter/src/rules/mod.rs | 5 +++-- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/linter/src/rules/ban_alter_domain_with_add_constraint.rs b/linter/src/rules/ban_alter_domain_with_add_constraint.rs index e32d27b5..8227e94d 100644 --- a/linter/src/rules/ban_alter_domain_with_add_constraint.rs +++ b/linter/src/rules/ban_alter_domain_with_add_constraint.rs @@ -48,7 +48,7 @@ mod test_rules { #[test] fn ban_alter_domain_without_add_constraint_is_ok() { - let sql = r#" + 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; @@ -57,15 +57,15 @@ mod test_rules { 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#" + 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 index 1d73b192..77b614e2 100644 --- a/linter/src/rules/ban_create_domain_with_constraint.rs +++ b/linter/src/rules/ban_create_domain_with_constraint.rs @@ -14,7 +14,7 @@ pub fn ban_create_domain_with_constraint( let mut errs = vec![]; for raw_stmt in tree { match &raw_stmt.stmt { - Stmt::CreateDomainStmt(stmt) if stmt.constraints.len() > 0 => { + Stmt::CreateDomainStmt(stmt) if !stmt.constraints.is_empty() => { errs.push(RuleViolation::new( RuleViolationKind::BanCreateDomainWithConstraint, raw_stmt.into(), @@ -47,18 +47,18 @@ mod test_rules { #[test] fn ban_create_domain_without_constraint_is_ok() { - let sql = r#" + 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#" + 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 9ee64c58..1dc3fbc9 100644 --- a/linter/src/rules/mod.rs +++ b/linter/src/rules/mod.rs @@ -53,5 +53,6 @@ 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::*; \ No newline at end of file +pub use ban_create_domain_with_constraint::*; +pub mod ban_alter_domain_with_add_constraint; +pub use ban_alter_domain_with_add_constraint::*;