Make migrations concurrent-safe

[samsondav]

• Addresses issue Running multiple migrations simultaneously can cause race conditions (on Postgres, probably on others) #22092
• Makes it safe to run multiple migrations on a database simultaneously from different machines or threads.
• Currently dependent on adapter, only postgresql supports it because
  it's the only one that supports DDL transactions

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @matthewd (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[matthewd]

Hmm... this seems to be a dangerous half-solution, to me.

IMO, we need to either make it always safe to concurrently attempt migrations, or retain our current "don't do that, it might break" position. Otherwise we're just lulling people into a false sense of security.

I'm struggling to come up with non-awful strategies that'll work outside a transaction, though. 😕

[samsondav]

@matthewd When you say half-solution I assume you mean because it only works in DDL transactions for postgres?

I agree with you here. The only non-transactional solution I can see would be to have some sort of lock surrounding the entire migration process. You would have to create an extra column on the schema_migrations table called lock or something. Ensure this is nil and set to true before running, then run the migrations, then manually set it to nil again.

The danger with doing this outside of a transaction is that if the ruby process or machine crashes mid-migration we are left with a lock on the schema_migrations table that will never expire. Which is pretty awful.

My view is that if you are running migrations on a database without DDL transaction support you have already given up your right to any kind of consistency before you even started.

This should not be a reason to prevent us from improving things for databases and migration types that actually support it.

[matthewd]

This should not be a reason to prevent us from making migrations concurrent safe when the database and migration type actually support it.

Yes, I believe it is a reason to prevent us doing that -- something that works most of the time is much more dangerous than something that's known to always be unsafe. I can live with different adapters having different safety behaviours, but I think any solution would need to address migrations that have opted out of the transaction.

[samsondav]

I'm not sure it is well-known that migrations aren't concurrent-safe by default. I certainly didn't know that until I was bitten by it, and Opsworks doesn't appear to know it either because they run migrations concurrently by default on their Rails apps. Googling for concurrent or multiple migrations doesn't show up any obvious answers.

I assumed there must have been some sort of locking going on until I actually delved into the code and realized it was all basically running with scissors.

Regardless, I think you do have a valid point. What about something like:

begin
  if get_lock_on_schema_migrations # inserts something (either true or a timestamp) into the one-row column schema_migrations.lock
    run_migrations
  else
    skip_migrations
  end
ensure
  release_lock_on_schema_migrations # removes thing from schema_migrations.lock
end

It's not as clean as the lock inside a transaction but it would have the advantage of working on all database adapters and whether DDL transactions are enabled or disabled.

The only problem is if the process crashes or the instance is shut down during the migration we are left with a dangling lock on the schema_migrations table.

In that case if you aren't using transactions then your database is going to be in an inconsistent state anyway so this is probably the least of your problems.

One possible solution to this would be to quit immediately if we can't get the lock with a message saying something like 'Another migration process has a lock on this database, skipping all migrations. If you want to manually force the migration, please run rake db:migrate:unlock and then rake db:migrate again'.

Thoughts?

[samsondav]

@matthewd If you think the above solution is acceptable, I'll go ahead and implement that.

If not, let me know what other options you would consider. It would be good to take at least some step to improving the situation with concurrency in migrations because currently it's completely undefined and dangerous.

[samsondav]

@matthewd I implemented a more robust solution using advisory locks here: https://github.com/rails/rails/pull/22122/files

[samsondav]

@matthewd the other branch was already merged to Rails master, you can close this PR. Thanks for the feedback, it resulted in a much better solution in the end 👍