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

Add disable_ddl_transaction! to the migration template #208

Closed
wants to merge 1 commit into from
Closed

Add disable_ddl_transaction! to the migration template #208

wants to merge 1 commit into from

Conversation

pyromaniac
Copy link

Most of the data migrations don't want to be wrapped in a transaction, the changes on each record normally should be applied immediately. It is easy to forget about the fact that every migration is wrapped within a transaction. And not it is simple to drop this statement if not needed in a particular case.

Most of the data migrations don't want to be wrapped in a transaction, the changes on each record normally should be applied immediately. It is easy to forget about the fact that every migration is wrapped within a transaction. And not it is simple to drop this statement if not needed in a particular case.
@ilyakatz
Copy link
Owner

Hi @pyromaniac, thanks for the suggestion. I'm not sure that it's necessarily a good idea just to add this point blank. I can see instances where a bunch of data is added in bulk (eg, 100 rows) and if it fails in the middle, adding it back in can cause issues. However, this does seem like a reasonable default for some applications. What do you think abut adding this as a default config, something like

config.enable_ddl_transactions = true

Where it's true by default, but can be set to false

@eni9889
Copy link

eni9889 commented Feb 2, 2022

@ilyakatz that makes sense to me. Any chance you could add that?

@pyromaniac
Copy link
Author

@ilyakatz frankly, I don't see it as a breaking change at all, I don't see any reason to make the gem more complex with new config option in this particular case. It is always easy to remove the statement from the generated migration if needed rather that keep in mind adding it every time. In my practice there were 0 cases when data migration was supposed to be wrapped within a transaction.

@ilyakatz
Copy link
Owner

I'm trying to be mindful of the current users of the gem. It has been downloaded 8M times so probably means that it's used by thousands of teams so adding a change in behavior is not ideal. Adding a config is a pretty small price to pay avoiding surprises, in my opinion.

@pyromaniac
Copy link
Author

You know what? This one is elegantly solved in #232 and no users harmed :) Thanks for a great gem again!

@pyromaniac pyromaniac closed this Jan 3, 2023
@pyromaniac pyromaniac deleted the patch-1 branch January 3, 2023 01:36
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 this pull request may close these issues.

3 participants