Should "Adding a foreign key" recommend two migrations? about online_migrations HOT 6 CLOSED

Ahhh we just found this

from online_migrations.

I think the docs could instead suggest distinct migrations without disable_ddl_transaction. Suggesting to split the migrations would allow the add_foreign_key migration to record as successful and the failing validate_foreign_key to be the one that errors in isolation.

I'd be happy to PR something like the following if you think it improves the docs

Existing

Proposed

Add the foreign key without validating existing rows, and then validate them in a separate transaction.

class AddForeignKeyToProjectsUser < ActiveRecord::Migration[7.1]
  def change
    add_foreign_key :projects, :users, validate: false
  end
end

class ValidateForeignKeyToProjectsUser < ActiveRecord::Migration[7.1]
  def change
    validate_foreign_key :projects, :users
  end
end

Note: If you combine those into a single migration you must use disable_ddl_transaction!, without it the migration will fail.

from online_migrations.

@fatkodima thanks for doing this work 💯

from online_migrations.

Suggesting to split the migrations would allow the add_foreign_key migration to record as successful and the failing validate_foreign_key to be the one that errors in isolation.

Can you point to the benefits of this approach?

from online_migrations.

Suggesting to split the migrations would allow the add_foreign_key migration to record as successful and the failing validate_foreign_key to be the one that errors in isolation.

Can you point to the benefits of this approach?

I think the benefit is to logically isolate the failing validate_foreign_key step of the migration while allowing the add_foreign_key step to proceed and register as completed. The system would then only reapply and report failures for non-completed migration steps.

I feel this isolation avoids the developer having to interpret the combined migration step logs to understand the add_foreign_key statement is skipped and the validate_foreign_key is failing, specifically the logged output from foreign_key_exists method in #54 (comment)

I'd like to note that the migration functionality between the two options is mostly the same. The only time that separating the steps across migrations might be useful is when the validate_foreign_key step errors and we fail / repeat the whole migration.

from online_migrations.

For me, having just everything in 1 migration and knowing that it will be idempotent is easier, because I can just copy paste the suggestion inside the already generated file (actually if the gem's suggestion happens after the migration was generated). If the gem suggests 2 files, we need to manually create a second file with proper timestamp etc. So some extra work.

But, if people think that this is more confusing, I am ok with the change, so PR welcome. You need to change the readme as well as the command_checker.rb to suggest the 2 files migration.

from online_migrations.