Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Do not modify column type while modifying fk constraint #644

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Do not modify column type while modifying fk constraint #644

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
70a375f
Do not modify column type while modifying fk constraint
milmazz Oct 17, 2024
475cf8c
avoid alter column type on modify if not needed
milmazz Oct 17, 2024
fc2e173
should be smarter now?
milmazz Oct 18, 2024
daeed39
fix syntax error bug when setting default value
milmazz Oct 21, 2024
79e6479
minor refactor
milmazz Oct 21, 2024
609d4c1
Update docs and add a warning block for this function
milmazz Oct 21, 2024
6b90290
minor doc update
milmazz Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 27 additions & 2 deletions integration_test/sql/migration.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,21 +48,29 @@ defmodule Ecto.Integration.MigrationTest do
def up do
create table(:alter_col_migration) do
add :from_null_to_not_null, :integer
add :another_from_null_to_not_null, :string
add :from_not_null_to_null, :integer, null: false

add :from_default_to_no_default, :integer, default: 0
add :from_no_default_to_default, :integer

add :another_from_default_to_no_default, :integer, default: 0
add :another_from_no_default_to_default, :integer
end

alter table(:alter_col_migration) do
modify :from_null_to_not_null, :string, null: false
modify :another_from_null_to_not_null, :string, null: false, from: {:string, null: true}
modify :from_not_null_to_null, :string, null: true

modify :from_default_to_no_default, :integer, default: nil
modify :from_no_default_to_default, :integer, default: 0

modify :another_from_default_to_no_default, :integer, default: nil, from: {:integer, default: 0}
modify :another_from_no_default_to_default, :integer, default: 0, from: {:integer, default: nil}
end

execute "INSERT INTO alter_col_migration (from_null_to_not_null) VALUES ('foo')"
execute "INSERT INTO alter_col_migration (from_null_to_not_null, another_from_null_to_not_null) VALUES ('foo', 'baz')"
end

def down do
Expand Down Expand Up @@ -132,12 +140,21 @@ defmodule Ecto.Integration.MigrationTest do
add :alter_fk_user_id, :id
end

create table(:alter_fk_comments) do
add :alter_fk_user_id, references(:alter_fk_users)
end

alter table(:alter_fk_posts) do
modify :alter_fk_user_id, references(:alter_fk_users, on_delete: :nilify_all)
modify :alter_fk_user_id, references(:alter_fk_users, on_delete: :nilify_all), from: {:id, null: true}
end

alter table(:alter_fk_comments) do
modify :alter_fk_user_id, references(:alter_fk_users, on_delete: :nilify_all), from: references(:alter_fk_users, on_delete: :nothing)
end
end

def down do
drop table(:alter_fk_comments)
drop table(:alter_fk_posts)
drop table(:alter_fk_users)
end
Expand Down Expand Up @@ -555,12 +572,18 @@ defmodule Ecto.Integration.MigrationTest do

assert ["foo"] ==
PoolRepo.all from p in "alter_col_migration", select: p.from_null_to_not_null
assert ["baz"] ==
PoolRepo.all from p in "alter_col_migration", select: p.another_from_null_to_not_null
assert [nil] ==
PoolRepo.all from p in "alter_col_migration", select: p.from_not_null_to_null
assert [nil] ==
PoolRepo.all from p in "alter_col_migration", select: p.from_default_to_no_default
assert [nil] ==
PoolRepo.all from p in "alter_col_migration", select: p.another_from_default_to_no_default
assert [0] ==
PoolRepo.all from p in "alter_col_migration", select: p.from_no_default_to_default
assert [0] ==
PoolRepo.all from p in "alter_col_migration", select: p.another_from_no_default_to_default

query = "INSERT INTO `alter_col_migration` (\"from_not_null_to_null\") VALUES ('foo')"
assert catch_error(PoolRepo.query!(query))
Expand Down Expand Up @@ -596,8 +619,10 @@ defmodule Ecto.Integration.MigrationTest do
assert [id] = PoolRepo.all from p in "alter_fk_users", select: p.id

PoolRepo.insert_all("alter_fk_posts", [[alter_fk_user_id: id]])
PoolRepo.insert_all("alter_fk_comments", [[alter_fk_user_id: id]])
PoolRepo.delete_all("alter_fk_users")
assert [nil] == PoolRepo.all from p in "alter_fk_posts", select: p.alter_fk_user_id
assert [nil] == PoolRepo.all from p in "alter_fk_comments", select: p.alter_fk_user_id

:ok = down(PoolRepo, num, AlterForeignKeyOnDeleteMigration, log: false)
end
Expand Down
94 changes: 73 additions & 21 deletions lib/ecto/adapters/postgres/connection.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ",
milmazz marked this conversation as resolved.
Show resolved Hide resolved
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 == 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,
Comment on lines +1603 to +1606
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar behavior is observed when doing something like:

 modify :account_id, :binary_id, null: false, from: {:binary_id, null: true}

The SQL syntax produced is this one:

ALTER TABLE "my_table"
  ALTER COLUMN "account_id" TYPE uuid,
  ALTER COLUMN "account_id" SET NOT NULL;

And that's producing way more AccessExclusiveLock than the alternative: ALTER TABLE "my_table" ALTER COLUMN "account_id" SET NOT NULL;

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}),
Expand All @@ -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 ->
[]
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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: []
Expand Down
43 changes: 36 additions & 7 deletions lib/ecto/migration.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1273,13 +1273,42 @@ 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 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.
>
> We have considered changing the column type even when it is not needed
> could lead to undesirable locks, that's why, at least in the PostgreSQL
> adapter, if you provide the option `:from`, and the column type matches,
> we will skip updating it.
>
> Examples
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
> Examples
> 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 will offer two benefits, apart from being a reversible migration,
> at least in the PostgreSQL adapter, if the column type remains the same, the column
> type update will be skipped.

## Examples

Expand Down
Loading