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.

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

Make migrations concurrent safe (using advisory locks) #22122

Merged
merged 1 commit into from Oct 30, 2015

Conversation

Projects
None yet
7 participants
@samphilipd
Contributor

samphilipd commented Oct 29, 2015

  • Works on Postgres and Mysql
  • Uses advisory locks because of two important properties:
    1. The can be obtained outside of the context of a transaction
    2. They are automatically released when the session ends, so if a
      migration process crashed for whatever reason the lock is not left
      open perpetually
  • Adds get_advisory_lock and release_advisory_lock methods to database
    adapters that take generic strings
  • Attempting to run a migration while another one is in process will
    raise a ConcurrentMigrationError instead of attempting to run in
    parallel with undefined behavior. This could be rescued and
    the migration could exit cleanly instead. Perhaps as a configuration
    option?
@rails-bot

This comment has been minimized.

rails-bot commented Oct 29, 2015

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (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.

@samphilipd samphilipd force-pushed the samphilipd:sam/manual_locking_on_schema_migrations branch Oct 29, 2015

@samphilipd

This comment has been minimized.

Contributor

samphilipd commented Oct 29, 2015

Improves on this previous implementation #22103

@samphilipd samphilipd force-pushed the samphilipd:sam/manual_locking_on_schema_migrations branch Oct 29, 2015

@schneems

This comment has been minimized.

Member

schneems commented Oct 29, 2015

Interesting idea. Is this a pain point that you've run into before? Are we protected a little by default by the databases already? For example I would have assumed postgres would use a locking structure to prevent duplicate changes to the same table.

@samphilipd samphilipd force-pushed the samphilipd:sam/manual_locking_on_schema_migrations branch Oct 29, 2015

@schneems

This comment has been minimized.

Member

schneems commented Oct 29, 2015

@rails-bot rails-bot assigned sgrif and unassigned schneems Oct 29, 2015

@sgrif

View changes

activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb Outdated
def release_advisory_lock(key)
key = transform_lock_key(key)
execute("select RELEASE_LOCK('#{key}')")

This comment has been minimized.

@sgrif

sgrif Oct 29, 2015

Member

Can we be consistent with capitalization here?

This comment has been minimized.

@samphilipd

samphilipd Oct 29, 2015

Contributor

Noted.

@sgrif

View changes

activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb Outdated
@@ -293,6 +293,10 @@ def supports_ddl_transactions?
true
end
def supports_advisory_locks?
true # pg > 8.2

This comment has been minimized.

@sgrif

sgrif Oct 29, 2015

Member

✂️ this comment

This comment has been minimized.

@samphilipd

samphilipd Oct 29, 2015

Contributor

Noted.

@sgrif

View changes

activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb Outdated
# pg advisory args must be 31 bit ints
ea.to_i & 0x7fffffff
end
end

This comment has been minimized.

@sgrif

sgrif Oct 29, 2015

Member

Do we need to be this generic? Given that this feature is only used for migrations right now, could we just cut all of this in favor of a known integer to use as the key?

This comment has been minimized.

@samphilipd

samphilipd Oct 29, 2015

Contributor

Sure, we could use a pre-defined integer. Two reasons why I didn't:

  1. We need to mangle for MySQL anyway because advisory locks are server-wide and not scoped to a database
  2. If we implement it this way we get generic advisory lock support on the connection in our Rails apps for free.

This comment has been minimized.

@sgrif

sgrif Oct 29, 2015

Member

we get generic advisory lock support on the connection in our Rails apps for free.

Features are never free. ;)

This comment has been minimized.

@samphilipd

samphilipd Oct 29, 2015

Contributor

Hmm. I could change the advisory lock methods to be more dumb - it would then take a 64 bit Fixnum for Postgres, anything autocast to string for MySQL and we can do the mangling in the migrator - does that work better?

This comment has been minimized.

@sgrif

sgrif Oct 29, 2015

Member

Won't your first point be a problem regardless of what we do here?

We need to mangle for MySQL anyway because advisory locks are server-wide and not scoped to a database

Since this is deterministic for all Rails apps, won't there always be a conflict?

This comment has been minimized.

@samphilipd

samphilipd Oct 29, 2015

Contributor

You can mangle using Base.connection.current_database (the database name) as a salt.

This way only Rails apps trying to migrate on the same database will share a mutex.

This comment has been minimized.

@samphilipd

samphilipd Oct 29, 2015

Contributor

A cross-database compatible solution looks like this:

    # Postgres limits us to 64 bit signed integers
    # MySQL advisory locks are server-wide so we have to scope to the database
    # MIGRATOR_SALT is a a random signed integer (max 31 bits) that identifies
    # the Rails migration process as the owner of the lock.
    MIGRATOR_SALT = 2053462845
    def generate_migrator_advisory_lock_key
      db_name_hash = Zlib.crc32(Base.connection.current_database)
      MIGRATOR_SALT * db_name_hash
    end
@sgrif

View changes

activerecord/lib/active_record/migration.rb Outdated
else
super
end
end

This comment has been minimized.

@sgrif

sgrif Oct 29, 2015

Member

Do you think this would be better written as:

class ConcurrentMigrationError < MigrationError #:nodoc:
  DEFAULT_MESSAGE = "Cannot run migrations because another migration process is currently running.".freeze

  def initialize(message = DEFAULT_MESSAGE)
    super
  end
end

This comment has been minimized.

@samphilipd

samphilipd Oct 29, 2015

Contributor

Noted.

@@ -1092,10 +1088,45 @@ def pending_migrations
end
def migrated
@migrated_versions ||= Set.new(self.class.get_all_versions)
@migrated_versions || load_migrated

This comment has been minimized.

@sgrif

sgrif Oct 29, 2015

Member

This will give an uninitialized ivar warning if I'm not mistaken. Should use defined? instead.

This comment has been minimized.

@samphilipd

samphilipd Oct 29, 2015

Contributor

It's explicitly set to nil in Migrator#initialize. Do we need a defined? in this case?

This comment has been minimized.

@sgrif
@sgrif

View changes

activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb Outdated
@@ -1040,6 +1056,15 @@ def binary_to_sql(limit) # :nodoc:
end
end
# MySQL > 5.7.5 limits lock keys to 64 chars maximum, so we use an MD5

This comment has been minimized.

@sgrif

sgrif Oct 29, 2015

Member

Comments about implementation details like this tend to get out of date. Let's put this in the commit message instead (if we keep it at all, see my previous comment about not needing to be this generic)

@sgrif

This comment has been minimized.

Member

sgrif commented Oct 29, 2015

Can we add a test case for this?

@samphilipd

This comment has been minimized.

Contributor

samphilipd commented Oct 29, 2015

@schneems

Interesting idea. Is this a pain point that you've run into before?

Yup, see the issue referenced in the commit message.

Are we protected a little by default by the databases already? For example I would have assumed postgres would use a locking structure to prevent duplicate changes to the same table.

Postgres will throw an exception if you try to simultanously add a new column with the same name twice for example, but other than that there is no protection. See issue #22092 for examples of what can go wrong here.

@samphilipd samphilipd force-pushed the samphilipd:sam/manual_locking_on_schema_migrations branch Oct 29, 2015

@sgrif

View changes

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb Outdated
# locks
#
# Return true if we got the lock, otherwise false
def get_advisory_lock(key)

This comment has been minimized.

@sgrif

sgrif Oct 29, 2015

Member

Let's # :nodoc: all of these. I don't want to introduce this as public API.

@sgrif

View changes

activerecord/lib/active_record/migration.rb Outdated
# Postgres limits us to 64 bit signed integers
# MySQL advisory locks are server-wide so we have to scope to the database
# MIGRATOR_SALT is a a random signed integer (max 31 bits) that identifies
# the Rails migration process as the owner of the lock.

This comment has been minimized.

@sgrif

sgrif Oct 29, 2015

Member

Again, these sort of implementation detail comments tend to get out of sync. Let's just leave this in the commit message.

@sgrif

View changes

Gemfile.lock Outdated
@@ -340,6 +346,7 @@ DEPENDENCIES
mysql2 (>= 0.4.0)
nokogiri (>= 1.6.7.rc3)
pg (>= 0.18.0)
pry

This comment has been minimized.

@sgrif

sgrif Oct 29, 2015

Member

I don't think you meant to commit this.

This comment has been minimized.

@samphilipd

samphilipd Oct 29, 2015

Contributor

Oops ;)

@sgrif

This comment has been minimized.

Member

sgrif commented Oct 29, 2015

This looks fine, other than the remaining comments and adding a test for this.

@samphilipd samphilipd force-pushed the samphilipd:sam/manual_locking_on_schema_migrations branch Oct 30, 2015

@samphilipd

This comment has been minimized.

Contributor

samphilipd commented Oct 30, 2015

@sgrif thanks for the feedback, I addressed all your comments and added tests. Let me know when you have reviewed and if everything is good I will clean up the commit history.

@samphilipd samphilipd force-pushed the samphilipd:sam/manual_locking_on_schema_migrations branch Oct 30, 2015

@sgrif

This comment has been minimized.

Member

sgrif commented Oct 30, 2015

Good to squash & rebase

Sam Davies
Use advisory locks to prevent concurrent migrations
- Addresses issue #22092
- Works on Postgres and MySQL
- Uses advisory locks because of two important properties:
  1. The can be obtained outside of the context of a transaction
  2. They are automatically released when the session ends, so if a
  migration process crashed for whatever reason the lock is not left
  open perpetually
- Adds get_advisory_lock and release_advisory_lock methods to database
  adapters
- Attempting to run a migration while another one is in process will
  raise a ConcurrentMigrationError instead of attempting to run in
  parallel with undefined behavior. This could be rescued and
  the migration could exit cleanly instead. Perhaps as a configuration
  option?

Technical Notes
==============

The Migrator uses generate_migrator_advisory_lock_key to build the key
for the lock. In order to be compatible across multiple adapters there
are some constraints on this key.
- Postgres limits us to 64 bit signed integers
- MySQL advisory locks are server-wide so we have to scope to the
  database
- To fulfil these requirements we use a Migrator salt (a randomly
  chosen signed integer with max length of 31 bits) that identifies
  the Rails migration process as the owner of the lock. We multiply
  this salt with a CRC32 unsigned integer hash of the database name to
  get a signed 64 bit integer that can also be converted to a string
  to act as a lock key in MySQL databases.
- It is important for subsequent versions of the Migrator to use the
  same salt, otherwise different versions of the Migrator will not see
  each other's locks.

@samphilipd samphilipd force-pushed the samphilipd:sam/manual_locking_on_schema_migrations branch to 2c2a875 Oct 30, 2015

sgrif added a commit that referenced this pull request Oct 30, 2015

Merge pull request #22122 from samphilipd/sam/manual_locking_on_schem…
…a_migrations

Make migrations concurrent safe (using advisory locks)

@sgrif sgrif merged commit 6120542 into rails:master Oct 30, 2015

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@samphilipd samphilipd deleted the samphilipd:sam/manual_locking_on_schema_migrations branch Oct 30, 2015

samphilipd pushed a commit to samphilipd/rails that referenced this pull request Nov 1, 2015

pixeltrix added a commit that referenced this pull request Nov 1, 2015

@xaviershay

This comment has been minimized.

Contributor

xaviershay commented Feb 5, 2016

just wanted to say thank you for doing this - came looking for this feature to find out it already existed!

@ejholmes ejholmes referenced this pull request Apr 19, 2016

Merged

Concurrency #3

@alovak

This comment has been minimized.

Contributor

alovak commented Aug 15, 2017

@samphilipd @xaviershay guys raising exception is cool, but...

what do you do when rails app on some node fails to start? Do you add a shell script to run it (with migrations) again?

What do you think about catching ConcurrentMigrationError and trying N times with few seconds sleep until another process finishes migrations?

@mikecmpbll

This comment has been minimized.

mikecmpbll commented Dec 21, 2017

hmm. should the advisory lock key include current schema for postgres? you ought to be able to migrate multiple postgresql schemas concurrently afaik (not a pg expert).

/related influitive/apartment#506

amatriain added a commit to amatriain/feedbunch that referenced this pull request Apr 18, 2018

Monkeypatch to disable advisory locks in postgres
Since this commit:

rails/rails#22122

rails tries to acquire an advisory lock when running migrations; this is
intended to avoid the (unlikely) case of multiple migrations running
concurrently.

However this fails when the app is accessing a Postgres db through
pgbounce in transaction pooling mode (which is the only mode that gives
reasonable memory gains while allowing transactions to work correctly).
Unfortunately FeedBunch is impacted by this. This means that capistrano
deployments fail when trying to run migrations, even if no migration is
actually part of the deployment.

The current ugly fix is a monkey patch to disable advisory lock support
in the Postgres adapter. This is messy and could break other things,
I'm not sure if advisory locks are used elsewhere in Rails.

A discussion is currently underway about adding a config option to
disable the acquisiton of an advisory lock before running a migration,
see:

rails/rails#31190

If such an option is implemented this patch should be removed at once.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment