Skip to content

Commit 47684c6

Browse files
psteinroepsteinroe
committed
feat(analyser): add serial column
1 parent f113ec7 commit 47684c6

File tree

26 files changed

+548
-63
lines changed

26 files changed

+548
-63
lines changed

‎.gitignore‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,9 @@ node_modules/
2626
.claude-session-id
2727

2828
squawk/
29+
eugene/
2930

3031
# Auto generated treesitter files
3132
crates/pgt_treesitter_grammar/src/grammar.json
3233
crates/pgt_treesitter_grammar/src/node-types.json
33-
crates/pgt_treesitter_grammar/src/parser.c
34+
crates/pgt_treesitter_grammar/src/parser.c

‎crates/pgt_analyser/CONTRIBUTING.md‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -309,6 +309,58 @@ You should:
309309

310310
If you run the test, you'll see any diagnostics your rule created in your console.
311311

312+
#### Snapshot Tests
313+
314+
After implementing your rule, you must create snapshot tests in the `tests/specs/<group>/<ruleName>/` directory.
315+
316+
Each test file should:
317+
1. Start with a comment describing what the test is checking
318+
2. Include either `-- expect_lint/<group>/<ruleName>` or `-- expect_no_diagnostics` on the first line
319+
3. Contain valid SQL that should either trigger or not trigger your rule
320+
321+
**Example test structure for `addSerialColumn` rule:**
322+
323+
```
324+
tests/specs/safety/addSerialColumn/
325+
├── basic.sql # Basic case that triggers the rule
326+
├── basic.sql.snap # Auto-generated snapshot
327+
├── bigserial.sql # Test bigserial type
328+
├── bigserial.sql.snap
329+
├── generated_stored.sql # Test GENERATED ... STORED
330+
├── generated_stored.sql.snap
331+
├── valid_regular_column.sql # Valid case - should NOT trigger
332+
└── valid_regular_column.sql.snap
333+
```
334+
335+
**Invalid test example (should trigger diagnostic):**
336+
```sql
337+
-- expect_lint/safety/addSerialColumn
338+
-- Test adding serial column to existing table
339+
ALTER TABLE prices ADD COLUMN id serial;
340+
```
341+
342+
**Valid test example (should NOT trigger diagnostic):**
343+
```sql
344+
-- Test adding regular column (should be safe)
345+
-- expect_no_diagnostics
346+
ALTER TABLE prices ADD COLUMN name text;
347+
```
348+
349+
**Running and updating tests:**
350+
351+
```shell
352+
# Run tests and generate snapshots
353+
cargo test -p pgt_analyser --test rules_tests
354+
355+
# Review and accept new/changed snapshots
356+
cargo insta test --accept
357+
358+
# Or review snapshots interactively
359+
cargo insta review
360+
```
361+
362+
The snapshot files (`.sql.snap`) are auto-generated and should be committed to the repository. They capture the expected diagnostic output for each test case.
363+
312364
### Code generation
313365

314366
For simplicity, use `just` to run all the commands with:

‎crates/pgt_analyser/src/lint/safety.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
//! Generated file, do not edit by hand, see `xtask/codegen`
22
33
use pgt_analyse::declare_lint_group;
4+
pub mod add_serial_column;
45
pub mod adding_field_with_default;
56
pub mod adding_foreign_key_constraint;
67
pub mod adding_not_null_field;
@@ -29,4 +30,4 @@ pub mod renaming_table;
2930
pub mod require_concurrent_index_creation;
3031
pub mod require_concurrent_index_deletion;
3132
pub mod transaction_nesting;
32-
declare_lint_group! { pub Safety { name : "safety" , rules : [self :: adding_field_with_default :: AddingFieldWithDefault , self :: adding_foreign_key_constraint :: AddingForeignKeyConstraint , self :: adding_not_null_field :: AddingNotNullField , self :: adding_primary_key_constraint :: AddingPrimaryKeyConstraint , self :: adding_required_field :: AddingRequiredField , self :: ban_char_field :: BanCharField , self :: ban_concurrent_index_creation_in_transaction :: BanConcurrentIndexCreationInTransaction , self :: ban_drop_column :: BanDropColumn , self :: ban_drop_database :: BanDropDatabase , self :: ban_drop_not_null :: BanDropNotNull , self :: ban_drop_table :: BanDropTable , self :: ban_truncate_cascade :: BanTruncateCascade , self :: changing_column_type :: ChangingColumnType , self :: constraint_missing_not_valid :: ConstraintMissingNotValid , self :: disallow_unique_constraint :: DisallowUniqueConstraint , self :: prefer_big_int :: PreferBigInt , self :: prefer_bigint_over_int :: PreferBigintOverInt , self :: prefer_bigint_over_smallint :: PreferBigintOverSmallint , self :: prefer_identity :: PreferIdentity , self :: prefer_jsonb :: PreferJsonb , self :: prefer_robust_stmts :: PreferRobustStmts , self :: prefer_text_field :: PreferTextField , self :: prefer_timestamptz :: PreferTimestamptz , self :: renaming_column :: RenamingColumn , self :: renaming_table :: RenamingTable , self :: require_concurrent_index_creation :: RequireConcurrentIndexCreation , self :: require_concurrent_index_deletion :: RequireConcurrentIndexDeletion , self :: transaction_nesting :: TransactionNesting ,] } }
33+
declare_lint_group! { pub Safety { name : "safety" , rules : [self :: add_serial_column :: AddSerialColumn , self :: adding_field_with_default :: AddingFieldWithDefault , self :: adding_foreign_key_constraint :: AddingForeignKeyConstraint , self :: adding_not_null_field :: AddingNotNullField , self :: adding_primary_key_constraint :: AddingPrimaryKeyConstraint , self :: adding_required_field :: AddingRequiredField , self :: ban_char_field :: BanCharField , self :: ban_concurrent_index_creation_in_transaction :: BanConcurrentIndexCreationInTransaction , self :: ban_drop_column :: BanDropColumn , self :: ban_drop_database :: BanDropDatabase , self :: ban_drop_not_null :: BanDropNotNull , self :: ban_drop_table :: BanDropTable , self :: ban_truncate_cascade :: BanTruncateCascade , self :: changing_column_type :: ChangingColumnType , self :: constraint_missing_not_valid :: ConstraintMissingNotValid , self :: disallow_unique_constraint :: DisallowUniqueConstraint , self :: prefer_big_int :: PreferBigInt , self :: prefer_bigint_over_int :: PreferBigintOverInt , self :: prefer_bigint_over_smallint :: PreferBigintOverSmallint , self :: prefer_identity :: PreferIdentity , self :: prefer_jsonb :: PreferJsonb , self :: prefer_robust_stmts :: PreferRobustStmts , self :: prefer_text_field :: PreferTextField , self :: prefer_timestamptz :: PreferTimestamptz , self :: renaming_column :: RenamingColumn , self :: renaming_table :: RenamingTable , self :: require_concurrent_index_creation :: RequireConcurrentIndexCreation , self :: require_concurrent_index_deletion :: RequireConcurrentIndexDeletion , self :: transaction_nesting :: TransactionNesting ,] } }

‎crates/pgt_analyser/src/lint/safety/add_serial_column.rs‎

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
use pgt_analyse::{Rule, RuleDiagnostic, RuleSource, context::RuleContext, declare_lint_rule};
2+
use pgt_console::markup;
3+
use pgt_diagnostics::Severity;
4+
5+
declare_lint_rule! {
6+
/// Adding a column with a SERIAL type or GENERATED ALWAYS AS ... STORED causes a full table rewrite.
7+
///
8+
/// When adding a column with a SERIAL type (serial, bigserial, smallserial) or a GENERATED ALWAYS AS ... STORED column
9+
/// to an existing table, PostgreSQL must rewrite the entire table while holding an ACCESS EXCLUSIVE lock.
10+
/// This blocks all reads and writes to the table for the duration of the rewrite operation.
11+
///
12+
/// SERIAL types are implemented using sequences and DEFAULT values, while GENERATED ... STORED columns require
13+
/// computing and storing values for all existing rows. Both operations require rewriting every row in the table.
14+
///
15+
/// ## Examples
16+
///
17+
/// ### Invalid
18+
///
19+
/// ```sql,expect_diagnostic
20+
/// ALTER TABLE prices ADD COLUMN id serial;
21+
/// ```
22+
///
23+
/// ```sql,expect_diagnostic
24+
/// ALTER TABLE prices ADD COLUMN id bigserial;
25+
/// ```
26+
///
27+
/// ```sql,expect_diagnostic
28+
/// ALTER TABLE prices ADD COLUMN total int GENERATED ALWAYS AS (price * quantity) STORED;
29+
/// ```
30+
///
31+
pub AddSerialColumn {
32+
version: "next",
33+
name: "addSerialColumn",
34+
severity: Severity::Error,
35+
recommended: true,
36+
sources: &[RuleSource::Eugene("E11")],
37+
}
38+
}
39+
40+
impl Rule for AddSerialColumn {
41+
type Options = ();
42+
43+
fn run(ctx: &RuleContext<Self>) -> Vec<RuleDiagnostic> {
44+
let mut diagnostics = Vec::new();
45+
46+
if let pgt_query::NodeEnum::AlterTableStmt(stmt) = &ctx.stmt() {
47+
for cmd in &stmt.cmds {
48+
if let Some(pgt_query::NodeEnum::AlterTableCmd(cmd)) = &cmd.node {
49+
if cmd.subtype() == pgt_query::protobuf::AlterTableType::AtAddColumn {
50+
if let Some(pgt_query::NodeEnum::ColumnDef(col_def)) =
51+
&cmd.def.as_ref().and_then(|d| d.node.as_ref())
52+
{
53+
// Check for SERIAL types
54+
if let Some(type_name) = &col_def.type_name {
55+
let type_str = get_type_name(type_name);
56+
if is_serial_type(&type_str) {
57+
diagnostics.push(
58+
RuleDiagnostic::new(
59+
rule_category!(),
60+
None,
61+
markup! {
62+
"Adding a column with type "<Emphasis>{type_str}</Emphasis>" requires a table rewrite."
63+
},
64+
)
65+
.detail(None, "SERIAL types require rewriting the entire table with an ACCESS EXCLUSIVE lock, blocking all reads and writes.")
66+
.note("SERIAL types cannot be added to existing tables without a full table rewrite. Consider using a non-serial type with a sequence instead."),
67+
);
68+
continue;
69+
}
70+
}
71+
72+
// Check for GENERATED ALWAYS AS ... STORED
73+
let has_stored_generated =
74+
col_def.constraints.iter().any(|constraint| {
75+
if let Some(pgt_query::NodeEnum::Constraint(c)) =
76+
&constraint.node
77+
{
78+
c.contype()
79+
== pgt_query::protobuf::ConstrType::ConstrGenerated
80+
&& c.generated_when == "a" // 'a' = ALWAYS
81+
} else {
82+
false
83+
}
84+
});
85+
86+
if has_stored_generated {
87+
diagnostics.push(
88+
RuleDiagnostic::new(
89+
rule_category!(),
90+
None,
91+
markup! {
92+
"Adding a column with "<Emphasis>"GENERATED ALWAYS AS ... STORED"</Emphasis>" requires a table rewrite."
93+
},
94+
)
95+
.detail(None, "GENERATED ... STORED columns require rewriting the entire table with an ACCESS EXCLUSIVE lock, blocking all reads and writes.")
96+
.note("GENERATED ... STORED columns cannot be added to existing tables without a full table rewrite."),
97+
);
98+
}
99+
}
100+
}
101+
}
102+
}
103+
}
104+
105+
diagnostics
106+
}
107+
}
108+
109+
fn get_type_name(type_name: &pgt_query::protobuf::TypeName) -> String {
110+
type_name
111+
.names
112+
.iter()
113+
.filter_map(|n| {
114+
if let Some(pgt_query::NodeEnum::String(s)) = &n.node {
115+
Some(s.sval.as_str())
116+
} else {
117+
None
118+
}
119+
})
120+
.collect::<Vec<_>>()
121+
.join(".")
122+
}
123+
124+
fn is_serial_type(type_name: &str) -> bool {
125+
matches!(
126+
type_name,
127+
"serial"
128+
| "bigserial"
129+
| "smallserial"
130+
| "pg_catalog.serial"
131+
| "pg_catalog.bigserial"
132+
| "pg_catalog.smallserial"
133+
// Also check for serial2, serial4, serial8 which are aliases
134+
| "serial2"
135+
| "serial4"
136+
| "serial8"
137+
| "pg_catalog.serial2"
138+
| "pg_catalog.serial4"
139+
| "pg_catalog.serial8"
140+
)
141+
}

‎crates/pgt_analyser/src/options.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
//! Generated file, do not edit by hand, see `xtask/codegen`
22
33
use crate::lint;
4+
pub type AddSerialColumn =
5+
<lint::safety::add_serial_column::AddSerialColumn as pgt_analyse::Rule>::Options;
46
pub type AddingFieldWithDefault =
57
<lint::safety::adding_field_with_default::AddingFieldWithDefault as pgt_analyse::Rule>::Options;
68
pub type AddingForeignKeyConstraint = < lint :: safety :: adding_foreign_key_constraint :: AddingForeignKeyConstraint as pgt_analyse :: Rule > :: Options ;

‎crates/pgt_analyser/tests/specs/safety/addSerialColumn/basic.sql‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
-- expect_lint/safety/addSerialColumn
2+
-- Test adding serial column to existing table
3+
ALTER TABLE prices ADD COLUMN id serial;

‎crates/pgt_analyser/tests/specs/safety/addSerialColumn/basic.sql.snap‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
---
2+
source: crates/pgt_analyser/tests/rules_tests.rs
3+
expression: snapshot
4+
snapshot_kind: text
5+
---
6+
# Input
7+
```
8+
-- expect_lint/safety/addSerialColumn
9+
-- Test adding serial column to existing table
10+
ALTER TABLE prices ADD COLUMN id serial;
11+
12+
```
13+
14+
# Diagnostics
15+
lint/safety/addSerialColumn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
16+
17+
× Adding a column with type serial requires a table rewrite.
18+
19+
i SERIAL types require rewriting the entire table with an ACCESS EXCLUSIVE lock, blocking all reads and writes.
20+
21+
i SERIAL types cannot be added to existing tables without a full table rewrite. Consider using a non-serial type with a sequence instead.

‎crates/pgt_analyser/tests/specs/safety/addSerialColumn/bigserial.sql‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
-- expect_lint/safety/addSerialColumn
2+
-- Test adding bigserial column to existing table
3+
ALTER TABLE prices ADD COLUMN big_id bigserial;

‎crates/pgt_analyser/tests/specs/safety/addSerialColumn/bigserial.sql.snap‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
---
2+
source: crates/pgt_analyser/tests/rules_tests.rs
3+
expression: snapshot
4+
snapshot_kind: text
5+
---
6+
# Input
7+
```
8+
-- expect_lint/safety/addSerialColumn
9+
-- Test adding bigserial column to existing table
10+
ALTER TABLE prices ADD COLUMN big_id bigserial;
11+
12+
```
13+
14+
# Diagnostics
15+
lint/safety/addSerialColumn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
16+
17+
× Adding a column with type bigserial requires a table rewrite.
18+
19+
i SERIAL types require rewriting the entire table with an ACCESS EXCLUSIVE lock, blocking all reads and writes.
20+
21+
i SERIAL types cannot be added to existing tables without a full table rewrite. Consider using a non-serial type with a sequence instead.

‎crates/pgt_analyser/tests/specs/safety/addSerialColumn/generated_stored.sql‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
-- expect_lint/safety/addSerialColumn
2+
-- Test adding GENERATED ALWAYS AS ... STORED column to existing table
3+
ALTER TABLE prices ADD COLUMN total integer GENERATED ALWAYS AS (price * quantity) STORED;

0 commit comments

Comments
 (0)