Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add_reference with concurrent index can be dangerous #250

Closed
Jetbo opened this issue Nov 20, 2023 · 2 comments
Closed

add_reference with concurrent index can be dangerous #250

Jetbo opened this issue Nov 20, 2023 · 2 comments

Comments

@Jetbo
Copy link

Jetbo commented Nov 20, 2023

Howdy,

I think section for adding-a-reference could be improved. For Postgres, it can cause a broken migration, as I have just recently found out.

The problem is that add_reference is a multi-step operation.

add_reference creates the column, then the index, then the foreign key. Since disable_ddl_transaction! is recommended when trying to add a reference with a concurrent index, the table can put into a partially migrated state with no automatic way to rollback since transactions were disabled.


For example,

class AddReferenceToUsers < ActiveRecord::Migration[7.1]
  disable_ddl_transaction!

  def change
    add_reference :users, :city, index: {algorithm: :concurrently}
  end
end

Would add the column city_id on users and then attempt to create the index. This indexing operation could fail due to a reason such as ActiveRecord::LockWaitTimeout: PG::LockNotAvailable: ERROR: canceling statement due to lock timeout. The column will remain even though the index failed since transactions are disabled. Re-running the migration will fail since Rails does not expect city_id on users to already exist.


I suggest that the recommendation for adding a reference with a concurrent index for Postgres is changed to this:

class AddCityToUsers < ActiveRecord::Migration[7.1]
  def change
    add_column :users, :city_id, :bigint
  end
end
class AddCityIndexToUsers < ActiveRecord::Migration[7.1]
  disable_ddl_transaction!

  def change
    add_index :users, :city_id, index: {algorithm: :concurrently}
  end
end

This way the index creation can be re-attempted separate from the column creation. Safe migration.

@Jetbo
Copy link
Author

Jetbo commented Nov 20, 2023

Something like this would also work

class AddCityToUsers < ActiveRecord::Migration[7.1]
  def change
    add_reference :users, :city, index: false
  end
end
class AddCityIndexToUsers < ActiveRecord::Migration[7.1]
  disable_ddl_transaction!

  def change
    add_index :users, :city_id, index: {algorithm: :concurrently}
  end
end

@Jetbo Jetbo changed the title add_reference with concurrent index is dangerous add_reference with concurrent index can be dangerous Nov 20, 2023
@ankane
Copy link
Owner

ankane commented Nov 20, 2023

Hi @Jetbo, check out #202 and #240.

@ankane ankane closed this as completed Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants