Skip to content

Commit ca0b55b

Browse files
johnmastrojohnmastro
authored
add rules to ban domains with constraints (#418)
Ban creating domains with constraints (`CREATE DOMAIN ... CHECK`) or adding constraints to existing domains (`ALTER DOMAIN ... ADD CONSTRAINT`). Domains with constraints are hard to use/operationalize well with online migrations, have performance problems, and are soft-discourage by Postgres developers as having unfavorable tradeoffs.
1 parent 04884b4 commit ca0b55b

16 files changed

+364
-41
lines changed

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

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
---
2+
id: ban-alter-domain-with-add-constraint
3+
title: ban-alter-domain-with-add-constraint
4+
---
5+
6+
## problem
7+
8+
Postgres [domains][], which associate a data type with an optional check constraint, have poor support for online migrations
9+
when associated with a check constraint.
10+
11+
[domains]: https://www.postgresql.org/docs/current/sql-createdomain.html
12+
13+
The purpose of domains is to make the named type-plus-constraint reusable, but this means that any change to the domain's constraint
14+
requires _all_ columns that use the domain to be revalidated. And, because Postgres can't reason well about arbitrary constraints,
15+
they increase the chances of a change requiring an expensive table rewrite.
16+
17+
A couple relevant quotes from a Postgres developer include:
18+
19+
> No, that's not going to work: coercing to a domain that has any
20+
> constraints is considered to require a rewrite.
21+
22+
And:
23+
24+
> In any case, the point remains that domains are pretty inefficient
25+
> compared to native types like varchar(12); partly because the system
26+
> can’t reason very well about arbitrary check constraints as compared
27+
> to simple length constraints, and partly because the whole feature
28+
> just isn’t implemented very completely or efficiently. So you’ll be
29+
> paying *a lot* for some hypothetical future savings.
30+
31+
32+
## solution
33+
34+
Either avoid domains altogether, or (most importantly) avoid adding constraints to domains. Instead, put the [constraint][]
35+
on the desired column(s) directly.
36+
37+
[constraint]: https://www.postgresql.org/docs/current/sql-createdomain.html
38+
39+
## links
40+
41+
[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‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
---
2+
id: ban-create-domain-with-constraint
3+
title: ban-create-domain-with-constraint
4+
---
5+
6+
## problem
7+
8+
Postgres [domains][], which associate a data type with an optional check constraint, have poor support for online migrations
9+
when associated with a check constraint.
10+
11+
[domains]: https://www.postgresql.org/docs/current/sql-createdomain.html
12+
13+
The purpose of domains is to make the named type-plus-constraint reusable, but this means that any change to the domain's constraint
14+
requires _all_ columns that use the domain to be revalidated. And, because Postgres can't reason well about arbitrary constraints,
15+
they increase the chances of a change requiring an expensive table rewrite.
16+
17+
A couple relevant quotes from a Postgres developer include:
18+
19+
> No, that's not going to work: coercing to a domain that has any
20+
> constraints is considered to require a rewrite.
21+
22+
And:
23+
24+
> In any case, the point remains that domains are pretty inefficient
25+
> compared to native types like varchar(12); partly because the system
26+
> can’t reason very well about arbitrary check constraints as compared
27+
> to simple length constraints, and partly because the whole feature
28+
> just isn’t implemented very completely or efficiently. So you’ll be
29+
> paying *a lot* for some hypothetical future savings.
30+
31+
32+
## solution
33+
34+
Either avoid domains altogether, or (most importantly) avoid adding constraints to domains. Instead, put the [constraint][]
35+
on the desired column(s) directly.
36+
37+
[constraint]: https://www.postgresql.org/docs/current/sql-createdomain.html
38+
39+
## links
40+
41+
[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‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ module.exports = {
2929
"require-concurrent-index-creation",
3030
"require-concurrent-index-deletion",
3131
"transaction-nesting",
32+
"ban-create-domain-with-constraint",
33+
"ban-alter-domain-with-add-constraint",
3234
// generator::new-rule-above
3335
],
3436
},

‎docs/src/pages/index.js‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,16 @@ const rules = [
183183
tags: ["schema"],
184184
description: "Prevent forbidden use of transactions during concurrent index creation.",
185185
},
186+
{
187+
name: "ban-create-domain-with-constraint",
188+
tags: ["schema", "locking"],
189+
description: "Domains with constraints have poor support for online migrations",
190+
},
191+
{
192+
name: "ban-alter-domain-with-add-constraint",
193+
tags: ["schema", "locking"],
194+
description: "Domains with constraints have poor support for online migrations",
195+
},
186196
// generator::new-rule-above
187197
]
188198

‎linter/src/lib.rs‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,9 @@ extern crate lazy_static;
99

1010
use crate::errors::CheckSqlError;
1111
use crate::rules::adding_required_field;
12+
use crate::rules::ban_alter_domain_with_add_constraint;
1213
use crate::rules::ban_concurrent_index_creation_in_transaction;
14+
use crate::rules::ban_create_domain_with_constraint;
1315
use crate::rules::ban_drop_not_null;
1416
use crate::rules::prefer_big_int;
1517
use crate::rules::prefer_identity;
@@ -98,6 +100,15 @@ lazy_static! {
98100

99101
],
100102
},
103+
SquawkRule {
104+
name: RuleViolationKind::BanAlterDomainWithAddConstraint,
105+
func: ban_alter_domain_with_add_constraint,
106+
messages: vec![
107+
ViolationMessage::Note(
108+
"Domains with constraints have poor support for online migrations".into()
109+
),
110+
],
111+
},
101112
SquawkRule {
102113
name: RuleViolationKind::BanCharField,
103114
func: ban_char_type,
@@ -119,6 +130,15 @@ lazy_static! {
119130
),
120131
],
121132
},
133+
SquawkRule {
134+
name: RuleViolationKind::BanCreateDomainWithConstraint,
135+
func: ban_create_domain_with_constraint,
136+
messages: vec![
137+
ViolationMessage::Note(
138+
"Domains with constraints have poor support for online migrations".into()
139+
),
140+
],
141+
},
122142
SquawkRule {
123143
name: RuleViolationKind::BanDropColumn,
124144
func: ban_drop_column,

‎linter/src/rules/ban_alter_domain_with_add_constraint.rs‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
use crate::{
2+
versions::Version,
3+
violations::{RuleViolation, RuleViolationKind},
4+
};
5+
6+
use squawk_parser::ast::{RawStmt, Stmt};
7+
8+
#[must_use]
9+
pub fn ban_alter_domain_with_add_constraint(
10+
tree: &[RawStmt],
11+
_pg_version: Option<Version>,
12+
_assume_in_transaction: bool,
13+
) -> Vec<RuleViolation> {
14+
let mut errs = vec![];
15+
for raw_stmt in tree {
16+
match &raw_stmt.stmt {
17+
Stmt::AlterDomainStmt(stmt) if stmt.subtype == "C" => {
18+
errs.push(RuleViolation::new(
19+
RuleViolationKind::BanAlterDomainWithAddConstraint,
20+
raw_stmt.into(),
21+
None,
22+
));
23+
}
24+
_ => continue,
25+
}
26+
}
27+
errs
28+
}
29+
30+
#[cfg(test)]
31+
mod test_rules {
32+
use crate::{
33+
check_sql_with_rule,
34+
violations::{RuleViolation, RuleViolationKind},
35+
};
36+
37+
use insta::assert_debug_snapshot;
38+
39+
fn lint_sql(sql: &str) -> Vec<RuleViolation> {
40+
check_sql_with_rule(
41+
sql,
42+
&RuleViolationKind::BanAlterDomainWithAddConstraint,
43+
None,
44+
false,
45+
)
46+
.unwrap()
47+
}
48+
49+
#[test]
50+
fn ban_alter_domain_without_add_constraint_is_ok() {
51+
let sql = r"
52+
ALTER DOMAIN domain_name_1 SET DEFAULT 1;
53+
ALTER DOMAIN domain_name_2 SET NOT NULL;
54+
ALTER DOMAIN domain_name_3 DROP CONSTRAINT other_domain_name;
55+
ALTER DOMAIN domain_name_4 RENAME CONSTRAINT constraint_name TO other_constraint_name;
56+
ALTER DOMAIN domain_name_5 RENAME TO other_domain_name;
57+
ALTER DOMAIN domain_name_6 VALIDATE CONSTRAINT constraint_name;
58+
ALTER DOMAIN domain_name_7 OWNER TO you;
59+
ALTER DOMAIN domain_name_8 SET SCHEMA foo;
60+
";
61+
assert_eq!(lint_sql(sql), vec![]);
62+
}
63+
64+
#[test]
65+
fn ban_alter_domain_with_add_constraint_works() {
66+
let sql = r"
67+
ALTER DOMAIN domain_name ADD CONSTRAINT constraint_name CHECK (value > 0);
68+
";
69+
assert_debug_snapshot!(lint_sql(sql));
70+
}
71+
}

‎linter/src/rules/ban_create_domain_with_constraint.rs‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
use crate::{
2+
versions::Version,
3+
violations::{RuleViolation, RuleViolationKind},
4+
};
5+
6+
use squawk_parser::ast::{RawStmt, Stmt};
7+
8+
#[must_use]
9+
pub fn ban_create_domain_with_constraint(
10+
tree: &[RawStmt],
11+
_pg_version: Option<Version>,
12+
_assume_in_transaction: bool,
13+
) -> Vec<RuleViolation> {
14+
let mut errs = vec![];
15+
for raw_stmt in tree {
16+
match &raw_stmt.stmt {
17+
Stmt::CreateDomainStmt(stmt) if !stmt.constraints.is_empty() => {
18+
errs.push(RuleViolation::new(
19+
RuleViolationKind::BanCreateDomainWithConstraint,
20+
raw_stmt.into(),
21+
None,
22+
));
23+
}
24+
_ => continue,
25+
}
26+
}
27+
errs
28+
}
29+
30+
#[cfg(test)]
31+
mod test_rules {
32+
use crate::{
33+
check_sql_with_rule,
34+
violations::{RuleViolation, RuleViolationKind},
35+
};
36+
use insta::assert_debug_snapshot;
37+
38+
fn lint_sql(sql: &str) -> Vec<RuleViolation> {
39+
check_sql_with_rule(
40+
sql,
41+
&RuleViolationKind::BanCreateDomainWithConstraint,
42+
None,
43+
false,
44+
)
45+
.unwrap()
46+
}
47+
48+
#[test]
49+
fn ban_create_domain_without_constraint_is_ok() {
50+
let sql = r"
51+
CREATE DOMAIN domain_name_1 AS TEXT;
52+
CREATE DOMAIN domain_name_2 AS CHARACTER VARYING;
53+
";
54+
assert_eq!(lint_sql(sql), vec![]);
55+
}
56+
57+
#[test]
58+
fn ban_create_domain_with_constraint_works() {
59+
let sql = r"
60+
CREATE DOMAIN domain_name_3 AS NUMERIC(15,5) CHECK (value > 0);
61+
";
62+
assert_debug_snapshot!(lint_sql(sql));
63+
}
64+
}

‎linter/src/rules/mod.rs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,3 +52,7 @@ pub mod adding_required_field;
5252
pub use adding_required_field::*;
5353
pub mod ban_concurrent_index_creation_in_transaction;
5454
pub use ban_concurrent_index_creation_in_transaction::*;
55+
pub mod ban_create_domain_with_constraint;
56+
pub use ban_create_domain_with_constraint::*;
57+
pub mod ban_alter_domain_with_add_constraint;
58+
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‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
---
2+
source: linter/src/rules/ban_alter_domain_with_add_constraint.rs
3+
expression: lint_sql(sql)
4+
5+
---
6+
[
7+
RuleViolation {
8+
kind: BanAlterDomainWithAddConstraint,
9+
span: Span {
10+
start: 0,
11+
len: Some(
12+
79,
13+
),
14+
},
15+
messages: [
16+
Note(
17+
"Domains with constraints have poor support for online migrations",
18+
),
19+
],
20+
},
21+
]

‎linter/src/rules/snapshots/squawk_linter__rules__ban_create_domain_with_constraint__test_rules__ban_create_domain_with_constraint_works.snap‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
---
2+
source: linter/src/rules/ban_create_domain_with_constraint.rs
3+
expression: lint_sql(sql)
4+
5+
---
6+
[
7+
RuleViolation {
8+
kind: BanCreateDomainWithConstraint,
9+
span: Span {
10+
start: 0,
11+
len: Some(
12+
67,
13+
),
14+
},
15+
messages: [
16+
Note(
17+
"Domains with constraints have poor support for online migrations",
18+
),
19+
],
20+
},
21+
]

0 commit comments

Comments
 (0)