From 03676b1e6f4189b5659c2c4a63b708e71923799c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 7 Nov 2018 12:46:47 +0100 Subject: [PATCH 01/11] Only commit migration transaction if migration can be inserted into the DB --- integration_test/sql/migrator.exs | 71 +++++++++++++++++++++----- lib/ecto/migrator.ex | 83 ++++++++++++++++++------------- 2 files changed, 107 insertions(+), 47 deletions(-) diff --git a/integration_test/sql/migrator.exs b/integration_test/sql/migrator.exs index 7fcd146c..32a80276 100644 --- a/integration_test/sql/migrator.exs +++ b/integration_test/sql/migrator.exs @@ -4,7 +4,7 @@ defmodule Ecto.Integration.MigratorTest do use Ecto.Integration.Case import Support.FileHelpers - import Ecto.Migrator, only: [migrated_versions: 1] + import ExUnit.CaptureLog alias Ecto.Integration.PoolRepo alias Ecto.Migration.SchemaMigration @@ -14,15 +14,38 @@ defmodule Ecto.Integration.MigratorTest do :ok end + defmodule AnotherSchemaMigration do + use Ecto.Migration + + def change do + execute PoolRepo.create_prefix("bad_schema_migrations"), + PoolRepo.drop_prefix("bad_schema_migrations") + + create table(:schema_migrations, prefix: "bad_schema_migrations") do + add :version, :string + add :inserted_at, :integer + end + end + end + + defmodule BrokenLinkMigration do + use Ecto.Migration + + def change do + Task.start_link(fn -> raise "oops" end) + Process.sleep(:infinity) + end + end + defmodule GoodMigration do use Ecto.Migration def up do - :ok + create table(:good_migration) end def down do - :ok + drop table(:good_migration) end end @@ -36,18 +59,14 @@ defmodule Ecto.Integration.MigratorTest do import Ecto.Migrator - test "schema migration" do - up(PoolRepo, 30, GoodMigration, log: false) - - [migration] = PoolRepo.all(SchemaMigration) - assert migration.version == 30 - assert migration.inserted_at - end - test "migrations up and down" do assert migrated_versions(PoolRepo) == [] assert up(PoolRepo, 31, GoodMigration, log: false) == :ok + [migration] = PoolRepo.all(SchemaMigration) + assert migration.version == 31 + assert migration.inserted_at + assert migrated_versions(PoolRepo) == [31] assert up(PoolRepo, 31, GoodMigration, log: false) == :already_up assert migrated_versions(PoolRepo) == [31] @@ -57,10 +76,37 @@ defmodule Ecto.Integration.MigratorTest do assert migrated_versions(PoolRepo) == [] end - test "bad migration" do + test "does not commit migration if insert into schema migration fails" do + # First we create a new schema migration table in another prefix + assert up(PoolRepo, 33, AnotherSchemaMigration, log: false) == :ok + assert migrated_versions(PoolRepo) == [33] + + assert capture_log(fn -> + catch_error(up(PoolRepo, 34, GoodMigration, log: false, prefix: "bad_schema_migrations")) + catch_error(PoolRepo.all("good_migration")) + catch_error(PoolRepo.all("good_migration", prefix: "bad_schema_migrations")) + end) =~ "Could not update schema migrations" + + assert down(PoolRepo, 33, AnotherSchemaMigration, log: false) == :ok + end + + test "bad execute migration" do assert catch_error(up(PoolRepo, 31, BadMigration, log: false)) end + test "broken link migration" do + Process.flag(:trap_exit, true) + + assert capture_log(fn -> + {:ok, pid} = Task.start_link(fn -> up(PoolRepo, 31, BrokenLinkMigration, log: false) end) + assert_receive {:EXIT, ^pid, _} + end) =~ "oops" + + assert capture_log(fn -> + catch_exit(up(PoolRepo, 31, BrokenLinkMigration, log: false)) + end) =~ "oops" + end + test "run up to/step migration" do in_tmp fn path -> create_migration(47) @@ -140,7 +186,6 @@ defmodule Ecto.Integration.MigratorTest do defmodule #{module} do use Ecto.Migration - def up do update &[#{num}|&1] end diff --git a/lib/ecto/migrator.ex b/lib/ecto/migrator.ex index 2481f9b7..3a6dc126 100644 --- a/lib/ecto/migrator.ex +++ b/lib/ecto/migrator.ex @@ -108,21 +108,12 @@ defmodule Ecto.Migrator do end defp do_up(repo, version, module, opts) do - run_maybe_in_transaction(repo, module, fn -> + async_migrate_maybe_in_transaction(repo, version, module, :up, opts, fn -> attempt(repo, version, module, :forward, :up, :up, opts) || attempt(repo, version, module, :forward, :change, :up, opts) || {:error, Ecto.MigrationError.exception( "#{inspect module} does not implement a `up/0` or `change/0` function")} end) - |> case do - :ok -> - verbose_schema_migration repo, "update schema migrations", fn -> - SchemaMigration.up(repo, version, opts[:prefix]) - end - :ok - error -> - error - end end @doc """ @@ -153,41 +144,65 @@ defmodule Ecto.Migrator do end defp do_down(repo, version, module, opts) do - run_maybe_in_transaction(repo, module, fn -> + async_migrate_maybe_in_transaction(repo, version, module, :down, opts, fn -> attempt(repo, version, module, :forward, :down, :down, opts) || attempt(repo, version, module, :backward, :change, :down, opts) || {:error, Ecto.MigrationError.exception( "#{inspect module} does not implement a `down/0` or `change/0` function")} end) - |> case do - :ok -> - verbose_schema_migration repo, "update schema migrations", fn -> - SchemaMigration.down(repo, version, opts[:prefix]) - end - :ok - error -> - error - end end - defp run_maybe_in_transaction(repo, module, fun) do - fn -> do_run_maybe_in_transaction(repo, module, fun) end - |> Task.async() - |> Task.await(:infinity) + defp async_migrate_maybe_in_transaction(repo, version, module, direction, opts, fun) do + parent = self() + ref = make_ref() + + %{pid: pid} = task = + Task.async(fn -> run_maybe_in_transaction(parent, ref, repo, module, fun) end) + + try do + receive do + {^ref, :ok} -> + verbose_schema_migration repo, "update schema migrations", fn -> + apply(SchemaMigration, direction, [repo, version, opts[:prefix]]) + end + + {^ref, _} -> + :ok + + {:EXIT, ^pid, _} -> + :ok + end + catch + kind, error -> + Task.shutdown(task, :brutal_kill) + :erlang.raise(kind, error, System.stacktrace()) + else + _ -> + send(task.pid, ref) + Task.await(task) + end end - defp do_run_maybe_in_transaction(repo, module, fun) do - cond do - module.__migration__[:disable_ddl_transaction] -> - fun.() - repo.__adapter__.supports_ddl_transaction? -> - {:ok, result} = repo.transaction(fun, log: false, timeout: :infinity) - result - true -> - fun.() + defp run_maybe_in_transaction(parent, ref, repo, module, fun) do + if module.__migration__[:disable_ddl_transaction] || + not repo.__adapter__.supports_ddl_transaction? do + send_and_receive(parent, ref, fun.()) + else + {:ok, result} = + repo.transaction( + fn -> send_and_receive(parent, ref, fun.()) end, + log: false, timeout: :infinity + ) + + result end catch kind, reason -> - {kind, reason, System.stacktrace} + send_and_receive(parent, ref, {kind, reason, System.stacktrace}) + end + + defp send_and_receive(parent, ref, value) do + send parent, {ref, value} + receive do: (^ref -> value) end defp attempt(repo, version, module, direction, operation, reference, opts) do From 14e8d157486bd96efb6dc6d8d80872dfb3be9409 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 7 Nov 2018 13:44:29 +0100 Subject: [PATCH 02/11] Update MySQL version on CI --- .travis.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index ea42fa9f..29122d7b 100644 --- a/.travis.yml +++ b/.travis.yml @@ -6,9 +6,9 @@ otp_release: addons: apt: packages: - - mysql-server-5.6 - - mysql-client-core-5.6 - - mysql-client-5.6 + - mysql-server-5.7 + - mysql-client-core-5.7 + - mysql-client-5.7 before_install: - sudo service postgresql stop - sudo apt-get -y -qq --purge remove postgresql libpq-dev libpq5 postgresql-client-common postgresql-common From 2f433048519296e4475b5bd54a18121c5bb0f0b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 7 Nov 2018 13:58:24 +0100 Subject: [PATCH 03/11] Add trusty source --- .travis.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.travis.yml b/.travis.yml index 29122d7b..8c7cd2aa 100644 --- a/.travis.yml +++ b/.travis.yml @@ -5,6 +5,8 @@ otp_release: - 20.2 addons: apt: + sources: + - mysql-5.7-trusty packages: - mysql-server-5.7 - mysql-client-core-5.7 From 41db8679998bb6fbd9a080ebbb0cca5d76a74010 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 7 Nov 2018 14:12:44 +0100 Subject: [PATCH 04/11] Attempt without version numbers --- .travis.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index 8c7cd2aa..0484235c 100644 --- a/.travis.yml +++ b/.travis.yml @@ -8,9 +8,9 @@ addons: sources: - mysql-5.7-trusty packages: - - mysql-server-5.7 - - mysql-client-core-5.7 - - mysql-client-5.7 + - mysql-server + - mysql-client-core + - mysql-client before_install: - sudo service postgresql stop - sudo apt-get -y -qq --purge remove postgresql libpq-dev libpq5 postgresql-client-common postgresql-common From 7afcbdc444df60cc52df97399ed10a663f736a54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 7 Nov 2018 14:23:16 +0100 Subject: [PATCH 05/11] Let's try without client core --- .travis.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 0484235c..f5c73f0f 100644 --- a/.travis.yml +++ b/.travis.yml @@ -9,7 +9,6 @@ addons: - mysql-5.7-trusty packages: - mysql-server - - mysql-client-core - mysql-client before_install: - sudo service postgresql stop From de1a0e865692f4b19a6216bb8d11d6617bb246ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 7 Nov 2018 14:23:55 +0100 Subject: [PATCH 06/11] Await infinity for migration task --- lib/ecto/migrator.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ecto/migrator.ex b/lib/ecto/migrator.ex index 3a6dc126..b5cac00d 100644 --- a/lib/ecto/migrator.ex +++ b/lib/ecto/migrator.ex @@ -179,7 +179,7 @@ defmodule Ecto.Migrator do else _ -> send(task.pid, ref) - Task.await(task) + Task.await(task, :infinity) end end From 7010b20ddfa6000b03ff1786bca756910d9bfca5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 7 Nov 2018 14:34:32 +0100 Subject: [PATCH 07/11] Try to restart sql --- .travis.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.travis.yml b/.travis.yml index f5c73f0f..527d8b61 100644 --- a/.travis.yml +++ b/.travis.yml @@ -18,6 +18,9 @@ before_install: - wget --quiet -O - http://apt.postgresql.org/pub/repos/apt/ACCC4CF8.asc | sudo apt-key add - - sudo apt-get update -qq - sudo apt-get -y -o Dpkg::Options::=--force-confdef -o Dpkg::Options::="--force-confnew" install postgresql-$PGVERSION postgresql-contrib-$PGVERSION +after_install: + - sudo mysql_upgrade -u root -p --force + - sudo service mysqld --full-restart sudo: required dist: trusty cache: apt From 154224025a6bd5b5427953a59342ece6f46c8683 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 7 Nov 2018 15:36:28 +0100 Subject: [PATCH 08/11] There is no after_install, use before_script --- .travis.yml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index 527d8b61..fe407dc4 100644 --- a/.travis.yml +++ b/.travis.yml @@ -18,9 +18,6 @@ before_install: - wget --quiet -O - http://apt.postgresql.org/pub/repos/apt/ACCC4CF8.asc | sudo apt-key add - - sudo apt-get update -qq - sudo apt-get -y -o Dpkg::Options::=--force-confdef -o Dpkg::Options::="--force-confnew" install postgresql-$PGVERSION postgresql-contrib-$PGVERSION -after_install: - - sudo mysql_upgrade -u root -p --force - - sudo service mysqld --full-restart sudo: required dist: trusty cache: apt @@ -48,6 +45,8 @@ before_script: - echo "host all postgrex_md5_pw 127.0.0.1/32 md5" | sudo tee -a /etc/postgresql/$PGVERSION/main/pg_hba.conf - echo "host all postgrex_cleartext_pw 127.0.0.1/32 password" | sudo tee -a /etc/postgresql/$PGVERSION/main/pg_hba.conf - sudo service postgresql restart + - sudo mysql_upgrade + - sudo service mysqld --full-restart notifications: recipients: - jose.valim@plataformatec.com.br From 77a90ffa4520e307a6f054332f0465ed73cbf29e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 7 Nov 2018 15:52:19 +0100 Subject: [PATCH 09/11] Change mysql service name --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index fe407dc4..418f5324 100644 --- a/.travis.yml +++ b/.travis.yml @@ -46,7 +46,7 @@ before_script: - echo "host all postgrex_cleartext_pw 127.0.0.1/32 password" | sudo tee -a /etc/postgresql/$PGVERSION/main/pg_hba.conf - sudo service postgresql restart - sudo mysql_upgrade - - sudo service mysqld --full-restart + - sudo service mysql --full-restart notifications: recipients: - jose.valim@plataformatec.com.br From 9cc18c7d821a34a6d91a6a9d45579a74ccd55708 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 7 Nov 2018 16:04:37 +0100 Subject: [PATCH 10/11] Include @fertapric suggestions --- lib/ecto/migrator.ex | 46 ++++++++++++++++++++++---------------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/lib/ecto/migrator.ex b/lib/ecto/migrator.ex index b5cac00d..f0657a68 100644 --- a/lib/ecto/migrator.ex +++ b/lib/ecto/migrator.ex @@ -155,31 +155,31 @@ defmodule Ecto.Migrator do defp async_migrate_maybe_in_transaction(repo, version, module, direction, opts, fun) do parent = self() ref = make_ref() + task = Task.async(fn -> run_maybe_in_transaction(parent, ref, repo, module, fun) end) + + if migrated_successfully?(ref, task) do + try do + # The table with schema migrations can only be updated from + # the parent process because it has a lock on the table + verbose_schema_migration repo, "update schema migrations", fn -> + apply(SchemaMigration, direction, [repo, version, opts[:prefix]]) + end + catch + kind, error -> + Task.shutdown(task, :brutal_kill) + :erlang.raise(kind, error, System.stacktrace()) + end + end - %{pid: pid} = task = - Task.async(fn -> run_maybe_in_transaction(parent, ref, repo, module, fun) end) - - try do - receive do - {^ref, :ok} -> - verbose_schema_migration repo, "update schema migrations", fn -> - apply(SchemaMigration, direction, [repo, version, opts[:prefix]]) - end - - {^ref, _} -> - :ok + send(task.pid, ref) + Task.await(task, :infinity) + end - {:EXIT, ^pid, _} -> - :ok - end - catch - kind, error -> - Task.shutdown(task, :brutal_kill) - :erlang.raise(kind, error, System.stacktrace()) - else - _ -> - send(task.pid, ref) - Task.await(task, :infinity) + defp migrated_successfully?(ref, %{pid: pid}) do + receive do + {^ref, :ok} -> true + {^ref, _} -> false + {:EXIT, ^pid, _} -> false end end From affde38413878d1abdc2dd81411d50b554d7ea3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 7 Nov 2018 16:24:25 +0100 Subject: [PATCH 11/11] Nits --- integration_test/sql/migrator.exs | 3 +-- lib/ecto/migrator.ex | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/integration_test/sql/migrator.exs b/integration_test/sql/migrator.exs index 32a80276..df3200b6 100644 --- a/integration_test/sql/migrator.exs +++ b/integration_test/sql/migrator.exs @@ -5,6 +5,7 @@ defmodule Ecto.Integration.MigratorTest do import Support.FileHelpers import ExUnit.CaptureLog + import Ecto.Migrator alias Ecto.Integration.PoolRepo alias Ecto.Migration.SchemaMigration @@ -57,8 +58,6 @@ defmodule Ecto.Integration.MigratorTest do end end - import Ecto.Migrator - test "migrations up and down" do assert migrated_versions(PoolRepo) == [] assert up(PoolRepo, 31, GoodMigration, log: false) == :ok diff --git a/lib/ecto/migrator.ex b/lib/ecto/migrator.ex index f0657a68..d899b475 100644 --- a/lib/ecto/migrator.ex +++ b/lib/ecto/migrator.ex @@ -157,7 +157,7 @@ defmodule Ecto.Migrator do ref = make_ref() task = Task.async(fn -> run_maybe_in_transaction(parent, ref, repo, module, fun) end) - if migrated_successfully?(ref, task) do + if migrated_successfully?(ref, task.pid) do try do # The table with schema migrations can only be updated from # the parent process because it has a lock on the table @@ -175,7 +175,7 @@ defmodule Ecto.Migrator do Task.await(task, :infinity) end - defp migrated_successfully?(ref, %{pid: pid}) do + defp migrated_successfully?(ref, pid) do receive do {^ref, :ok} -> true {^ref, _} -> false