Transactions can be turned off per Migration #9507

Merged
merged 2 commits into from Mar 6, 2013

5 participants

@senny
Ruby on Rails member

Closes #9483.

There are SQL Queries that can't run inside a transaction. Since
the Migrator used to wrap all Migrations inside a transaction there
was no way to run these queries within a migration.

This patch adds self.with_transaction = false to the migration to
turn transactions off when necessary.

@senny
Ruby on Rails member
@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Mar 1, 2013
activerecord/lib/active_record/migration.rb
@@ -330,6 +330,23 @@ def initialize
#
# For a list of commands that are reversible, please see
# <tt>ActiveRecord::Migration::CommandRecorder</tt>.
+ #
+ # == Transactional Migrations
+ #
+ # If the database adapter supports ddl transactions, all migrations will
+ # automatically be wrapped in a transaction. There are queries that you
+ # can't execute inside a transaction. For these situations you can turn
@carlosantoniodasilva
Ruby on Rails member
carlosantoniodasilva added a line comment Mar 1, 2013

... inside a transaction though, and for... wdyt?

@senny
Ruby on Rails member
senny added a line comment Mar 1, 2013

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Mar 1, 2013
activerecord/test/cases/migration_test.rb
assert_not Person.column_methods_hash.include?(:last_name)
+
+ migration = Class.new(ActiveRecord::Migration) {
+ self.with_transaction = false
+
+ def version; 101 end
+ def migrate(x)
+ add_column "people", "last_name", :string
+ raise 'Something broke'
+ end
+ }.new
+
+ migrator = ActiveRecord::Migrator.new(:up, [migration], 101)
+
+ e = assert_raise(StandardError) { migrator.migrate }
+
@carlosantoniodasilva
Ruby on Rails member
carlosantoniodasilva added a line comment Mar 1, 2013

✂️

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

I'm not sure if I like with_transaction... How about: use_transaction = false, or skip_transaction = true, or even skip_ddl_transaction.

@senny
Ruby on Rails member

From the wording I would go with skip_transaction but I don't like the fact that it's a negative option I would like something that has the meaning of:

  • true => with transaction.
  • false => without transactions.
@carlosantoniodasilva
Ruby on Rails member

Does it need to be an option?

@senny
Ruby on Rails member

My first implementation did not use an option but a method call like without_transaction but in the end I liked the option better because in my opinion it's easier to understand what happens.

If you don't like with_transaction I would rename it to use_transaction=false but I don't have a very strong opinion about it. If you'd like to change it let me know.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Mar 5, 2013
activerecord/lib/active_record/migration.rb
+ # == Transactional Migrations
+ #
+ # If the database adapter supports ddl transactions, all migrations will
+ # automatically be wrapped in a transaction. There are queries that you
+ # can't execute inside a transaction though, and for these situations
+ # you can turn the automatic transactions off.
+ #
+ # class ChangeEnum < ActiveRecord::Migration
+ # self.with_transaction = false
+ # def up
+ # execute "ALTER TYPE model_size ADD VALUE 'new_value'"
+ # end
+ # end
+ #
+ # Remeber that you can still open your own transactions, even if you
+ # are in a Migration with <tt>self.with_transaction = false</tt>.
@carlosantoniodasilva
Ruby on Rails member
carlosantoniodasilva added a line comment Mar 5, 2013

There's one extra space at the beginning of these lines. Also Migration can be downcased I think.

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

Seems good @senny, I'm just not sure about the naming yet. with_transaction seems the best option so far, I though about wrap_in_transaction but that doesn't look that good. Wdyt, are we good with with_transaction?

@senny senny transactional migration test-case was broken.
The cleanup commit a85625d broke the test-case.
The schema was no longer modified so there was no
way to check that the rollback actually happened.
f1241ef
@exviva exviva commented on an outdated diff Mar 5, 2013
activerecord/lib/active_record/migration.rb
+ #
+ # == Transactional Migrations
+ #
+ # If the database adapter supports ddl transactions, all migrations will
+ # automatically be wrapped in a transaction. There are queries that you
+ # can't execute inside a transaction though, and for these situations
+ # you can turn the automatic transactions off.
+ #
+ # class ChangeEnum < ActiveRecord::Migration
+ # self.with_transaction = false
+ # def up
+ # execute "ALTER TYPE model_size ADD VALUE 'new_value'"
+ # end
+ # end
+ #
+ # Remeber that you can still open your own transactions, even if you
@exviva
exviva added a line comment Mar 5, 2013

Typo Remeber => Remember

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fxn fxn commented on an outdated diff Mar 5, 2013
activerecord/CHANGELOG.md
@@ -1,5 +1,20 @@
## Rails 4.0.0 (unreleased) ##
+* Make it possible to execute migrations without a transaction even
+ if the database adapter supports ddl transactions.
@fxn
Ruby on Rails member
fxn added a line comment Mar 5, 2013

DDL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fxn fxn commented on an outdated diff Mar 5, 2013
activerecord/lib/active_record/migration.rb
@@ -330,6 +330,23 @@ def initialize
#
# For a list of commands that are reversible, please see
# <tt>ActiveRecord::Migration::CommandRecorder</tt>.
+ #
+ # == Transactional Migrations
+ #
+ # If the database adapter supports ddl transactions, all migrations will
@fxn
Ruby on Rails member
fxn added a line comment Mar 5, 2013

DDL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@senny senny transactions can be turned off per Migration.
Closes #9483.

There are SQL Queries that can't run inside a transaction. Since
the Migrator used to wrap all Migrations inside a transaction there
was no way to run these queries within a migration.

This patch adds `self.disable_ddl_transaction!` to the migration to
turn transactions off when necessary.
b337390
@senny
Ruby on Rails member

@rafaelfranca @carlosantoniodasilva I pushed an updated and rebased version:

  • We now use a macro called disable_ddl_transaction!
  • I added documentation to the migrations guide
  • I edited the copy according to the feedback comments
@rafaelfranca rafaelfranca commented on the diff Mar 6, 2013
activerecord/lib/active_record/migration.rb
+ cattr_accessor :verbose
@rafaelfranca
Ruby on Rails member
rafaelfranca added a line comment Mar 6, 2013

Why this one?

@senny
Ruby on Rails member
senny added a line comment Mar 6, 2013

it's just a glitch in the diff, you see, it was removed on the top: https://github.com/rails/rails/pull/9507/files#L1L368

@rafaelfranca
Ruby on Rails member
rafaelfranca added a line comment Mar 6, 2013

Ahhh. Right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rafaelfranca rafaelfranca merged commit 4ce9843 into rails:master Mar 6, 2013
@carlosantoniodasilva
Ruby on Rails member

👍

@Crisfole

👍 Thanks Senny

@PhilCoggins

@senny Our team came across this when trying to reduce memory consumption on a data migration for a larger table. It helped us out a lot, thank you for adding this!

We were curious why the name disable_ddl_transaction! was used, since this disables all transactions inside a migration, not just for DDL SQL statements. Perhaps you, or someone else could provide some insight on why you didn't use a more generic name like disable_transaction!.

@senny
Ruby on Rails member

@PhilCoggins this name was chosen after some brainstorming. It expressed best what most people relate with the wrapping transaction. While it does not disable "all transactions", the wrapping transaction can contain non-ddl statements. You can still have transactions inside your migration when you use disable_ddl_transaction!. disable_transaction! might be to general, one name that came up during the initial discussion was self.transactional = false. We opted for disable_ddl_transaction! to communicate the meaning that that wrapping transaction is only in place for adapters that support ddl-transactions. For example it's not there with MySQL. Hope that makes sense.

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