Running multiple migrations simultaneously can cause race conditions (on Postgres, probably on others) #22092

Closed
samphilipd opened this Issue Oct 27, 2015 · 17 comments

Comments

Projects
None yet
7 participants
@samphilipd
Contributor

samphilipd commented Oct 27, 2015

Currently it seems that the rake db:migrate task does not try to get any kind of exclusive 'migrator' lock on the database (at least for Postgres, I haven't tested for other databases).

This results in a problem where running multiple migrations simultaneously from different threads/machines can cause undefined behavior or migration failures. This is especially a problem with Rails deployments to multi-instance clusters.

Here is a quick 'n dirty example case where we simply add a column:

class TestDummy < ActiveRecord::Migration
  def change
    add_index :review_requests, :url_code, using: :hash, name: 'hash_index_review_requests_on_url_code'
  end
end

Now let's run this in three simultaneous threads (simulating a deploy to three instances simultaneously) and see what happens:

=>$ for run in {1..3}
do
bundle exec rake db:migrate &
done
[2] 88935
[3] 88936
[4] 88937
=>$ == 20151027212412 TestDummy: migrating ========================================
-- add_index(:review_requests, :url_code, {:using=>:hash, :name=>"hash_index_review_requests_on_url_code"})
== 20151027212412 TestDummy: migrating ========================================
-- add_index(:review_requests, :url_code, {:using=>:hash, :name=>"hash_index_review_requests_on_url_code"})
== 20151027212412 TestDummy: migrating ========================================
-- add_index(:review_requests, :url_code, {:using=>:hash, :name=>"hash_index_review_requests_on_url_code"})
   -> 1.1617s
== 20151027212412 TestDummy: migrated (1.1619s) ===============================

rake aborted!
StandardError: An error has occurred, this and all later migrations canceled:

PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint "pg_class_relname_nsp_index"
DETAIL:  Key (relname, relnamespace)=(hash_index_review_requests_on_url_code, 5605052) already exists.
: CREATE  INDEX  "hash_index_review_requests_on_url_code" ON "review_requests" USING hash ("url_code")/Users/sam/code/fosubo/fosubo-app/db/migrate/20151027212412_test_dummy.rb:3:in `change'
ActiveRecord::RecordNotUnique: PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint "pg_class_relname_nsp_index"
DETAIL:  Key (relname, relnamespace)=(hash_index_review_requests_on_url_code, 5605052) already exists.
: CREATE  INDEX  "hash_index_review_requests_on_url_code" ON "review_requests" USING hash ("url_code")
rake aborted!
StandardError: An error has occurred, this and all later migrations canceled:

PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint "pg_class_relname_nsp_index"
DETAIL:  Key (relname, relnamespace)=(hash_index_review_requests_on_url_code, 5605052) already exists.
: CREATE  INDEX  "hash_index_review_requests_on_url_code" ON "review_requests" USING hash ("url_code")/Users/sam/code/fosubo/fosubo-app/db/migrate/20151027212412_test_dummy.rb:3:in `change'
PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint "pg_class_relname_nsp_index"
DETAIL:  Key (relname, relnamespace)=(hash_index_review_requests_on_url_code, 5605052) already exists.
/Users/sam/code/fosubo/fosubo-app/db/migrate/20151027212412_test_dummy.rb:3:in `change'
ActiveRecord::RecordNotUnique: PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint "pg_class_relname_nsp_index"
DETAIL:  Key (relname, relnamespace)=(hash_index_review_requests_on_url_code, 5605052) already exists.
: CREATE  INDEX  "hash_index_review_requests_on_url_code" ON "review_requests" USING hash ("url_code")
/Users/sam/code/fosubo/fosubo-app/db/migrate/20151027212412_test_dummy.rb:3:in `change'
Tasks: TOP => db:migrate
(See full trace by running task with --trace)
/Users/sam/code/fosubo/fosubo-app/db/migrate/20151027212412_test_dummy.rb:3:in `change'
PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint "pg_class_relname_nsp_index"
DETAIL:  Key (relname, relnamespace)=(hash_index_review_requests_on_url_code, 5605052) already exists.
/Users/sam/code/fosubo/fosubo-app/db/migrate/20151027212412_test_dummy.rb:3:in `change'
Tasks: TOP => db:migrate
(See full trace by running task with --trace)

Result: disaster, two of our deploys failed.

In this particular case it wouldn't cause much damage, just some exceptions. But we can imagine scenarios where running a migration more than once could cause serious issues, for example:

class SecondTestDummy < ActiveRecord::Migration
  # John has too much money in his account because of the bug we recently fixed, make sure to reset it.
  def change
    john = User.find(1)
    john.money -= 100
    john.save!
  end
end

The expectation for migrations is that they get run only once. However, if we ran repeated the same process with the migration above, it is possible that John could end up with -200 or -300 money, rather than -100 as intended.

Personally, I can't imagine a scenario where you would EVER want to allow more than one migration to run at a time.

I propose a fix to this which would enforce that rule by wrapping each migration with exclusive table locking on the schema_migrations table. If another migration is running, the process will have to wait until it can get a lock before running.

This would solve the above case because only one migrating process would get the lock first. It would then apply the migration and add the new schema version before releasing the lock. Now a new process takes the lock and sees that the migration has already been applied, so it does nothing instead of attempting to add an index that already exists.

If someone can confirm to me this is the right approach, I would be happy to write a patch.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Oct 27, 2015

Member

Is not implementing this being too defensive? Who is running the migration, is not the developer? I can't imagine this happening without being a human error, and yes, we can try to not allow that, but I'm usually against writing code to defend against human error without evidence that this error happens a lot.

Member

rafaelfranca commented Oct 27, 2015

Is not implementing this being too defensive? Who is running the migration, is not the developer? I can't imagine this happening without being a human error, and yes, we can try to not allow that, but I'm usually against writing code to defend against human error without evidence that this error happens a lot.

@samphilipd

This comment has been minimized.

Show comment
Hide comment
@samphilipd

samphilipd Oct 27, 2015

Contributor

@rafaelfranca You are correct, in an ideal world the developer would do work to ensure only one migration process is run at the same time.

However in our case, this is is actually a common problem that we run into on a regular basis while deploying to Amazon Opsworks, where migrations are run by default on multiple machines simultaneously when deploying.

And in fact, anybody who deploys to multiple machines simultaneously and runs migrations naively risks running into this problem. It's made all the more insidious because this will be an intermittent problem. It MAY work fine many times and then bite you at random due to timing differences on the machines.

I certainly think it would be good engineering to enforce a migration lock in any case just to be sure only one process can run it. We can add a timeout in case one of the other migrations is really taking a long time (sensible default would probably be something like 5 or 10 minutes).

Contributor

samphilipd commented Oct 27, 2015

@rafaelfranca You are correct, in an ideal world the developer would do work to ensure only one migration process is run at the same time.

However in our case, this is is actually a common problem that we run into on a regular basis while deploying to Amazon Opsworks, where migrations are run by default on multiple machines simultaneously when deploying.

And in fact, anybody who deploys to multiple machines simultaneously and runs migrations naively risks running into this problem. It's made all the more insidious because this will be an intermittent problem. It MAY work fine many times and then bite you at random due to timing differences on the machines.

I certainly think it would be good engineering to enforce a migration lock in any case just to be sure only one process can run it. We can add a timeout in case one of the other migrations is really taking a long time (sensible default would probably be something like 5 or 10 minutes).

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Oct 27, 2015

Member

Is the multiple machines running the same set of migrations against the same database? If that is the case does not make sense to run the migrations in only one machine. That way you would never be in this case.

Member

rafaelfranca commented Oct 27, 2015

Is the multiple machines running the same set of migrations against the same database? If that is the case does not make sense to run the migrations in only one machine. That way you would never be in this case.

@samphilipd

This comment has been minimized.

Show comment
Hide comment
@samphilipd

samphilipd Oct 27, 2015

Contributor

Yes, they are running the same migrations against the same database.

And yes, the solution is to run it on only one machine, but there is another problem with doing this - if you run migrations on one machine and skip it on the others, the other instances are likely to restart their servers before the migration has completed resulting in an out of date schema cache.

The workaround is to do a rolling deploy, where you enable migrations on all instances, and fully complete the deploy including the migration on each instance before moving onto the next one. Actually I even wrote a gem to do this: https://github.com/fosubo/opworks_interactor

But this means that deploys are much slower than they need to be. And most people are not going to bother with this kind of solution (or even understand why it might be an issue).

Regardless, running the migration on all instances is the default mode for Rails apps deployed to Amazon Opsworks so I would imagine this is probably a fairly widespread problem.

The correct way to do migrations is to enforce isolation using database locks.

As developers we would like to have confidence that a migration will be run once and exactly once, regardless of the deployment method.

Contributor

samphilipd commented Oct 27, 2015

Yes, they are running the same migrations against the same database.

And yes, the solution is to run it on only one machine, but there is another problem with doing this - if you run migrations on one machine and skip it on the others, the other instances are likely to restart their servers before the migration has completed resulting in an out of date schema cache.

The workaround is to do a rolling deploy, where you enable migrations on all instances, and fully complete the deploy including the migration on each instance before moving onto the next one. Actually I even wrote a gem to do this: https://github.com/fosubo/opworks_interactor

But this means that deploys are much slower than they need to be. And most people are not going to bother with this kind of solution (or even understand why it might be an issue).

Regardless, running the migration on all instances is the default mode for Rails apps deployed to Amazon Opsworks so I would imagine this is probably a fairly widespread problem.

The correct way to do migrations is to enforce isolation using database locks.

As developers we would like to have confidence that a migration will be run once and exactly once, regardless of the deployment method.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Oct 27, 2015

Member

Yeah, that make sense. Your approach seems reasonable. Feel free to work on it.

Member

rafaelfranca commented Oct 27, 2015

Yeah, that make sense. Your approach seems reasonable. Feel free to work on it.

@saurabhbhatia

This comment has been minimized.

Show comment
Hide comment
@saurabhbhatia

saurabhbhatia Oct 27, 2015

@samphilipd Are you trying to run a migration through their internal recipe or a custom recipe of yours ? The reason I ask is, I do production deploys to opsworks everyweek and schema version number is the exactly the same across the instances. So my guess is, it runs it one place and replicates the schema file over other instances. It has worked out perfectly fine so far for me. Also, there is no custom recipe to do the migrations in my chef cookbook.

screen shot 2015-10-28 at 10 11 36 am

Hope this helps. Cheers!

@samphilipd Are you trying to run a migration through their internal recipe or a custom recipe of yours ? The reason I ask is, I do production deploys to opsworks everyweek and schema version number is the exactly the same across the instances. So my guess is, it runs it one place and replicates the schema file over other instances. It has worked out perfectly fine so far for me. Also, there is no custom recipe to do the migrations in my chef cookbook.

screen shot 2015-10-28 at 10 11 36 am

Hope this helps. Cheers!

samphilipd pushed a commit to samphilipd/rails that referenced this issue Oct 28, 2015

Sam Davies
Add ACCESS EXCLUSIVE table locking for migrations
- Addresses issue #22092
- Currently dependent on adapter, only postgresql supports it because
  it's the only one that supports DDL transactions
@samphilipd

This comment has been minimized.

Show comment
Hide comment
@samphilipd

samphilipd Oct 28, 2015

Contributor

@saurabhbhatia yes, we are using the default recipe. The default recipe runs migrations on all machines. You probably haven't noticed issues because your migrations complete fairly quickly.

We have some migrations for large tables (500,000+ rows) that take a long time, and this is where the race is triggered.

@rafaelfranca PR is here: #22103

Contributor

samphilipd commented Oct 28, 2015

@saurabhbhatia yes, we are using the default recipe. The default recipe runs migrations on all machines. You probably haven't noticed issues because your migrations complete fairly quickly.

We have some migrations for large tables (500,000+ rows) that take a long time, and this is where the race is triggered.

@rafaelfranca PR is here: #22103

samphilipd pushed a commit to samphilipd/rails that referenced this issue Oct 28, 2015

Sam Davies
Add ACCESS EXCLUSIVE table locking for migrations
- Addresses issue #22092
- Currently dependent on adapter, only postgresql supports it because
  it's the only one that supports DDL transactions

samphilipd pushed a commit to samphilipd/rails that referenced this issue Oct 29, 2015

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 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?

samphilipd pushed a commit to samphilipd/rails that referenced this issue Oct 29, 2015

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 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?

samphilipd pushed a commit to samphilipd/rails that referenced this issue Oct 30, 2015

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 pushed a commit to samphilipd/rails that referenced this issue Oct 30, 2015

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

This comment has been minimized.

Show comment
Hide comment
@samphilipd

samphilipd Oct 30, 2015

Contributor

@rafaelfranca this issue can be marked resolved, it was fixed in my PR here: #22122

Contributor

samphilipd commented Oct 30, 2015

@rafaelfranca this issue can be marked resolved, it was fixed in my PR here: #22122

@davetchen

This comment has been minimized.

Show comment
Hide comment
@davetchen

davetchen Nov 4, 2016

@rafaelfranca we're running into this exact problem with rake db:migrate deploying Rails 4.2.x with MySQL on a multi-instance cluster on AWS. Any chance to get this fix back-ported to the 4-2-stable branch?

@rafaelfranca we're running into this exact problem with rake db:migrate deploying Rails 4.2.x with MySQL on a multi-instance cluster on AWS. Any chance to get this fix back-ported to the 4-2-stable branch?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Nov 5, 2016

Member

No. stable branches doesn't receive new features.

Member

rafaelfranca commented Nov 5, 2016

No. stable branches doesn't receive new features.

@davetchen

This comment has been minimized.

Show comment
Hide comment
@davetchen

davetchen Nov 6, 2016

Well, for multi-instance deployments one can argue that this fix addresses a "borderline bug", and as @samphilipd said, I would also imagine that this is probably a fairly widespread problem (it did take us a while to recognize the issue).

Short of back-porting this change, can you recommend a workaround?

Well, for multi-instance deployments one can argue that this fix addresses a "borderline bug", and as @samphilipd said, I would also imagine that this is probably a fairly widespread problem (it did take us a while to recognize the issue).

Short of back-porting this change, can you recommend a workaround?

@samphilipd

This comment has been minimized.

Show comment
Hide comment
@samphilipd

samphilipd Nov 6, 2016

Contributor

@davetchen upgrade to Rails 5?

Contributor

samphilipd commented Nov 6, 2016

@davetchen upgrade to Rails 5?

@davetchen

This comment has been minimized.

Show comment
Hide comment
@davetchen

davetchen Nov 7, 2016

Good suggestion @samphilipd , but may take us a little time to get there!

In the short-term, the only other way I can think of to avoid the race condition is to explicitly wrap our data migrations with an "advisory lock", much as what you're already doing. Maybe using this gem:

https://github.com/mceachen/with_advisory_lock

Good suggestion @samphilipd , but may take us a little time to get there!

In the short-term, the only other way I can think of to avoid the race condition is to explicitly wrap our data migrations with an "advisory lock", much as what you're already doing. Maybe using this gem:

https://github.com/mceachen/with_advisory_lock

@felixbuenemann

This comment has been minimized.

Show comment
Hide comment
@felixbuenemann

felixbuenemann Jun 29, 2017

Contributor

If you need this on Rails 4.2, you can put the monkey patch from this gist into an initializer. It contains the changes from #22122 with a slight modification required because select_value in ActiveRecord 4.2 does not use typecasting. It also fixes the version requirement for MySQL which is 3.21.27 not 5.0.0.

Contributor

felixbuenemann commented Jun 29, 2017

If you need this on Rails 4.2, you can put the monkey patch from this gist into an initializer. It contains the changes from #22122 with a slight modification required because select_value in ActiveRecord 4.2 does not use typecasting. It also fixes the version requirement for MySQL which is 3.21.27 not 5.0.0.

@schickling schickling referenced this issue in prismagraphql/prisma Nov 15, 2017

Open

Database migrations #1263

0 of 14 tasks complete
@SamSaffron

This comment has been minimized.

Show comment
Hide comment
@SamSaffron

SamSaffron Nov 22, 2017

Contributor

@rafaelfranca @sgrif I kind of object to all of the contortion here and introduction of advisory locks into migrations.

I feel this is all very misguided and protecting against these kind of errors is up to the migration author not active record.

The new advisory locks have caused us extreme pain with transaction pooling in pgbouncer, since the lock is acquired outside of a transaction we end up having a lot of deploys randomly failing.

We are now left with complicated monkey patches to rip out this feature.

I disagree with it on quite a few counts:

  1. Running migrations in parallel is not something people should be doing in the first place

  2. There already exists a transaction around the migration that explodes if it is run in parallel

  3. If you are already being this paranoid and don't trust your devs to write migrations properly you probably want to run your migrations in serialized transactions anyway.

I do not see how the advisory locks here achieve anything at all, except for stopping you from doing double work that gets rolled back if you have an eclectic deploy system.

ddl_transaction(migration) do
    migration.migrate(direction)
    record_version_state_after_migrating(migration.version)
end

ActiveRecord::SchemaMigration.create!(version: version.to_s)

 pry(main)> ActiveRecord::SchemaMigration.create!(version: "xxx")
   (0.2ms)  BEGIN
  SQL (1.8ms)  INSERT INTO "schema_migrations" ("version") VALUES ('xxx') RETURNING "version"
   (0.4ms)  COMMIT
pry(main)> ActiveRecord::SchemaMigration.create!(version: "xxx")
   (0.2ms)  BEGIN
  SQL (1.0ms)  INSERT INTO "schema_migrations" ("version") VALUES ('xxx') RETURNING "version"
   (0.2ms)  ROLLBACK
ActiveRecord::RecordNotUnique: PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint "schema_migrations_pkey"
DETAIL:  Key (version)=(xxx) already exists.
: INSERT INTO "schema_migrations" ("version") VALUES ('xxx') RETURNING "version"
from /home/sam/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/rack-mini-profiler-0.10.5/lib/patches/db/pg.rb:90:in `async_exec'

So protection already exists, only rationale of adding this complex advisory lock thing is to "avoid double work that gets rolled back"

I think it is perfectly fine to rollback a transaction if you have 2 of the exact same migrations running in parallel.

At the bare minimum this new advisory lock stuff should be default off since it is broken with pgbouncer.

Contributor

SamSaffron commented Nov 22, 2017

@rafaelfranca @sgrif I kind of object to all of the contortion here and introduction of advisory locks into migrations.

I feel this is all very misguided and protecting against these kind of errors is up to the migration author not active record.

The new advisory locks have caused us extreme pain with transaction pooling in pgbouncer, since the lock is acquired outside of a transaction we end up having a lot of deploys randomly failing.

We are now left with complicated monkey patches to rip out this feature.

I disagree with it on quite a few counts:

  1. Running migrations in parallel is not something people should be doing in the first place

  2. There already exists a transaction around the migration that explodes if it is run in parallel

  3. If you are already being this paranoid and don't trust your devs to write migrations properly you probably want to run your migrations in serialized transactions anyway.

I do not see how the advisory locks here achieve anything at all, except for stopping you from doing double work that gets rolled back if you have an eclectic deploy system.

ddl_transaction(migration) do
    migration.migrate(direction)
    record_version_state_after_migrating(migration.version)
end

ActiveRecord::SchemaMigration.create!(version: version.to_s)

 pry(main)> ActiveRecord::SchemaMigration.create!(version: "xxx")
   (0.2ms)  BEGIN
  SQL (1.8ms)  INSERT INTO "schema_migrations" ("version") VALUES ('xxx') RETURNING "version"
   (0.4ms)  COMMIT
pry(main)> ActiveRecord::SchemaMigration.create!(version: "xxx")
   (0.2ms)  BEGIN
  SQL (1.0ms)  INSERT INTO "schema_migrations" ("version") VALUES ('xxx') RETURNING "version"
   (0.2ms)  ROLLBACK
ActiveRecord::RecordNotUnique: PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint "schema_migrations_pkey"
DETAIL:  Key (version)=(xxx) already exists.
: INSERT INTO "schema_migrations" ("version") VALUES ('xxx') RETURNING "version"
from /home/sam/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/rack-mini-profiler-0.10.5/lib/patches/db/pg.rb:90:in `async_exec'

So protection already exists, only rationale of adding this complex advisory lock thing is to "avoid double work that gets rolled back"

I think it is perfectly fine to rollback a transaction if you have 2 of the exact same migrations running in parallel.

At the bare minimum this new advisory lock stuff should be default off since it is broken with pgbouncer.

@samphilipd

This comment has been minimized.

Show comment
Hide comment
@samphilipd

samphilipd Nov 22, 2017

Contributor

@SamSaffron Advisory locks are always acquired outside of a transaction regardless of if you are using Postgres or pgbouncer.

I'm not sure I understand the problem here, can you explain in a little more detail?

Contributor

samphilipd commented Nov 22, 2017

@SamSaffron Advisory locks are always acquired outside of a transaction regardless of if you are using Postgres or pgbouncer.

I'm not sure I understand the problem here, can you explain in a little more detail?

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Nov 22, 2017

Member

@samphilipd see concurrent discussion in #31190 -- it's always acquired outside a transaction.. it's just only a problem when using PgBouncer (in transaction pooling mode), because when doing so, the DB-side connection that acquires the lock may not be the DB-side connection that runs any subsequent query from the same Rails-side connection, including, say, the query to release the lock.

Member

matthewd commented Nov 22, 2017

@samphilipd see concurrent discussion in #31190 -- it's always acquired outside a transaction.. it's just only a problem when using PgBouncer (in transaction pooling mode), because when doing so, the DB-side connection that acquires the lock may not be the DB-side connection that runs any subsequent query from the same Rails-side connection, including, say, the query to release the lock.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment