Use of 'commit_db_transaction' about strong_migrations HOT 9 CLOSED

@ankane I stumbled upon the same issue. I wasn't aware that the statements following the commit_db_transaction are not wrapped in a transaction until a coworker asked me to take a look at it. Could you update the example(s) in the README with a safer approach? I don't use add_index and original poster haven't mentioned it either, I was following the algorithm for adding a column with default value here. Thanks! One more thing, there is a warning in the logs when the method in question is called: warning: there is no transaction in progress. EDIT: Perhaps the method plays two roles in the example - adding the column in one transaction and committing the user updates in iterations of auto-committing transactions which avoids updating the whole users table at once.

from strong_migrations.

Yeah, just for the record, after discussing with the team here the approach we are taking is also to split up the migrations. We are making sure that we will push application code that will deal with that column on a separate PR too.

Thanks!

from strong_migrations.

There's no way around this with concurrent indexes since they must be run outside a transaction. My recommendation would be to create a separate migration for each add_index command if you're worried about it failing.

from strong_migrations.

@ankane @dsbonev Same issue here and I'm unsure of the best approach... (I was also following the steps to add a column with default value - no index).

Thanks!

from strong_migrations.

@dsbonev @lmatiolis I don't understand the concern. The approach outlined in the docs is safe/recommended.

from strong_migrations.

Oh, sorry for not being clear before @ankane. Let me try to explain the issue with more details so you can better evaluate if it's really a problem or not:

If you run this migration as is described on the docs:

  def up
    # 1
    add_column :users, :some_column, :text

    # 2
    change_column_default :users, :some_column, "default_value"

    # 3
    commit_db_transaction

    # 4.b (Rails < 5)
    User.find_in_batches do |users|
      User.where(id: users.map(&:id)).update_all some_column: "default_value"
    end
  end

  def down
    remove_column :users, :some_column
  end

A warning will appear on the console stating:

WARNING: there is no transaction in progress

If everything worked well, then it's all good!

But if an unexpected error happens after executing commit_db_transaction, like this:

class AddSomeColumnToUsers < ActiveRecord::Migration
  def up
    # 1
    add_column :users, :some_column, :text

    # 2
    change_column_default :users, :some_column, "default_value"

    # 3
    commit_db_transaction

    # 4.b (Rails < 5)
    User.find_in_batches do |users|
      User.where(id: users.map(&:id)).update_all some_column: "default_value"
      raise "something unexpected happens here"
    end
  end

  def down
    remove_column :users, :some_column
  end
end

The migration will of course fail because of the exception there and no rollback will happen (meaning that the column will remain added). Also, the migrations table won't be updated, so this migration wasn't considered executed, although it did add the column. Next time you run db:migrate, you will be getting an error that that column already exists on the database, because it will try to execute this migration again:

PG::DuplicateColumn: ERROR:  column "some_column" of relation "users" already exists

Thanks!

from strong_migrations.

@ankane I think the current functionality is fine, since this should only ever happen in a development environment, but it might be worthwhile to mention this behavior in the docs so that users are aware of it.

I know that commit_db_transaction is a Rails command and not specific to Strong Migrations, but I don't believe it's a well known command, and many users may not understand the implications. They might unknowingly encounter this scenario while working and be confused as to why their migrate/rollback isn't working as expected, etc. A quick heads up in the docs could potentially save someone an hour or two of debugging.

from strong_migrations.

@ankane - Would it be better to recommend developers use separate migrations rather calling commit_db_transaction? Breaking the process up into multiple migrations makes it easier to write idempotent migrations that can be retried in the event of a network or build machine hiccup.

from strong_migrations.

Yeah, we could recommend doing steps 1 and 2 inside a migration, and then step 4 in the Rails console, which would solve the rollback issue. One worry is developers may try to put step 4 in the migration w/o commit_db_transaction, which will be problematic.

from strong_migrations.