Skip to content
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

Rails 5.2.0 migration fails with departure #31

Closed
aqeelvn opened this issue Apr 27, 2018 · 5 comments · Fixed by #32
Closed

Rails 5.2.0 migration fails with departure #31

aqeelvn opened this issue Apr 27, 2018 · 5 comments · Fixed by #32

Comments

@aqeelvn
Copy link

aqeelvn commented Apr 27, 2018

Context

Migration with departure in rails 5.2 fails. This PR has tried to add support to 5.2 but a new issue is noticed.

Issue

Here is a sample error log when doing a simple migration with departure:

Altering `my_database`.`test_tables`...
Creating new table...
Created new table my_database._test_tables_new OK.
Altering new table...
Altered `my_database`.`_test_tables_new` OK.
2018-04-25T16:50:04 Creating triggers...
2018-04-25T16:50:04 Created triggers OK.
2018-04-25T16:50:04 Copying approximately 1 rows...
2018-04-25T16:50:05 Copied rows OK.
2018-04-25T16:50:05 Analyzing new table...
2018-04-25T16:50:05 Swapping tables...
2018-04-25T16:50:05 Swapped original and new tables OK.
2018-04-25T16:50:05 Dropping old table...
2018-04-25T16:50:05 Dropped old table `my_database`.`_test_tables_old` OK.
2018-04-25T16:50:05 Dropping triggers...
2018-04-25T16:50:05 Dropped triggers OK.
# Event  Count
# ====== =====
# INSERT     1
Successfully altered `my_database`.`test_tables`.

   -> 0.8832s
== 20180425111712 AddColumn3ToTestTable: migrated (0.8833s) ===================

rails aborted!
Mysql2::Error: MySQL client is not connected

Reason for failing

This issue is introduced by this rails commit

 def with_advisory_lock
  lock_id = generate_migrator_advisory_lock_id
  connection = Base.connection
  got_lock = connection.get_advisory_lock(lock_id)
  raise ConcurrentMigrationError unless got_lock
  load_migrated # reload schema_migrations to be sure it wasn't changed by another process before we got the lock
  yield
ensure
  if got_lock && !connection.release_advisory_lock(lock_id)
    raise ConcurrentMigrationError.new(
      ConcurrentMigrationError::RELEASE_LOCK_FAILED_MESSAGE
    )
  end
end

Comparison with previous rails (5.1.6)

A simplified logic of how migration runs in rails 5.1.6:

  • create new connection to database (connection_1)
  • obtain database lock with connection_1
  • run migration
  • create new connection to database (connection_2
  • release lock with connection_2

In rails 5.2.0 this is slightly changed to:

  • create new connection to database (connection_1)
  • obtain database lock with connection_1
  • run migration
  • release lock with same connection_1

Now when migration is run with departure's percona adapter, the first connection_1 always gets disconnected, because reconnect_with_percona does a new establish_connection, and the existing default connection (connection_1) will be disconnected and replaced with percona_connection under the same connection_specification_name (which is "primary"). Because rails now re-uses the connection object created before migration, this will raise exception because it was disconnected for percona.

Related rails issue:

rails/rails#32622

@sauloperez
Copy link

Thanks a lot @aqeelvn and Welcome to Departure! 🎉

I just skimmed through your (awesome) description and I'll need to go through your PR some other day. I hope you understand.

@wyhaines
Copy link

👀

@wyhaines
Copy link

@departurerb/core-maintainers Ben, I recall a couple months ago you said that where you work, you were testing Rails 5.2 support? Is there a PR? What is the status of that?

@benlangfeld
Copy link
Member

@wyhaines I'm just getting back to this now after having my attention pulled away for several months on something else. I hope to have something sorted out by the end of the year.

@saulm
Copy link

saulm commented Feb 8, 2019

@aqeelvn @sauloperez @wyhaines @benlangfeld

I can confirm this solution works and that @aqeelvn explanation is right, I wasn't able to replicate the issue with the specs, but I've created a dummy rails app with a single migration, and here are two builds:

If someone can guide me into how to replicate this in the gem's test suite I'll do it.

Thanks in advance

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants