Update #6791 "AR::Migrator.bulk_migration option"

[Empact]

This update #6791 and integrates it with the disable_ddl_transaction! option that was added since the PR was first opened.

Could probably use some additional testing to handle the interplay between these two features.

[tamird]

@Empact could you rebase this?

[Empact]

Done - incidentally I noticed some more defunct ivars, a total of 4, so I removed them in a separate commit.

[tamird]

@Empact sorry about the back and forth - looks like this is already supported unless explicitly disabled by the migration?

[Empact]

@tamird No this is not currently supported - currently each migration is run in a transaction, unless disable_ddl_transaction! is called, but if you run several migrations, there's no way to cleanly roll back them all if a single migration fails.

This feature is useful for deploys in that you can ensure that no database changes occur if any of your migrations fail. This would mean you could have a truly clean rollback to the pre-deploy state, rather than have some data changes persist even if you roll back your code changes in response to a migration failure.

[tamird]

Hmm. I think I disagree with this change - it feels natural to me to expect each migration to be atomic but extending atomicity to the group of transactions that happen to run in the same invocation feels surprising and even sloppy to me. 👍 on the drive-by cleanups you found though, would be good to open a separate PR for them so that this discussion doesn't prevent their inclusion.

[Empact]

Ok closing based on your comments here, and @rafaelfranca's comment on #6791

Extracted the ivars commit to #17092

[matthewd]

This sounds quite desirable to me; if half the migrations run and then one fails, neither the previously-deployed nor currently-checked-out code can be expected to work with the resulting database. In sufficiently merge-ful conditions, there may in fact be no intermediate commit that had this set of migrations.

However, I think a migration that's explicitly opted out of a transaction should still have its wishes honoured. If we can arrange that, without too badly contorting things, then I'm 👍 to merge (and, personally, use) this option.

[Empact]

@matthewd I may take the time to extract it as a gem. Will post back here if I do.

[matthewd]

@Empact if you can fix the behaviour for migrations that have explicitly disabled the surrounding transaction, I'll merge it: it sounds desirable to me, and while that opinion doesn't seem universal, I haven't seen any objections to it being a non-default option.

[Empact]

@matthewd Does this line do what you're thinking?:
bulk_migrating = bulk_migration && runnable.none?(&:disable_ddl_transaction)

[matthewd]

I guess I was imagining that the "ideal" interpretation would be to still cluster transaction-able migrations together. So:

begin
migration_1
migration_2
commit
migration_3 (disable_ddl_transaction)
begin
migration_4
migration_5
commit

But yeah, fallback to the current behaviour sounds quite defensible... and much simpler to implement. 😄

[Empact]

We could print a message when a ddl-disabled migration leads to the run being run individually.

[bogdan]

+1, @Empact thanks for renewing this one

[pingortle]

Does anyone have plans to follow up on this? If not, perhaps we should close the issue.