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

Transactions can be turned off per Migration #9507

Merged

Conversation

senny
Copy link
Member

@senny senny commented Mar 1, 2013

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
Copy link
Member Author

senny commented Mar 1, 2013

@rafaelfranca @carlosantoniodasilva what do you think?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@carlosantoniodasilva
Copy link
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
Copy link
Member Author

senny commented Mar 1, 2013

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
Copy link
Member

Does it need to be an option?

@senny
Copy link
Member Author

senny commented Mar 1, 2013

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.

# end
#
# Remeber that you can still open your own transactions, even if you
# are in a Migration with <tt>self.with_transaction = false</tt>.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@carlosantoniodasilva
Copy link
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?

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.
# end
# end
#
# Remeber that you can still open your own transactions, even if you
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo Remeber => Remember

Closes rails#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.
@senny
Copy link
Member Author

senny commented Mar 5, 2013

@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


cattr_accessor :verbose
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this one?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh. Right.

rafaelfranca added a commit that referenced this pull request Mar 6, 2013
Transactions can be turned off per Migration
@rafaelfranca rafaelfranca merged commit 4ce9843 into rails:master Mar 6, 2013
@carlosantoniodasilva
Copy link
Member

👍

@Crisfole
Copy link

👍 Thanks Senny

@PhilCoggins
Copy link
Contributor

@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
Copy link
Member Author

senny commented Aug 3, 2015

@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"ALTER TYPE ... ADD VALUE .." doesn't work in migration for ENUM types (postgresql only)
7 participants