diff --git a/crates/squawk_linter/src/lib.rs b/crates/squawk_linter/src/lib.rs index 9012f5fc..f321bc94 100644 --- a/crates/squawk_linter/src/lib.rs +++ b/crates/squawk_linter/src/lib.rs @@ -54,6 +54,7 @@ use rules::renaming_column; use rules::renaming_table; use rules::require_concurrent_index_creation; use rules::require_concurrent_index_deletion; +use rules::require_concurrent_partition_detach; use rules::require_enum_value_ordering; use rules::require_table_schema; use rules::require_timeout_settings; @@ -96,6 +97,7 @@ pub enum Rule { RequireEnumValueOrdering, RequireTableSchema, IdentifierTooLong, + RequireConcurrentPartitionDetach, // xtask:new-rule:error-name } @@ -149,6 +151,7 @@ impl TryFrom<&str> for Rule { "require-enum-value-ordering" => Ok(Rule::RequireEnumValueOrdering), "require-table-schema" => Ok(Rule::RequireTableSchema), "identifier-too-long" => Ok(Rule::IdentifierTooLong), + "require-concurrent-partition-detach" => Ok(Rule::RequireConcurrentPartitionDetach), // xtask:new-rule:str-name _ => Err(format!("Unknown violation name: {s}")), } @@ -214,6 +217,7 @@ impl fmt::Display for Rule { Rule::RequireEnumValueOrdering => "require-enum-value-ordering", Rule::RequireTableSchema => "require-table-schema", Rule::IdentifierTooLong => "identifier-too-long", + Rule::RequireConcurrentPartitionDetach => "require-concurrent-partition-detach", // xtask:new-rule:variant-to-name }; write!(f, "{val}") @@ -450,6 +454,9 @@ impl Linter { if self.rules.contains(&Rule::IdentifierTooLong) { identifier_too_long(self, file); } + if self.rules.contains(&Rule::RequireConcurrentPartitionDetach) { + require_concurrent_partition_detach(self, file); + } // xtask:new-rule:rule-call // locate any ignores in the file diff --git a/crates/squawk_linter/src/rules/mod.rs b/crates/squawk_linter/src/rules/mod.rs index b4550e50..c563d28a 100644 --- a/crates/squawk_linter/src/rules/mod.rs +++ b/crates/squawk_linter/src/rules/mod.rs @@ -27,6 +27,7 @@ pub(crate) mod renaming_column; pub(crate) mod renaming_table; pub(crate) mod require_concurrent_index_creation; pub(crate) mod require_concurrent_index_deletion; +pub(crate) mod require_concurrent_partition_detach; pub(crate) mod require_enum_value_ordering; pub(crate) mod require_table_schema; pub(crate) mod require_timeout_settings; @@ -62,6 +63,7 @@ pub(crate) use renaming_column::renaming_column; pub(crate) use renaming_table::renaming_table; pub(crate) use require_concurrent_index_creation::require_concurrent_index_creation; pub(crate) use require_concurrent_index_deletion::require_concurrent_index_deletion; +pub(crate) use require_concurrent_partition_detach::require_concurrent_partition_detach; pub(crate) use require_enum_value_ordering::require_enum_value_ordering; pub(crate) use require_table_schema::require_table_schema; pub(crate) use require_timeout_settings::require_timeout_settings; diff --git a/crates/squawk_linter/src/rules/require_concurrent_partition_detach.rs b/crates/squawk_linter/src/rules/require_concurrent_partition_detach.rs new file mode 100644 index 00000000..08fa3472 --- /dev/null +++ b/crates/squawk_linter/src/rules/require_concurrent_partition_detach.rs @@ -0,0 +1,87 @@ +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +use crate::{Edit, Fix, Linter, Rule, Violation}; + +fn concurrently_fix(detach_partition: &ast::DetachPartition) -> Option { + let path = detach_partition.path()?; + let at = path.syntax().text_range().end(); + let edit = Edit::insert(" concurrently", at); + Some(Fix::new("Add `concurrently`", vec![edit])) +} + +pub(crate) fn require_concurrent_partition_detach(ctx: &mut Linter, parse: &Parse) { + for stmt in parse.tree().stmts() { + if let ast::Stmt::AlterTable(alter_table) = stmt { + for action in alter_table.actions() { + if let ast::AlterTableAction::DetachPartition(detach_partition) = action { + if detach_partition.concurrently_token().is_none() + && detach_partition.finalize_token().is_none() + { + let fix = concurrently_fix(&detach_partition); + ctx.report( + Violation::for_node( + Rule::RequireConcurrentPartitionDetach, + "Detaching a partition requires an `ACCESS EXCLUSIVE` lock, which prevents reads and writes to the table.".into(), + detach_partition.syntax(), + ) + .help("Detach the partition `CONCURRENTLY`.") + .fix(fix), + ); + } + } + } + } + } +} + +#[cfg(test)] +mod test { + use insta::assert_snapshot; + + use crate::{ + Rule, + test_utils::{fix_sql, lint_errors, lint_ok}, + }; + + fn fix(sql: &str) -> String { + fix_sql(sql, Rule::RequireConcurrentPartitionDetach) + } + + #[test] + fn detach_partition_missing_concurrently_err() { + let sql = "ALTER TABLE t DETACH PARTITION p;"; + assert_snapshot!(lint_errors(sql, Rule::RequireConcurrentPartitionDetach), @" + warning[require-concurrent-partition-detach]: Detaching a partition requires an `ACCESS EXCLUSIVE` lock, which prevents reads and writes to the table. + ╭▸ + 1 │ ALTER TABLE t DETACH PARTITION p; + │ ━━━━━━━━━━━━━━━━━━ + │ + ├ help: Detach the partition `CONCURRENTLY`. + ╭╴ + 1 │ ALTER TABLE t DETACH PARTITION p concurrently; + ╰╴ ++++++++++++ + "); + } + + #[test] + fn detach_partition_concurrently_ok() { + let sql = "ALTER TABLE t DETACH PARTITION p CONCURRENTLY;"; + lint_ok(sql, Rule::RequireConcurrentPartitionDetach); + } + + #[test] + fn detach_partition_finalize_ok() { + let sql = "ALTER TABLE t DETACH PARTITION p FINALIZE;"; + lint_ok(sql, Rule::RequireConcurrentPartitionDetach); + } + + #[test] + fn fix_add_concurrently() { + let sql = "ALTER TABLE t DETACH PARTITION p;"; + let result = fix(sql); + assert_snapshot!(result, @"ALTER TABLE t DETACH PARTITION p concurrently;"); + } +} diff --git a/crates/xtask/src/new_rule.rs b/crates/xtask/src/new_rule.rs index c64c20fa..d5b6e373 100644 --- a/crates/xtask/src/new_rule.rs +++ b/crates/xtask/src/new_rule.rs @@ -17,8 +17,7 @@ use squawk_syntax::{{ use crate::{{Linter, Violation, Rule}}; pub(crate) fn {rule_name_snake}(ctx: &mut Linter, parse: &Parse) {{ - let file = parse.tree(); - for stmt in file.stmts() {{ + for stmt in parse.tree().stmts() {{ match stmt {{ // TODO: update to the stmt you want to check ast::Stmt::CreateTable(create_table) => {{ diff --git a/docs/docs/require-concurrent-partition-detach.md b/docs/docs/require-concurrent-partition-detach.md new file mode 100644 index 00000000..e536bdcd --- /dev/null +++ b/docs/docs/require-concurrent-partition-detach.md @@ -0,0 +1,31 @@ +--- +id: require-concurrent-partition-detach +title: require-concurrent-partition-detach +--- + +## problem + +Detaching a partition via `ALTER TABLE t DETACH PARTITION y;` requires an `ACCESS EXCLUSIVE` lock, which prevents reads and writes to the table. + +## solution + +Ensure all partition detaches use the `CONCURRENTLY` option. + +Instead of: + +```sql +-- blocks reads and writes +ALTER TABLE t DETACH PARTITION p; +``` + +Use: + +```sql +-- allows reads and writes while detaching +ALTER TABLE t DETACH PARTITION p CONCURRENTLY; +``` + +## links + +- +- diff --git a/docs/sidebars.js b/docs/sidebars.js index df11b267..2f498cda 100644 --- a/docs/sidebars.js +++ b/docs/sidebars.js @@ -37,6 +37,7 @@ module.exports = { "require-enum-value-ordering", "require-table-schema", "identifier-too-long", + "require-concurrent-partition-detach", // xtask:new-rule:error-name ], }, diff --git a/docs/src/pages/index.js b/docs/src/pages/index.js index 344bdbcd..b0004b24 100644 --- a/docs/src/pages/index.js +++ b/docs/src/pages/index.js @@ -215,7 +215,12 @@ const rules = [ { name: "identifier-too-long", tags: ["schema"], - description: "Warn about implicit truncation for identifiers that are too long.", + description: "Prevent implicit truncation for identifiers that are too long.", + }, + { + name: "require-concurrent-partition-detach", + tags: ["schema", "locking"], + description: "Prevent blocking reads/writes to table while partition is detached.", }, // xtask:new-rule:rule-doc-meta ]