Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

AR::Migration#migrate: apply all migration under same DDL transaction #6768

Closed
wants to merge 1 commit into from

4 participants

@bogdan

Example: let's say that we need to apply two migration on production. First one delete unused column A. Second one does complicated data migration that failed during migration. In this case source code will be rollback to previous working version, but only the last migration got reverted. Production will run previous working version of source code that expects column A to exists, but it is not.

The thing that ActiveRecord::Migrator rollbacks only last applied migration will create inconsistency between data and code. It could be sensitive in some cases in production mode.

This patch applies all migrations under same ddl transaction if it is supported by database, so that Database state and source code are always consistent.

@carlosantoniodasilva carlosantoniodasilva commented on the diff
activerecord/test/cases/migration_test.rb
@@ -248,6 +248,30 @@ def migrate(x); raise 'Something broke'; end
refute Person.column_methods_hash.include?(:last_name)
end
+ def test_migrtor_two_up_with_exception_and_rollback

typo: migrator

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

This looks good @bogdan, thank you! I agree we could revert back all migrations instead of just the failing one, it's common to have migrations related to each other, and this would avoid leaving the database in an inconsistent state.

Just linking to a discussion from core list, in case anyone wants to take a look: https://groups.google.com/forum/?fromgroups#!topic/rubyonrails-core/TsqOmUxMFtM

@NZKoz
Owner

Simon De Boer raised some very valuable points on the core list, hopefully he can raise those here for simplicity's sake. However in short, the current model where each migration is a single logical unit makes debugging failure much easier, and avoids implying that this is somehow 'magically safe'

I have another objection which is that this couples together changes. Remember that the bulk of migrations in a long lived project tend to not even be DDL changes, but data fixes or other jobs which need to be run to tidy up an unforeseen error. These changes are often not just concerned with the database but also other external services. Say migrating avatars from local disk to s3. Redoing those migrations because adding a unique index failed isn't very helpful.

If you have two migrations which must be run together, or not at all, then they should be a single migration.

@rosenfeld

Honestly I couldn't see any reasonable argument in all this comment. How would rolling back all changes be a bad thing if the unique index creation has failed. You're assuming it is not important but I find it super important in some cases and I'd really want to rollback everything if it has failed for some reason.

If you're concerned about "magically safe" then tell us an example of when it would not be safe and then add to the docs: "be aware that this method is not totally safe as ..."

Also, you're assuming that most migrations are data changes but in my projects most of them are actually DDL changes.

Also, there is no point in grouping different migrations in a single one just to be sure they will be run in a single transaction. This is actually a workaround for the current limitation.

People voting against this should have a better argument. The ones pointed out simply don't make sense to me.

@NZKoz
Owner

As it stands now:

  • If you want your changes run together, group them into a single migration
  • If you want them applied independently, leave them separate

If we apply this change:

  • If you want your changes run together, do nothing
  • If you want them applied independently, you are completely out of luck.

As it stands, the arguments in favor don't chin the bar for me. Barring some additional arguments I'll close this pull request.

@rosenfeld

not completely out of luck. Just run a migration at a time if that makes sense to you to have some of them migrated and others not. Or ask for some option like "outside_transaction=1"

@NZKoz
Owner

Running a migration at a time is an unrealistic suggestion, there's no tooling support for that.

We can re-evaluate a change like this if you resubmit it with an option for running them independently. However to be perfectly honest I can't see the utility of such an option in the face of simon's very valid objections.

@NZKoz NZKoz closed this
@rosenfeld

Here is Simon's comments:

Changing the schema of a RDBS and deploying the dependent code should be performed with great care, the basic concept of each migration being "whole" change keeps things tidy. Giving any impression to a developer that enclosing a bunch of migrations as a transaction creates a highly-available deploy would be a disservice. There are so many other factors that go into making HA happen. Very few sites actually require anything close to HA, those that do are going to be putting all sorts of tools around their deploy process.

The suggested code/schema upgrade should go this way, hopefully in as short a period as possible, with some sanity checks between each step:
Disable website
Dump current database
Migrate database
Update code
Re-enable website

Lets say there were 10 migrations to do and #5 failed, being in the state that 1-4 have run is where you want to be. Now you can look at why #5 failed, fix it (check it in), and continue.

This becomes particularly important if one of 1-4 were very long running statements, large import, index updating, etc. In these cases it becomes even more important that the website be shut down because the DB is going to be pinning its CPU and the user experience is going to suffer dramatically.

If the change was implemented I would suggest that it be off by default in development, where the situation of wanting the first few migrations to take and then to begin debugging is the much more normal behaviour.

@rosenfeld

Now, I really don't get why you think they are really valid concerns.

And this behavior of just closing tickets is one of the things I dislike in Rails. This is very sad after being used to much more professional and polite ways of dealing with pull requests in other open-source projects. And this is one of the reasons I don't contribute myself to Rails anymore.

@NZKoz
Owner

I'd be more than happy to review an updated pull request for this, or even reopen this one if it's easier once the issues are addressed

Leaving pull requests open forever may seem nice, but we're just fooling ourselves. Better to close and open quickly rather than have a million neglected unwatched pull requests open and have their submitters living in the dark about the state of their feature.

Sorry if you take offense at that, but we tried the "leave things open forever" approach and it doesn't work.

@rosenfeld

Okay, I got your point, not angry anymore. But as a suggestion, it would be more polite to say something like you just did before closing as this is usually not expected in oss. Something like "closing for now. Will reopen once the discussed issues are addressed."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
14 activerecord/lib/active_record/migration.rb
@@ -735,18 +735,18 @@ def migrate
running.select! { |m| yield m }
end
- running.each do |migration|
- Base.logger.info "Migrating to #{migration.name} (#{migration.version})" if Base.logger
+ begin
+ ddl_transaction do
+ running.each do |migration|
+ Base.logger.info "Migrating to #{migration.name} (#{migration.version})" if Base.logger
- begin
- ddl_transaction do
migration.migrate(@direction)
record_version_state_after_migrating(migration.version)
end
- rescue => e
- canceled_msg = Base.connection.supports_ddl_transactions? ? "this and " : ""
- raise StandardError, "An error has occurred, #{canceled_msg}all later migrations canceled:\n\n#{e}", e.backtrace
end
+ rescue => e
+ canceled_msg = Base.connection.supports_ddl_transactions? ? "this and all later" : "all"
+ raise StandardError, "An error has occurred, #{canceled_msg} migrations canceled:\n\n#{e}", e.backtrace
end
end
View
24 activerecord/test/cases/migration_test.rb
@@ -248,6 +248,30 @@ def migrate(x); raise 'Something broke'; end
refute Person.column_methods_hash.include?(:last_name)
end
+ def test_migrtor_two_up_with_exception_and_rollback

typo: migrator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ unless ActiveRecord::Base.connection.supports_ddl_transactions?
+ skip "not supported on #{ActiveRecord::Base.connection.class}"
+ end
+
+ refute Person.column_methods_hash.include?(:last_name)
+
+ valid_migration = Class.new(ActiveRecord::Migration) do
+ def migrate(x); add_column(:people, :last_name, :string); end
+ end.new('zomg1', 99)
+ invalid_migration = Class.new(ActiveRecord::Migration) do
+ def migrate(x); raise "Something broke"; end
+ end.new('zomg2', 100)
+
+ migrator = ActiveRecord::Migrator.new(:up, [valid_migration, invalid_migration], 100)
+
+ e = assert_raise(StandardError) { migrator.migrate }
+
+ assert_equal "An error has occurred, this and all later migrations canceled:\n\nSomething broke", e.message
+
+ Person.reset_column_information
+ refute Person.column_methods_hash.include?(:last_name)
+ end
+
def test_schema_migrations_table_name
ActiveRecord::Base.table_name_prefix = "prefix_"
ActiveRecord::Base.table_name_suffix = "_suffix"
Something went wrong with that request. Please try again.