Only commit migration transaction if migration can be inserted into the DB

[josevalim]

No description provided.

[fertapric]

What do you think about naming this report_and_wait_for_schema_migrations_update or similar? It can also be splitted into:

report_migration_result(parent, {ref, value})
wait_for_schema_migrations_update(ref, value)

I'm trying to be more clear about why this "two-step" flow is required. With that goal in mind, the code above:

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, :infinity)
end

could be abstracted to:

parent = self()
result_ref = make_ref()

task = Task.async(fn -> run_maybe_in_transaction(parent, result_ref, repo, module, fun) end)

case wait_for_migration_result(task, result_ref) do
  :ok ->
    # The table with schema migrations can only be updated from the parent process
    # because it has a lock acquired on that table.
    report_schema_migrations_update(task, fn ->
      verbose_schema_migration repo, "update schema migrations", fn ->
        apply(SchemaMigration, direction, [repo, version, opts[:prefix]])
      end
    end)
  
  _ ->
    :ok
end)

Task.await(task, :infinity)

Thoughts?

[josevalim]

I like it. I pushed something similar, although I kept the try catch inline because I don't want to hide the task management into a bunch of functions and instead keep it all in one place. :)

[fertapric]

Minor nitpick: should this be moved to the top where the line import Ecto.Migrator, only: [migrated_versions: 1] was?

[samsondav]

There is an alternative approach to this, which is to use advisory locks.

I wrote a patch to Rails a couple of years ago that added this functionality to ActiveRecord. It avoids the problem of locking the schema_migrations table in a separate transaction.

That way you don't need two transactions at all, and can commit to the schema_migrations table directly from the main migration transaction.

[wojtekmach]

if we use advisory lock perhaps we can rollback the change that requires pool_size: 2 for migrations? Ref: elixir-ecto/ecto#2258

[josevalim]

@samphilipd that's nice to hear. Can you dig a link to that commit/PR please?

Although I would wait before migrating to advisory locks because we need to support the current mechanism anyway for other databases.

[samsondav]

@josevalim Both MySQL and Postgres support advisory locks. I am not sure about other databases, these are the only two that Rails guarantees safe concurrent migrations for AFAIK.

@wojtekmach it seems likely that yes, you will be able to do the migration with only one connection if you use advisory locks.

Take a look at migration.rb in ActiveRecord.

My original PR is here.

[josevalim]

Thanks for the info! There is also a discussion here and here.

[josevalim]

❤️ 💚 💙 💛 💜