Idea: only disable DDL transaction for transactions that use helpers

[pirj]

That is practically achievable by providing a SafeMigration concern that users could include in their migrations.
It will:

• provide updated helpers
• set lock and statement timeouts
• disable DDL transaction

This approach would still allow having migrations that have DDL transaction turned on.

[rchoquet]

We could discuss that a bit, are you proposing to explicitly include the concern to be able to use monkey patched migration methods?
If that's the case, not sure I agree with it, it kinds of go against the philosophy of this gem, which is to avoid developers from having to understand when or why a migration is dangerous.

What do you think about: automatically disabling transaction when using non-transaction compatible methods (like index or foreign key ones), otherwise keep the default transaction.

We would need to handle cases such as

add_column :foo, :bar, :text
add_index :foo, :bar

where we could either automatically commit the transaction before the add_index, or simply let the migration fail because of ADD INDEX CONCURRENTLY being executed inside a transaction, and let the developer do it in a separate migration.

And we also need to adapt the retry mechanism to work correctly with transactional migration, as we would not be able to execute any more statement in an aborted transaction

[pirj]

avoid developers from having to understand

That would be ideal.

automatically disabling transaction when using non-transaction compatible methods

Do you think it is possible? As I understand it, the migration runner checks disable_ddl_statement before running change, and by the moment any of the non-transaction compatible methods is called, it's too late to change the return value of disable_ddl_statement.
That is precisely why I suggest users to decide to make their migrations safe.

One mechanism capable of analysing what is inside the change method before running it against the DB is CommandRecorder that I've touched in a recent PR. However, it seems like a major hassle to run it for up migration, as it's primarily used to determine the reverse order for down basing on the change call.

Of course, I had strong_migrations on guard in mind. If it complains and doesn't let the migration through, a developer adds an include SafeMigration, and our versions of migration helpers take over.
Is it fail-safe enough, and simple enough for developers?

On the other hand, developers don't have to keep in mind what to do if a migration with disabled DDL transaction worked only partially.

we could either automatically commit the transaction

I am not sure if I entirely understand what you mean.
Funny enough, I still recall Rails 5 was mistakenly interleaving in certain conditions: BEGIN, then SAVEPOINT, then COMMIT and then - RELEASE SAVEPOINT. It was a major pain.

[pirj]

I was getting curious why my add_index was CONCURRENTLY even though I didn't specify that explicitly, and why it was running outside a transaction even though there were no disable_ddl_transaction! in the migration, and it surprised my that strong_migrations do that https://github.com/ankane/strong_migrations/blob/2c114876b4252d391330bec074e15101e39490f5/lib/strong_migrations/safe_methods.rb#L8

So it seems like there's a confirmed way to selectively disable the transaction only for those migrations that use overridden helpers.