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

Migrator fails for systems with multiple, shared databases #14176

Open
axos88 opened this Issue Feb 24, 2014 · 13 comments

Comments

Projects
None yet
10 participants
@axos88

axos88 commented Feb 24, 2014

Hello!

I have found that the root of the problem I am describing is the method how the ActiveRecord migrator detects the migrations that need to be run.

The problem surfaces only in more complex systems, and can be more easily imagined to arise with rails, but it is independent from rails and might arise with gems as well.

Suppose the following scenario:

Two rails applications, sharing a database, while having their on database as well. (3 databases, 2 applicaitons)

In order to reduce code duplication and maintenance hell, we create a rails engine that will contain the models and migrations for the shared database.

     initializer :shared_db_append_migrations do |app|
        unless app.root.to_s.match root.to_s
          require 'shared-db-common/migration_base'

          config.paths["db/migrate"].expanded.each do |expanded_path|
            app.config.paths["db/migrate"] << expanded_path
          end
        end
      end

Both applications include the common engine and use the models from there.

The models in the engine have a superclass that tells them to use a different connection than the default one:

class Base < ActiveRecord::Base
  self.abstract_class = true

  begin
    establish_connection "shared_db_#{Rails.env}"
  rescue ActiveRecord::AdapterNotSpecified
    establish_connection "#{Rails.env}"
  end
end

The migrations in the engines likewise:

class MigrationBase < ActiveRecord::Migration
  def connection
    begin
      ActiveRecord::Base.establish_connection("shared_db_#{Rails.env}").connection
    rescue ActiveRecord::AdapterNotSpecified
      super
    end
  end
end

The engine tells the main application to include its migrations in their search path.

When running the migrations for the first application, everything runs fine, the shared database migrations are ran as well. GOOD!

When running the migrations for the seconds application, they fail. They try re-running the shared migrations.

I have found that the ActiveRecord migrator tries connecting to the main database, reading out the schema_migrations table, and trying to build a list of all migrations BEFORE connecting to the actual databases that the migrations will run on, and then going through the list, running them one by one.
ActiveRecord will save the migration state back to the main database, instead of the database they have run on.

This presents a problem because it actually ties the database state to the applicaiton, and not to the database itself. When the second application runs, it will see the migrations as not to have run, and tries rerunning them.

The correct way to implement it would be to create a list of all database migrations, order them by timestamp (btw, why are we using only second-based timestamps, and not something finer like microsecs or nanosecs to avoid collisions in the schema_migrations table? Or a timestamp and a hash of the file itself to make sure they have not been changed since they ran?), and then go through the FULL list of migrations, and look them up in the CORRECT database's schema_versions table, and run them if not present.

This is a major change in the execution way, and even though it is less effective (we need to do a lot of lookups to the schema_versions table), the time for that is negligible comparing to the time that the actual migrations run, and would fix the issue I am describing. The database state would not end up being saved to another database either.

In a nutshell.

Rails App A with DB A
Rails App B with DB B
Rails Engine S with DB S

both rails apps include the engine S, and have a different connection string named shared_db_development, shared_db_production, etc for those models.

When App A runs the migrations:

  • schema versions is looked up from DB A
  • migrations are run from Rails APP A, Rails Engine S
  • new migration state is saved back to DB A

When App B runs the migrations:

  • schema versions is looked up from DB B
  • migrations are run from Rails APP B
  • migrations appear to not have run from Engine S (becasue not present in schema versions in DB B), and are attempted to run again
  • Exception occurs.

Solution:

  • save and load schema_versions state from the database connection specified by the migration itself, not the migrator, and AcitveRecord::Base.

Please let me know what you think,
Akos Vandra

@robin850 robin850 added this to the 4.2.0 milestone Feb 25, 2014

@robin850

This comment has been minimized.

Member

robin850 commented Feb 25, 2014

(@jeremy : I take the liberty to ping you there since you're planning to make the migrations work with several databases and there are several proposals).

@axos88 axos88 added the stale label May 27, 2014

@rails-bot

This comment has been minimized.

rails-bot commented May 27, 2014

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

@robin850 robin850 removed the stale label Jul 21, 2014

@rafaelfranca rafaelfranca modified the milestones: 4.2.0, 5.0.0 Aug 18, 2014

@rails-bot rails-bot added the stale label Nov 19, 2014

@rails-bot

This comment has been minimized.

rails-bot commented Nov 19, 2014

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

@axos88

This comment has been minimized.

axos88 commented Nov 24, 2014

@sgrif sgrif removed the stale label Nov 24, 2014

@rafaelfranca rafaelfranca added the pinned label Nov 26, 2014

@blm768

This comment has been minimized.

blm768 commented Mar 15, 2016

If there's still interest in fixing this, I'm getting bitten by a similar issue. We really could use a good way to specify which database/connection holds the schema_migrations table. I don't have any real experience with Rails internals, but I might be able to play with some ideas.

@jeremy

This comment has been minimized.

Member

jeremy commented Mar 15, 2016

We use separate schema.rb for each database and specify the database connection within it. It's a bit tricky to set up, but it's possible!

Along with hacks necessary in each version of Rails, here's what a secondary schema.rb looks like. This works Rails 2 through Rails 5:

class SignalId::Schema < ActiveRecord::Schema
  def self.connection
    SignalId::Base.primary_connection
  end

  def connection
    self.class.connection
  end

  # Use secondary migrations path
  def self.migrations_path
    SignalId.root.join('db/migrate')
  end

  # Because Rails 4+ uses AR::Base.connection, we override to use our connection:
  def self.initialize_schema_migrations_table
    connection.initialize_schema_migrations_table
  end

  # Because Rails 4+ uses AR::Base.connection, we override to use our connection:
  def self.assume_migrated_upto_version(*args)
    connection.assume_migrated_upto_version *args
  end

  define :version => 20151029004509 do

    # FIXME: Not sure why this is needed, but breaks in 3.1 otherwise
    self.class.delegate = self

    # … All the normal schema dump here …
  end
end

And here's a migration, also overriding the connection to use:

class IncreasePlanColumnSize < ActiveRecord::Migration
  def self.connection
    SignalId::Base.primary_connection
  end
end

Then we add Rake tasks to coordinate schema dump/load and migrations. Here's a full rundown for Rails 4:

# Augments the given builtin task with the 37id task of the same (or given) name.
def augment(name, with = nil)
  task(name) { Rake::Task["37id:#{with || name}"].invoke }
end

unless ENV['SKIP_37ID']
  augment 'db:version'

  augment 'db:create'
  augment 'db:create:all', 'db:create'

  if Rails.env.development?
    augment 'db:create', 'db:create_test'
    augment 'db:create:all', 'db:create_test'
  end

  augment 'db:schema:load'
  augment 'db:fixtures:load'
end

Rake::Task['db:test:prepare'].enhance ['37id:db:establish_local_test_connection']

namespace '37id' do
  namespace :db do
    def with_37id_connection(config = SignalId::Database.configuration(SignalId.env))
      raise ArgumentError unless config
      original = ActiveRecord::Base.remove_connection
      ActiveRecord::Base.establish_connection(config)
      yield config
    ensure
      ActiveRecord::Base.establish_connection(original)
    end

    task :guard_production do |t|
      if Rails.env.starts_with?("beta") || Rails.env.starts_with?("production") || Rails.env.starts_with?("rollout")
        abort "Refusing to run task in the '#{Rails.env}' environment"
      end
    end

    task :establish_local_test_connection => :environment do
      # Forces SignalId::Base.primary_connection to hit 37id_test even though
      # the env appears to be development.
      SignalId.env = 'test'.inquiry
    end

    desc 'Creates the signal_id database'
    task :create => :environment do
      ActiveRecord::Tasks::DatabaseTasks.create SignalId::Database.configuration(SignalId.env)
      with_37id_connection do |config|
        ActiveRecord::Base.connection.initialize_schema_migrations_table
        ActiveRecord::Base.connection.execute("grant select on #{config['database']}.* to 37id_ro@localhost identified by ''")
      end
    end

    desc 'Creates the signal_id test database'
    task :create_test => :environment do
      ActiveRecord::Tasks::DatabaseTasks.create SignalId::Database.configuration('test')
      with_37id_connection SignalId::Database.configuration('test') do |test_config|
        ActiveRecord::Base.connection.initialize_schema_migrations_table
        ActiveRecord::Base.connection.execute("grant select on #{test_config['database']}.* to 37id_ro@localhost identified by ''")
      end
    end

    desc 'Migrates the signal_id database'
    task :migrate => :environment do
      with_37id_connection do
        ActiveRecord::Migrator.migrate("#{File.dirname(__FILE__)}/../../db/migrate", ENV['VERSION'].try(:to_i))
      end
    end

    task :'migrate:down' => :environment do
      with_37id_connection do
        ActiveRecord::Migrator.down("#{File.dirname(__FILE__)}/../../db/migrate", ENV['VERSION'].try(:to_i))
      end
    end

    desc 'Retrieves the recording_bodies schema version number'
    task :version => :environment do
      with_37id_connection do
        puts "Current 37id version: #{ActiveRecord::Migrator.current_version}"
      end
    end

    desc 'Dumps the signal_id schema'
    task :'schema:dump' => :environment do
      require 'active_record/schema_dumper'
      File.open(File.expand_path('../../../db/schema_dump.rb', __FILE__), 'w') do |file|
        ActiveRecord::SchemaDumper.dump(SignalId::Base.connection, file)
      end
    end

    desc 'Loads the signal_id schema'
    task :'schema:load' => [:environment, :guard_production] do
      load "#{File.dirname(__FILE__)}/../../db/schema.rb"
    end

    desc 'Loads the signal_id fixtures'
    task :'fixtures:load' => [:environment, :guard_production] do
      require 'signal_id/testing'
      SignalId::Testing::Fixtures.create
    end

    desc 'Sets up the signal_id database'
    task :setup => [:create, :'schema:load', :'fixtures:load']
  end
end

Note how we rely on the with_37id_connection wrapper to make many of the tasks work. That's because they're hardcoded to AR::Base.connection. So they best we can do is swap its connection out temporarily.

Lots of room here to push these capabilities up into Rails itself. The biggest one is being able to provide the connection for schema changes. IMO we could simplify considerably by having our tasks, migrations, loaders/dumpers, etc all work with a consolidated Schema. That'd default to the vanilla Rails setup: db/migrate/, db/schema.rb, and AR::B.connection, but we could provide others—and even do it automatically!

For example, we'd automatically manage dbs matching the pattern db/<name>/schema.rb. Using this structure would tell AR to migrate db/secondary/migrate/, dump/load db/secondary/schema.rb, and perform db rake tasks in secondary:db:* namespace, all against the db connection corresponding to config/database/secondary.yml.

@blm768

This comment has been minimized.

blm768 commented Mar 15, 2016

That's an interesting solution. I've seen the technique of redefining Migration::connection, but I've never seen someone subclass Schema.

For what I'm trying to do, I think the first step would just be fixing the hard-coded connection references in Migrator. I've got an app that uses Active Record but not the rest of Rails, and I'm building a Rails front-end for it. The Rails-less app has its own Migrator wrapper, and the Rails app doesn't migrate that database, which makes the Rake tasks mostly irrelevant for my application.

That could provide a good base for more significant improvements.

@jeremy

This comment has been minimized.

Member

jeremy commented Mar 15, 2016

fixing the hard-coded connection references in Migrator

👍 👍

@blm768

This comment has been minimized.

blm768 commented Mar 16, 2016

I can think of a couple of ways to handle the hard-coded connection references in Migrator. The least intrusive would be to define a #connection method which returns AR::Base.connection and can be redefined in subclasses.

EDIT: Migrator already has a #connection method! There are some places where it's not enough, though, such as in #migrate:

ActiveRecord::Base.connection_pool.with_connection do |conn|
  time = Benchmark.measure do
    exec_migration(conn, direction)
  end
end

(EDIT AGAIN: That's in Migration, not Migrator. Derp. Migrator still needs #connection.)
There has to be a reason it's using a fresh connection, right?

Then again, I think it should be possible to replace AR::Base.connection_pool with connection.pool...

@dixpac

This comment has been minimized.

Contributor

dixpac commented Mar 20, 2016

@blm768 have you started working on this solution? I would love to help you :)

@blm768

This comment has been minimized.

blm768 commented Mar 20, 2016

I've made a few tweaks in my fork. It's nothing fancy yet; I've just added an overridable #connection method to Migrator and tweaked Migration so it doesn't have a hard-coded dependency on AR::Base's connection pool. I think the next step would be figuring out how to handle the Rake tasks.

@metaskills

This comment has been minimized.

Contributor

metaskills commented Mar 22, 2016

@jeremy That's a great set of code examples. This is a shameless plug, but we wrote a simple gem that kind of wraps up a few of these features. Only tested for Rails 4 now. Hopefully sharing can expose alternate ideas and implementations. https://github.com/customink/secondbase

@rafaelfranca rafaelfranca removed this from the 5.0.0 [temp] milestone Apr 5, 2016

@maclover7

This comment has been minimized.

Member

maclover7 commented Apr 23, 2016

Opened PR #24694 to try to solve this.

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