Repository: elixir-ecto/ecto_sql
Pull request #644 (2024-10-17 21:57:05)

Do not modify column type while modifying fk constraint
The following migration syntax:

```elixir
alter table(:my_table) do
  modify :parent_id, references(:my_table, type: :binary_id, on_delete: :nilify_all, validate: false),
    from: {:binary_id, null: true}
end
```

Produces a SQL equivalent like this:

```sql
ALTER TABLE "my_table"
  ALTER COLUMN "parent_id" TYPE uuid,
  ADD CONSTRAINT "my_table_parent_id_fkey" FOREIGN KEY ("parent_id") REFERENCES "my_table"("id") ON DELETE SET NULL NOT VALID
```

The problem with the first directive, `ALTER COLUMN`, is that even if the field type is the same, at least in PostgreSQL, is creating bunch of `AccessExclusiveLock` (the most restrictive one) on each of the indexes associated to that field, and the table itself, when that's not needed at all, at least in this case.

For example:

```console
test_dev=# BEGIN;
  LOCK TABLE "schema_migrations" IN SHARE UPDATE EXCLUSIVE MODE;
  ALTER TABLE "my_table" ALTER COLUMN "parent_id" TYPE uuid, ADD CONSTRAINT "my_table_parent_id_fkey" FOREIGN KEY ("parent_id") REFERENCES "my_table"("id") ON DELETE SET NULL NOT VALID;
  INSERT INTO "schema_migrations" ("version","inserted_at") VALUES ('20210718210952',NOW());
  SELECT locktype, relation::regclass, mode, transactionid AS tid, virtualtransaction AS vtid, pid, granted FROM pg_locks;
COMMIT;
BEGIN
LOCK TABLE
ALTER TABLE
INSERT 0 1
   locktype    |                     relation                        |           mode           |   tid   |  vtid   |  pid  | granted
---------------+-----------------------------------------------------+--------------------------+---------+---------+-------+---------
 relation      | schema_migrations                                   | RowExclusiveLock         |         | 4/14045 | 16813 | t
 virtualxid    |                                                     | ExclusiveLock            |         | 4/14045 | 16813 | t
 relation      | my_table_parent_id_ppppppp_id_index                 | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | pg_locks                                            | AccessShareLock          |         | 4/14045 | 16813 | t
 relation      | 134438                                              | AccessShareLock          |         | 4/14045 | 16813 | t
 relation      | 134438                                              | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | my_table_parent_id_rrrrr_index                      | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | my_table                                            | AccessShareLock          |         | 4/14045 | 16813 | t
 relation      | my_table                                            | ShareLock                |         | 4/14045 | 16813 | t
 relation      | my_table                                            | ShareRowExclusiveLock    |         | 4/14045 | 16813 | t
 relation      | my_table                                            | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | my_table_project_id_xxxx_index                      | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | 135159                                              | AccessShareLock          |         | 4/14045 | 16813 | t
 relation      | 135159                                              | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | 132875                                              | AccessShareLock          |         | 4/14045 | 16813 | t
 relation      | 132875                                              | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | 132507                                              | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | 132506                                              | AccessShareLock          |         | 4/14045 | 16813 | t
 relation      | 132506                                              | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | schema_migrations                                   | ShareUpdateExclusiveLock |         | 4/14045 | 16813 | t
 relation      | 132504                                              | AccessShareLock          |         | 4/14045 | 16813 | t
 relation      | 132504                                              | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | 135184                                              | AccessShareLock          |         | 4/14045 | 16813 | t
 relation      | 135184                                              | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | 132505                                              | AccessShareLock          |         | 4/14045 | 16813 | t
 relation      | 132505                                              | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | my_table_parent_id_ttttt_index                      | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 transactionid |                                                     | ExclusiveLock            | 2175345 | 4/14045 | 16813 | t
 relation      | my_table_parent_id_aaaaaaaaaaa_id_index             | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | 132503                                              | AccessShareLock          |         | 4/14045 | 16813 | t
 relation      | 132503                                              | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | my_table_parent_id_gggg_index                       | AccessExclusiveLock      |         | 4/14045 | 16813 | t
(35 rows)

COMMIT
```

So, the proposal is to avoid setting the type when you pass `references` to `modify`, if there is any type incompatibility, PostgreSQL, will let you know about it right away:

```
ERROR:  foreign key constraint "my_table_parent_id_fkey" cannot be implemented
DETAIL:  Key columns "parent_id" and "id" are of incompatible types: character varying and uuid.
```

With this proposal, the resulting SQL syntax should be something like:

```sql
ALTER TABLE "my_table"
  ADD CONSTRAINT "my_table_parent_id_fkey" FOREIGN KEY ("parent_id") REFERENCES "my_table"("id") ON DELETE SET NULL NOT VALID
```
That will produce more manageable lock types:

```console
   locktype    |     relation      |           mode           |   tid   |  vtid   |  pid  | granted
---------------+-------------------+--------------------------+---------+---------+-------+---------
 relation      | pg_locks          | AccessShareLock          |         | 4/14540 | 16849 | t
 relation      | schema_migrations | RowExclusiveLock         |         | 4/14540 | 16849 | t
 virtualxid    |                   | ExclusiveLock            |         | 4/14540 | 16849 | t
 transactionid |                   | ExclusiveLock            | 2175967 | 4/14540 | 16849 | t
 relation      | schema_migrations | ShareUpdateExclusiveLock |         | 4/14540 | 16849 | t
 relation      | my_table          | AccessShareLock          |         | 4/14540 | 16849 | t
 relation      | my_table          | ShareRowExclusiveLock    |         | 4/14540 | 16849 | t
(7 rows)
```

And with that, you can avoid potential downtime in production :bowtie: 

josevalim: Hi @milmazz! 👋 

> So, the proposal is to avoid setting the type when you pass references to modify, if there is any type incompatibility, PostgreSQL, will let you know about it right away:

But if I want to modify the type, what do we do? Maybe we should rather allow the type for references to be skipped and, in such cases, we don't modify it?
halfdan: ```
alter table(:my_table) do
  modify :parent_id, references(:my_table, type: :binary_id, on_delete: :nilify_all, validate: false),
    from: {:binary_id, null: true}
end
```

Wouldn't it be sufficient to skip the `alter column` if the type in the `from` and `references` match up? (here `:binary_id`)
greg-rychlewski: One thing I noticed when looking into this is that our description for modify is `Modifies the type of a column when altering a table.` in the docs. And the advice is to use `execute` when not wanting to modify the type. 

We have convenience functions for check constraints so maybe it would make sense to have one for foreign key constraints.

Patch:
diff --git a/lib/ecto/adapters/myxql/connection.ex b/lib/ecto/adapters/myxql/connection.ex
index 59a09d1..eb172a7 100644
--- a/lib/ecto/adapters/myxql/connection.ex
+++ b/lib/ecto/adapters/myxql/connection.ex
@@ -826,7 +826,7 @@ if Code.ensure_loaded?(MyXQL) do
             fun,
             ?(,
             modifier,
-            Enum.map_intersperse(args, ", ", &top_level_expr(&1, sources, query)),
+            Enum.map_intersperse(args, ", ", &expr(&1, sources, query)),
             ?)
           ]
       end
diff --git a/lib/ecto/adapters/postgres/connection.ex b/lib/ecto/adapters/postgres/connection.ex
index 996e8e1..ad39f41 100644
--- a/lib/ecto/adapters/postgres/connection.ex
+++ b/lib/ecto/adapters/postgres/connection.ex
@@ -1546,35 +1546,76 @@ if Code.ensure_loaded?(Postgrex) do
     end
 
     defp column_change(table, {:modify, name, %Reference{} = ref, opts}) do
-      [
-        drop_reference_expr(opts[:from], table, name),
-        "ALTER COLUMN ",
-        quote_name(name),
-        " TYPE ",
-        reference_column_type(ref.type, opts),
-        ", ADD ",
+      reference_column_type = reference_column_type(ref.type, opts)
+      from_column_type = extract_column_type(opts[:from])
+
+      drop_reference_expr = drop_reference_expr(opts[:from], table, name)
+      prefix_with_comma = (drop_reference_expr != [] && ", ") || ""
+
+      common_suffix = [
         reference_expr(ref, table, name),
         modify_null(name, opts),
         modify_default(name, ref.type, opts)
       ]
+
+      if reference_column_type == reference_column_type(from_column_type, opts) do
+        [
+          drop_reference_expr,
+          prefix_with_comma,
+          "ADD " | common_suffix
+        ]
+      else
+        [
+          drop_reference_expr,
+          prefix_with_comma,
+          "ALTER COLUMN ",
+          quote_name(name),
+          " TYPE ",
+          reference_column_type,
+          ", ADD " | common_suffix
+        ]
+      end
     end
 
     defp column_change(table, {:modify, name, type, opts}) do
-      [
-        drop_reference_expr(opts[:from], table, name),
-        "ALTER COLUMN ",
-        quote_name(name),
-        " TYPE ",
-        column_type(type, opts),
-        modify_null(name, opts),
-        modify_default(name, type, opts)
-      ]
+      column_type = column_type(type, opts)
+      from_column_type = extract_column_type(opts[:from])
+
+      drop_reference_expr = drop_reference_expr(opts[:from], table, name)
+      any_drop_ref? = drop_reference_expr != []
+
+      if column_type == column_type(from_column_type, opts) do
+        modify_null = modify_null(name, Keyword.put(opts, :prefix_with_comma, any_drop_ref?))
+        any_modify_null? = modify_null != []
+
+        modify_default =
+          modify_default(
+            name,
+            type,
+            Keyword.put(opts, :prefix_with_comma, any_drop_ref? or any_modify_null?)
+          )
+
+        [drop_reference_expr, modify_null, modify_default]
+      else
+        [
+          drop_reference_expr,
+          (any_drop_ref? && ", ") || "",
+          "ALTER COLUMN ",
+          quote_name(name),
+          " TYPE ",
+          column_type,
+          modify_null(name, opts),
+          modify_default(name, type, opts)
+        ]
+      end
     end
 
     defp column_change(_table, {:remove, name}), do: ["DROP COLUMN ", quote_name(name)]
 
     defp column_change(table, {:remove, name, %Reference{} = ref, _opts}) do
-      [drop_reference_expr(ref, table, name), "DROP COLUMN ", quote_name(name)]
+      drop_reference_expr = drop_reference_expr(ref, table, name)
+      prefix_with_comma = (drop_reference_expr != [] && ", ") || ""
+      [drop_reference_expr, prefix_with_comma, "DROP COLUMN ", quote_name(name)]
     end
 
     defp column_change(_table, {:remove, name, _type, _opts}),
@@ -1595,17 +1636,23 @@ if Code.ensure_loaded?(Postgrex) do
       do: ["DROP COLUMN IF EXISTS ", quote_name(name)]
 
     defp modify_null(name, opts) do
+      prefix_with_comma = Keyword.get(opts, :prefix_with_comma, true)
+      prefix = if prefix_with_comma, do: ", ", else: ""
+
       case Keyword.get(opts, :null) do
-        true -> [", ALTER COLUMN ", quote_name(name), " DROP NOT NULL"]
-        false -> [", ALTER COLUMN ", quote_name(name), " SET NOT NULL"]
+        true -> [prefix, "ALTER COLUMN ", quote_name(name), " DROP NOT NULL"]
+        false -> [prefix, "ALTER COLUMN ", quote_name(name), " SET NOT NULL"]
         nil -> []
       end
     end
 
     defp modify_default(name, type, opts) do
+      prefix_with_comma = Keyword.get(opts, :prefix_with_comma, true)
+      prefix = if prefix_with_comma, do: ", ", else: ""
+
       case Keyword.fetch(opts, :default) do
         {:ok, val} ->
-          [", ALTER COLUMN ", quote_name(name), " SET", default_expr({:ok, val}, type)]
+          [prefix, "ALTER COLUMN ", quote_name(name), " SET", default_expr({:ok, val}, type)]
 
         :error ->
           []
@@ -1797,6 +1844,11 @@ if Code.ensure_loaded?(Postgrex) do
       [type, generated_expr(generated)]
     end
 
+    defp extract_column_type({type, _}) when is_atom(type), do: type
+    defp extract_column_type(type) when is_atom(type), do: type
+    defp extract_column_type(%Reference{type: type}), do: type
+    defp extract_column_type(_), do: nil
+
     defp generated_expr(nil), do: []
 
     defp generated_expr(expr) when is_binary(expr) do
@@ -1833,7 +1885,7 @@ if Code.ensure_loaded?(Postgrex) do
       do: drop_reference_expr(ref, table, name)
 
     defp drop_reference_expr(%Reference{} = ref, table, name),
-      do: ["DROP CONSTRAINT ", reference_name(ref, table, name), ", "]
+      do: ["DROP CONSTRAINT ", reference_name(ref, table, name)]
 
     defp drop_reference_expr(_, _, _),
       do: []
diff --git a/lib/ecto/migration.ex b/lib/ecto/migration.ex
index c1f28e1..ab258e3 100644
--- a/lib/ecto/migration.ex
+++ b/lib/ecto/migration.ex
@@ -396,7 +396,7 @@ defmodule Ecto.Migration do
     @type t :: %__MODULE__{
             table: String.t(),
             prefix: String.t() | nil,
-            name: atom,
+            name: String.t() | atom,
             columns: [atom | String.t()],
             unique: boolean,
             concurrently: boolean,
@@ -779,7 +779,8 @@ defmodule Ecto.Migration do
 
   ## Options
 
-    * `:name` - the name of the index. Defaults to "#{table}_#{column}_index".
+    * `:name` - the name of the index. Can be provided as a string or an atom.
+    Defaults to "#{table}_#{column}_index".
     * `:prefix` - specify an optional prefix for the index.
     * `:unique` - indicates whether the index should be unique. Defaults to `false`.
     * `:comment` - adds a comment to the index.
@@ -1273,13 +1274,41 @@ defmodule Ecto.Migration do
 
   See `add/3` for more information on supported types.
 
-  If you want to modify a column without changing its type,
-  such as adding or dropping a null constraints, consider using
-  the `execute/2` command with the relevant SQL command instead
-  of `modify/3`, if supported by your database. This may avoid
-  redundant type updates and be more efficient, as an unnecessary
-  type update can lock the table, even if the type actually
-  doesn't change.
+  > #### Modifying a column without changing its type {: .warning}
+  >
+  > If you want to modify a column without changing its type,
+  > such as adding or dropping a null constraint, consider using
+  > the `execute/2` command with the relevant SQL command instead
+  > of `modify/3`, if supported by your database. This may avoid
+  > redundant type updates and be more efficient, as an unnecessary
+  > type update can lock the table, even if the type actually
+  > doesn't change.
+  >
+  > These undesired locks can be avoided when using the PostgreSQL adapter by
+  > providing the `:from` option and ensuring its type matches the type provided
+  > to `modify/3`. In that scenario, the type change part of the migration is omitted.
+  >
+  > Examples:
+  >
+  >     # modify column with rollback options
+  >     alter table("posts") do
+  >       modify :title, :text, null: false, from: {:text, null: true}
+  >     end
+  >
+  >     # adding a new foreign key constraint
+  >     alter table("posts") do
+  >       modify :author_id, references(:authors, type: :id, validate: false), from: :id
+  >     end
+  >
+  >     # Modify the :on_delete option of an existing foreign key
+  >     alter table("comments") do
+  >       modify :post_id, references(:posts, on_delete: :delete_all),
+  >         from: references(:posts, on_delete: :nothing)
+  >     end
+  >
+  > The previous syntax offers two benefits:
+  > 1. the migrations are reversible
+  > 2. the PostgreSQL adapter will skip the type update, due to the `:from` type matching the modify type
 
   ## Examples
 


Test patch:
diff --git a/integration_test/pg/migrations_test.exs b/integration_test/pg/migrations_test.exs
index a6c87cb..4b59a1b 100644
--- a/integration_test/pg/migrations_test.exs
+++ b/integration_test/pg/migrations_test.exs
@@ -40,6 +40,47 @@ defmodule Ecto.Integration.MigrationsTest do
     end
   end
 
+  defmodule AlterColumnMigrationViaModify do
+    use Ecto.Migration
+
+    def up do
+      create table(:my_users) do
+        add :from_null_to_not_null, :integer
+
+        add :from_default_to_no_default, :integer, default: 0
+        add :from_no_default_to_default, :integer
+      end
+
+      create table(:my_comments) do
+        add :user_id, references(:users)
+      end
+
+      create table(:my_posts) do
+        add :user_id, :bigserial
+      end
+
+      alter table(:my_users) do
+        modify :from_null_to_not_null, :string, null: false, from: {:string, null: true}
+        modify :from_default_to_no_default, :integer, default: nil, from: {:integer, default: 0}
+        modify :from_no_default_to_default, :integer, default: 0, from: {:integer, default: nil}
+      end
+
+      alter table(:my_comments) do
+        modify :user_id, references(:my_users, on_delete: :nilify_all), from: references(:my_users, on_delete: :nothing)
+      end
+
+      alter table(:my_posts) do
+        modify :user_id, references(:my_users), from: :bigserial
+      end
+    end
+
+    def down do
+      drop table(:my_posts)
+      drop table(:my_comments)
+      drop table(:my_users)
+    end
+  end
+
   test "logs Postgres notice messages" do
     log =
       capture_log(fn ->
@@ -90,6 +131,46 @@ defmodule Ecto.Integration.MigrationsTest do
       assert down_log =~ "commit []"
     end
 
+    test "does not alter column type when enough info is provided to modify/3" do
+      num = @base_migration + System.unique_integer([:positive])
+      up_log =
+        capture_log(fn ->
+          Ecto.Migrator.up(PoolRepo, num, AlterColumnMigrationViaModify, log_migrator_sql: :info, log_migrations_sql: :info, log: :info)
+        end)
+
+
+
+      assert Regex.scan(~r/(begin \[\])/, up_log) |> length() == 2
+      assert up_log =~ ~s(ALTER TABLE "my_users")
+      refute up_log =~ ~s(ALTER COLUMN "from_null_to_not_null" TYPE) 
+      assert up_log =~ ~s(ALTER COLUMN "from_null_to_not_null" SET NOT NULL,)
+      refute up_log =~ ~s(ALTER COLUMN "from_default_to_no_default" TYPE)
+      assert up_log =~ ~s(ALTER COLUMN "from_default_to_no_default" SET DEFAULT NULL,)
+      refute up_log =~ ~s(ALTER COLUMN "from_no_default_to_default" TYPE)
+      assert up_log =~ ~s(ALTER COLUMN "from_no_default_to_default" SET DEFAULT 0)
+
+      assert up_log =~ ~s(ALTER TABLE "my_comments")
+      assert up_log =~ ~s(DROP CONSTRAINT "my_comments_user_id_fkey",)
+      refute up_log =~ ~s(ALTER COLUMN "user_id" TYPE)
+      assert up_log =~ ~s/ADD CONSTRAINT "my_comments_user_id_fkey" FOREIGN KEY ("user_id") REFERENCES "my_users"("id") ON DELETE SET NULL/
+
+      assert up_log =~ ~s(ALTER TABLE "my_posts")
+      refute up_log =~ ~s(ALTER COLUMN "user_id" TYPE)
+      assert up_log =~ ~s/ADD CONSTRAINT "my_posts_user_id_fkey" FOREIGN KEY ("user_id") REFERENCES "my_users"("id")/
+      assert Regex.scan(~r/(commit \[\])/, up_log) |> length() == 2
+
+      down_log =
+        capture_log(fn ->
+          Ecto.Migrator.down(PoolRepo, num, AlterColumnMigrationViaModify, log_migrator_sql: :info, log_migrations_sql: :info, log: :info)
+        end)
+
+      assert down_log =~ "begin []"
+      assert down_log =~ ~s(DROP TABLE "my_comments")
+      assert down_log =~ ~s(DROP TABLE "my_posts")
+      assert down_log =~ ~s(DROP TABLE "my_users")
+      assert down_log =~ "commit []"
+    end
+
     test "logs advisory lock and transaction commands" do
       num = @base_migration + System.unique_integer([:positive])
       up_log =
diff --git a/test/ecto/adapters/myxql_test.exs b/test/ecto/adapters/myxql_test.exs
index c2d499d..ee02cbb 100644
--- a/test/ecto/adapters/myxql_test.exs
+++ b/test/ecto/adapters/myxql_test.exs
@@ -416,6 +416,14 @@ defmodule Ecto.Adapters.MyXQLTest do
     assert all(query) == ~s{SELECT coalesce(s0.`x`, 5) FROM `schema` AS s0}
   end
 
+  test "coalesce with subquery" do
+    squery = from s in Schema, select: s.x
+    query = Schema |> select([s], coalesce(subquery(squery), 5)) |> plan()
+
+    assert all(query) ==
+             ~s{SELECT coalesce((SELECT ss0.`x` AS `x` FROM `schema` AS ss0), 5) FROM `schema` AS s0}
+  end
+
   test "where" do
     query = Schema |> where([r], r.x == 42) |> where([r], r.y != 43) |> select([r], r.x) |> plan()
 
@@ -478,7 +486,7 @@ defmodule Ecto.Adapters.MyXQLTest do
       |> plan()
 
     assert all(query) ==
-             ~s{SELECT s0.`x` FROM `schema` AS s0 ORDER BY exists(SELECT ss0.`x` AS `result` FROM `schema` AS ss0 WHERE (ss0.`x` = s0.`x`))}
+             ~s{SELECT s0.`x` FROM `schema` AS s0 ORDER BY exists((SELECT ss0.`x` AS `result` FROM `schema` AS ss0 WHERE (ss0.`x` = s0.`x`)))}
   end
 
   test "union and union all" do
@@ -882,7 +890,7 @@ defmodule Ecto.Adapters.MyXQLTest do
       |> plan()
 
     assert all(query) ==
-             ~s{SELECT s0.`x` FROM `schema` AS s0 GROUP BY exists(SELECT ss0.`x` AS `result` FROM `schema` AS ss0 WHERE (ss0.`x` = s0.`x`))}
+             ~s{SELECT s0.`x` FROM `schema` AS s0 GROUP BY exists((SELECT ss0.`x` AS `result` FROM `schema` AS ss0 WHERE (ss0.`x` = s0.`x`)))}
   end
 
   test "interpolated values" do
@@ -1089,7 +1097,7 @@ defmodule Ecto.Adapters.MyXQLTest do
         |> plan
 
       assert all(query) ==
-               ~s{SELECT s0.`x` FROM `schema` AS s0 WINDOW `w` AS (ORDER BY exists(SELECT ss0.`x` AS `result` FROM `schema` AS ss0 WHERE (ss0.`x` = s0.`x`)))}
+               ~s{SELECT s0.`x` FROM `schema` AS s0 WINDOW `w` AS (ORDER BY exists((SELECT ss0.`x` AS `result` FROM `schema` AS ss0 WHERE (ss0.`x` = s0.`x`))))}
     end
 
     test "two windows" do
diff --git a/test/ecto/adapters/postgres_test.exs b/test/ecto/adapters/postgres_test.exs
index 617f59f..9c15729 100644
--- a/test/ecto/adapters/postgres_test.exs
+++ b/test/ecto/adapters/postgres_test.exs
@@ -584,6 +584,14 @@ defmodule Ecto.Adapters.PostgresTest do
     assert all(query) == ~s{SELECT coalesce(s0."x", 5) FROM "schema" AS s0}
   end
 
+  test "coalesce with subquery" do
+    squery = from s in Schema, select: s.x
+    query = Schema |> select([s], coalesce(subquery(squery), 5)) |> plan()
+
+    assert all(query) ==
+             ~s{SELECT coalesce((SELECT ss0."x" AS "x" FROM "schema" AS ss0), 5) FROM "schema" AS s0}
+  end
+
   test "where" do
     query = Schema |> where([r], r.x == 42) |> where([r], r.y != 43) |> select([r], r.x) |> plan()
 
@@ -2535,7 +2543,6 @@ defmodule Ecto.Adapters.PostgresTest do
              ALTER COLUMN "space_id" TYPE integer,
              ALTER COLUMN "space_id" DROP NOT NULL,
              DROP CONSTRAINT "posts_group_id_fkey",
-             ALTER COLUMN "group_id" TYPE bigint,
              ADD CONSTRAINT "posts_group_id_fkey" FOREIGN KEY ("group_id") REFERENCES "groups"("gid"),
              ALTER COLUMN "status" TYPE varchar(100),
              ALTER COLUMN "status" SET NOT NULL,
@@ -2636,6 +2643,26 @@ defmodule Ecto.Adapters.PostgresTest do
            ]
   end
 
+  test "alter table without updating column type via modify/3" do
+    alter =
+      {:alter, table(:posts),
+       [
+         {:modify, :category_id, %Reference{table: :categories, type: :id}, from: :id},
+         {:modify, :author_id, %Reference{table: :authors, type: :id, on_delete: :delete_all},
+          from: %Reference{table: :authors, type: :id, on_delete: :nothing}}
+       ]}
+
+    assert execute_ddl(alter) ==
+             [
+               remove_newlines("""
+               ALTER TABLE "posts"
+               ADD CONSTRAINT "posts_category_id_fkey" FOREIGN KEY ("category_id") REFERENCES "categories"("id"),
+               DROP CONSTRAINT "posts_author_id_fkey",
+               ADD CONSTRAINT "posts_author_id_fkey" FOREIGN KEY ("author_id") REFERENCES "authors"("id") ON DELETE CASCADE
+               """)
+             ]
+  end
+
   test "create index" do
     create = {:create, index(:posts, [:category_id, :permalink])}