Skip to content

Commit

Permalink
Simplify add_index idempotency implementation
Browse files Browse the repository at this point in the history
  • Loading branch information
fatkodima committed Jan 16, 2024
1 parent 1a2211f commit d7817a0
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 10 deletions.
18 changes: 8 additions & 10 deletions lib/online_migrations/schema_statements.rb
Original file line number Diff line number Diff line change
Expand Up @@ -726,19 +726,18 @@ def add_index(table_name, column_name, **options)
# @see https://edgeapi.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters/SchemaStatements.html#method-i-remove_index
#
def remove_index(table_name, column_name = nil, **options)
algorithm = options[:algorithm]
if column_name.blank? && options[:column].blank? && options[:name].blank?
raise ArgumentError, "No name or columns specified"
end

__ensure_not_in_transaction! if algorithm == :concurrently
__ensure_not_in_transaction! if options[:algorithm] == :concurrently

column_names = index_column_names(column_name || options[:column])

if index_exists?(table_name, column_names, **options)
if index_exists?(table_name, column_name, **options)
if OnlineMigrations.config.statement_timeout
# "DROP INDEX CONCURRENTLY" requires a "SHARE UPDATE EXCLUSIVE" lock.
# It only conflicts with constraint validations, other creating/removing indexes,
# and some "ALTER TABLE"s.

super(table_name, **options.merge(column: column_names))
super(table_name, column_name, **options)
else
OnlineMigrations.deprecator.warn(<<~MSG)
Running `remove_index` without a statement timeout is deprecated.
Expand All @@ -748,12 +747,11 @@ def remove_index(table_name, column_name = nil, **options)
MSG

disable_statement_timeout do
super(table_name, **options.merge(column: column_names))
super(table_name, column_name, **options)
end
end
else
Utils.say("Index was not removed because it does not exist (this may be due to an aborted migration " \
"or similar): table_name: #{table_name}, column_name: #{column_names}")
Utils.say("Index was not removed because it does not exist.")
end
end

Expand Down
6 changes: 6 additions & 0 deletions test/schema_statements/indexes_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,12 @@ def test_remove_index_by_name
assert_not @connection.index_exists?(:users, :name)
end

def test_remove_index_without_columns_and_name_provided
assert_raises_with_message(ArgumentError, "No name or columns specified") do
@connection.remove_index(:users)
end
end

def test_remove_index_concurrently_in_transaction
@connection.add_index(:users, :name)
assert_raises_in_transaction do
Expand Down

0 comments on commit d7817a0

Please sign in to comment.