[pg] Use ALTER TABLE instead of copies (#243)

For the Postgres backend, use the appropriate `ALTER TABLE` command
for changed columns instead of copying and recreating the table.

This makes the generated sql more readable and also makes it easier to
be correct with foreign key constraints, since the recreated table does not
automatically replace the old one in constraints.

butane/tests/migration-tests.rs

@@ -227,6 +227,50 @@ fn migration_add_field_with_default_pg() {
     );
 }
 
+#[cfg(feature = "pg")]
+#[test]
+fn migration_modify_field_pg() {
+    let (mut conn, _data) = pg_connection();
+    // Not verifying rename right now because we don't detect it
+    // https://github.com/Electron100/butane/issues/89
+
+    migration_modify_field_type_change(
+        &mut conn,
+        "ALTER TABLE Foo ALTER COLUMN bar SET DATA TYPE BIGINT;",
+        "ALTER TABLE Foo ALTER COLUMN bar SET DATA TYPE INTEGER;",
+    );
+
+    migration_modify_field_nullability_change(
+        &mut conn,
+        "ALTER TABLE Foo ALTER COLUMN bar DROP NOT NULL;",
+        "ALTER TABLE Foo ALTER COLUMN bar SET NOT NULL;",
+    );
+
+    migration_modify_field_pkey_change(
+        &mut conn,
+        "ALTER TABLE Foo DROP CONSTRAINT IF EXISTS Foo_pkey;\nALTER TABLE Foo ADD PRIMARY KEY (baz);",
+        "ALTER TABLE Foo DROP CONSTRAINT IF EXISTS Foo_pkey;\nALTER TABLE Foo ADD PRIMARY KEY (bar);",
+    );
+
+    migration_modify_field_uniqueness_change(
+        &mut conn,
+        "ALTER TABLE Foo ADD UNIQUE (bar);",
+        "ALTER TABLE Foo DROP CONSTRAINT Foo_bar_key;",
+    );
+
+    migration_modify_field_default_added(
+        &mut conn,
+        "ALTER TABLE Foo ALTER COLUMN bar SET DEFAULT 42;",
+        "ALTER TABLE Foo ALTER COLUMN bar DROP DEFAULT;",
+    );
+
+    migration_modify_field_different_default(
+        &mut conn,
+        "ALTER TABLE Foo ALTER COLUMN bar SET DEFAULT 42;",
+        "ALTER TABLE Foo ALTER COLUMN bar SET DEFAULT 41;",
+    );
+}
+
 #[cfg(feature = "sqlite")]
 #[test]
 fn migration_add_and_remove_field_sqlite() {
@@ -371,6 +415,114 @@ fn migration_add_field_with_default(conn: &mut Connection, up_sql: &str, down_sq
     test_migrate(conn, init, v2, up_sql, down_sql);
 }
 
+fn migration_modify_field_type_change(conn: &mut Connection, up_sql: &str, down_sql: &str) {
+    let init = quote! {
+        struct Foo {
+            id: i64,
+            bar: i32,
+        }
+    };
+
+    let v2 = quote! {
+        struct Foo {
+            id: i64,
+            bar: i64,
+        }
+    };
+    test_migrate(conn, init, v2, up_sql, down_sql);
+}
+
+fn migration_modify_field_nullability_change(conn: &mut Connection, up_sql: &str, down_sql: &str) {
+    let init = quote! {
+        struct Foo {
+            id: i64,
+            bar: i32,
+        }
+    };
+
+    let v2 = quote! {
+        struct Foo {
+            id: i64,
+            bar: Option<i32>,
+        }
+    };
+    test_migrate(conn, init, v2, up_sql, down_sql);
+}
+
+fn migration_modify_field_uniqueness_change(conn: &mut Connection, up_sql: &str, down_sql: &str) {
+    let init = quote! {
+        struct Foo {
+            id: i64,
+            bar: i32,
+        }
+    };
+
+    let v2 = quote! {
+        struct Foo {
+            id: i64,
+            #[unique]
+            bar: i32,
+        }
+    };
+    test_migrate(conn, init, v2, up_sql, down_sql);
+}
+
+fn migration_modify_field_pkey_change(conn: &mut Connection, up_sql: &str, down_sql: &str) {
+    let init = quote! {
+        struct Foo {
+            #[pk]
+            bar: i64,
+            baz: i32,
+        }
+    };
+
+    let v2 = quote! {
+        struct Foo {
+            bar: i64,
+            #[pk]
+            baz: i32
+        }
+    };
+    test_migrate(conn, init, v2, up_sql, down_sql);
+}
+
+fn migration_modify_field_default_added(conn: &mut Connection, up_sql: &str, down_sql: &str) {
+    let init = quote! {
+        struct Foo {
+            id: i64,
+            bar: String,
+        }
+    };
+
+    let v2 = quote! {
+        struct Foo {
+            id: i64,
+            #[default=42]
+            bar: String,
+        }
+    };
+    test_migrate(conn, init, v2, up_sql, down_sql);
+}
+
+fn migration_modify_field_different_default(conn: &mut Connection, up_sql: &str, down_sql: &str) {
+    let init = quote! {
+        struct Foo {
+            id: i64,
+            #[default=41]
+            bar: String,
+        }
+    };
+
+    let v2 = quote! {
+        struct Foo {
+            id: i64,
+            #[default=42]
+            bar: String,
+        }
+    };
+    test_migrate(conn, init, v2, up_sql, down_sql);
+}
+
 fn migration_add_and_remove_field(conn: &mut Connection, up_sql: &str, down_sql: &str) {
     let init = quote! {
         struct Foo {

butane_core/src/db/helper.rs

@@ -293,7 +293,7 @@ fn sql_column(col: query::Column, w: &mut impl Write) {
     .unwrap()
 }
 
-pub fn sql_literal_value(val: SqlVal) -> Result<String> {
+pub fn sql_literal_value(val: &SqlVal) -> Result<String> {
     use SqlVal::*;
     match val {
         SqlVal::Null => Ok("NULL".to_string()),
@@ -307,6 +307,6 @@ pub fn sql_literal_value(val: SqlVal) -> Result<String> {
         Json(val) => Ok(format!("{val}")),
         #[cfg(feature = "datetime")]
         Timestamp(ndt) => Ok(ndt.format("'%Y-%m-%dT%H:%M:%S%.f'").to_string()),
-        Custom(val) => Err(Error::LiteralForCustomUnsupported((*val).clone())),
+        Custom(val) => Err(Error::LiteralForCustomUnsupported(*(*val).clone())),
     }
 }

butane_core/src/db/pg.rs

@@ -553,7 +553,7 @@ fn sql_for_op(current: &mut ADB, op: &Operation) -> Result<String> {
         Operation::RemoveTable(name) => Ok(drop_table(name)),
         Operation::AddColumn(tbl, col) => add_column(tbl, col),
         Operation::RemoveColumn(tbl, name) => Ok(remove_column(tbl, name)),
-        Operation::ChangeColumn(tbl, old, new) => change_column(current, tbl, old, Some(new)),
+        Operation::ChangeColumn(tbl, old, new) => change_column(current, tbl, old, new),
     }
 }
 
@@ -662,7 +662,7 @@ fn add_column(tbl_name: &str, col: &AColumn) -> Result<String> {
         "ALTER TABLE {} ADD COLUMN {} DEFAULT {};",
         helper::quote_reserved_word(tbl_name),
         define_column(col)?,
-        helper::sql_literal_value(default)?
+        helper::sql_literal_value(&default)?
     )];
     if col.reference().is_some() {
         stmts.push(define_constraint(tbl_name, col));
@@ -679,31 +679,13 @@ fn remove_column(tbl_name: &str, name: &str) -> String {
     )
 }
 
-fn copy_table(old: &ATable, new: &ATable) -> String {
-    let column_names = new
-        .columns
-        .iter()
-        .map(|col| helper::quote_reserved_word(col.name()))
-        .collect::<Vec<Cow<str>>>()
-        .join(", ");
-    format!(
-        "INSERT INTO {} SELECT {} FROM {};",
-        helper::quote_reserved_word(&new.name),
-        column_names,
-        helper::quote_reserved_word(&old.name)
-    )
-}
-
-fn tmp_table_name(name: &str) -> String {
-    format!("{name}__butane_tmp")
-}
-
 fn change_column(
     current: &mut ADB,
     tbl_name: &str,
     old: &AColumn,
-    new: Option<&AColumn>,
+    new: &AColumn,
 ) -> Result<String> {
+    use helper::quote_reserved_word;
     let table = current.get_table(tbl_name);
     if table.is_none() {
         crate::warn!(
@@ -713,28 +695,110 @@ fn change_column(
         );
         return Ok(String::new());
     }
-    let old_table = table.unwrap();
-    let mut new_table = old_table.clone();
-    new_table.name = tmp_table_name(&new_table.name);
-    match new {
-        Some(col) => new_table.replace_column(col.clone()),
-        None => new_table.remove_column(old.name()),
-    }
-    let mut stmts: Vec<String> = vec![
-        create_table(&new_table, false)?,
-        create_table_constraints(&new_table),
-        copy_table(old_table, &new_table),
-        drop_table(&old_table.name),
-        format!(
-            "ALTER TABLE {} RENAME TO {};",
-            helper::quote_reserved_word(&new_table.name),
-            helper::quote_reserved_word(tbl_name)
-        ),
-    ];
-    stmts.retain(|stmt| !stmt.is_empty());
+
+    // Let's figure out what changed about the column
+    let mut stmts: Vec<String> = Vec::new();
+    if old.name() != new.name() {
+        // column rename
+        stmts.push(format!(
+            "ALTER TABLE {} RENAME COLUMN {} TO {};",
+            quote_reserved_word(tbl_name),
+            quote_reserved_word(old.name()),
+            quote_reserved_word(new.name())
+        ));
+    }
+    if old.typeid()? != new.typeid()? {
+        // column type change
+        stmts.push(format!(
+            "ALTER TABLE {} ALTER COLUMN {} SET DATA TYPE {};",
+            quote_reserved_word(tbl_name),
+            quote_reserved_word(old.name()),
+            col_sqltype(new)?,
+        ));
+    }
+    if old.nullable() != new.nullable() {
+        stmts.push(format!(
+            "ALTER TABLE {} ALTER COLUMN {} {} NOT NULL;",
+            quote_reserved_word(tbl_name),
+            quote_reserved_word(old.name()),
+            if new.nullable() { "DROP" } else { "SET" }
+        ));
+    }
+    if old.is_pk() != new.is_pk() {
+        // Change to primary key
+        // Either way, drop the previous primary key
+        // Butane does not currently support composite primary keys
+
+        if new.is_pk() {
+            // Drop the old primary key
+            stmts.push(format!(
+                "ALTER TABLE {} DROP CONSTRAINT IF EXISTS {}_pkey;",
+                quote_reserved_word(tbl_name),
+                tbl_name
+            ));
+
+            // add the new primary key
+            stmts.push(format!(
+                "ALTER TABLE {} ADD PRIMARY KEY ({});",
+                quote_reserved_word(tbl_name),
+                quote_reserved_word(new.name())
+            ));
+        } else {
+            // this field is no longer the primary key. Butane requires a single primary key,
+            // so some other column must be the primary key now. It will drop the constraint when processed.
+        }
+    }
+    if old.unique() != new.unique() {
+        // Changed uniqueness constraint
+        if new.unique() {
+            stmts.push(format!(
+                "ALTER TABLE {} ADD UNIQUE ({});",
+                quote_reserved_word(tbl_name),
+                quote_reserved_word(new.name())
+            ));
+        } else {
+            // Standard constraint naming scheme
+            stmts.push(format!(
+                "ALTER TABLE {} DROP CONSTRAINT {}_{}_key;",
+                quote_reserved_word(tbl_name),
+                tbl_name,
+                &old.name()
+            ));
+        }
+    }
+
+    if old.default() != new.default() {
+        stmts.push(match new.default() {
+            None => format!(
+                "ALTER TABLE {} ALTER COLUMN {} DROP DEFAULT;",
+                quote_reserved_word(tbl_name),
+                quote_reserved_word(old.name())
+            ),
+            Some(val) => format!(
+                "ALTER TABLE {} ALTER COLUMN {} SET DEFAULT {};",
+                quote_reserved_word(tbl_name),
+                quote_reserved_word(old.name()),
+                helper::sql_literal_value(val)?
+            ),
+        });
+    }
+
+    if old.reference() != new.reference() {
+        if old.reference().is_some() {
+            // Drop the old reference
+            stmts.push(format!(
+                "ALTER TABLE {} DROP CONSTRAINT {}_{}_fkey;",
+                quote_reserved_word(tbl_name),
+                tbl_name,
+                old.name()
+            ));
+        }
+        if new.reference().is_some() {
+            stmts.push(define_constraint(tbl_name, new));
+        }
+    }
+
     let result = stmts.join("\n");
-    new_table.name.clone_from(&old_table.name);
-    current.replace_table(new_table);
     Ok(result)
 }
 

butane_core/src/db/sqlite.rs

@@ -621,7 +621,7 @@ fn add_column(tbl_name: &str, col: &AColumn) -> Result<String> {
         "ALTER TABLE {} ADD COLUMN {} DEFAULT {};",
         helper::quote_reserved_word(tbl_name),
         define_column(col),
-        helper::sql_literal_value(default)?
+        helper::sql_literal_value(&default)?
     ))
 }