data-migrate icon indicating copy to clipboard operation
data-migrate copied to clipboard

Add disable_ddl_transaction! to the migration template

Open pyromaniac opened this issue 2 years ago • 4 comments

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.

pyromaniac avatar Jan 30 '22 05:01 pyromaniac

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

ilyakatz avatar Jan 31 '22 18:01 ilyakatz

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

eni9889 avatar Feb 02 '22 16:02 eni9889

@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.

pyromaniac avatar Apr 12 '22 11:04 pyromaniac

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.

ilyakatz avatar Apr 13 '22 03:04 ilyakatz

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

pyromaniac avatar Jan 03 '23 01:01 pyromaniac