Skip to content

Commit

Permalink
Fix finalize_column_type_change to not recreate already existing in…
Browse files Browse the repository at this point in the history
…dexes on the temporary column
  • Loading branch information
fatkodima committed Jan 19, 2024
1 parent 006ca82 commit fafb11e
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 20 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
## master (unreleased)

- Fix `finalize_column_type_change` to not recreate already existing indexes on the temporary column
- Remove potentially heavy queries used to get the ranges of a background migration

## 0.12.0 (2024-01-18)
Expand Down
7 changes: 0 additions & 7 deletions lib/online_migrations/change_column_type_helpers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -416,15 +416,8 @@ def __copy_indexes(table_name, from_column, to_column)
end
end

if index.name.include?(from_column)
name = index.name.gsub(from_column, to_column)
end

name = index_name(table_name, new_columns) if !name || name.length > max_identifier_length

options = {
unique: index.unique,
name: name,
length: index.lengths,
order: index.orders,
}
Expand Down
20 changes: 7 additions & 13 deletions lib/online_migrations/schema_statements.rb
Original file line number Diff line number Diff line change
Expand Up @@ -681,32 +681,26 @@ def add_reference_concurrently(table_name, ref_name, **options)
# @see https://edgeapi.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters/SchemaStatements.html#method-i-add_index
#
def add_index(table_name, column_name, **options)
algorithm = options[:algorithm]

__ensure_not_in_transaction! if algorithm == :concurrently

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

index_name = options[:name]
index_name ||= index_name(table_name, column_names)
__ensure_not_in_transaction! if options[:algorithm] == :concurrently

if index_exists?(table_name, column_name, **options)
index = indexes(table_name).find { |i| i.defined_for?(column_name, **options) }
if index
schema = __schema_for_table(table_name)

if __index_valid?(index_name, schema: schema)
if __index_valid?(index.name, schema: schema)
Utils.say("Index was not created because it already exists.")
return
else
Utils.say("Recreating invalid index: table_name: #{table_name}, column_name: #{column_name}")
remove_index(table_name, column_name, name: index_name, algorithm: algorithm)
remove_index(table_name, column_name, **options)
end
end

if OnlineMigrations.config.statement_timeout
# "CREATE INDEX CONCURRENTLY" requires a "SHARE UPDATE EXCLUSIVE" lock.
# It only conflicts with constraint validations, creating/removing indexes,
# and some other "ALTER TABLE"s.
super(table_name, column_name, **options.merge(name: index_name))
super
else
OnlineMigrations.deprecator.warn(<<~MSG)
Running `add_index` without a statement timeout is deprecated.
Expand All @@ -716,7 +710,7 @@ def add_index(table_name, column_name, **options)
MSG

disable_statement_timeout do
super(table_name, column_name, **options.merge(name: index_name))
super
end
end
end
Expand Down
15 changes: 15 additions & 0 deletions test/schema_statements/changing_column_type_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,21 @@ def test_finalize_column_type_change_keeps_columns_in_sync
assert_equal project.id, old_id
end

def test_finalize_column_type_change_does_not_recreate_existing_index
# assert_not @connection.index_exists?(:projects, :company_id)
@connection.add_index(:projects, :company_id)

@connection.initialize_column_type_change(:projects, :company_id, :bigint)
# Lets imagine, that the index was added before, manually, to the column.
@connection.add_index(:projects, :company_id_for_type_change, name: "my_custom_name") # use custom index name

refute_sql("CREATE INDEX") do
@connection.finalize_column_type_change(:projects, :company_id)
assert @connection.index_exists?(:projects, :company_id)
assert @connection.index_exists?(:projects, :company_id_for_type_change)
end
end

def test_finalize_columns_type_change
@connection.initialize_columns_type_change(:projects, [[:id, :bigint], [:name, :string]])
@connection.finalize_columns_type_change(:projects, :id, :name)
Expand Down

0 comments on commit fafb11e

Please sign in to comment.