From fafb11e17679c3023a008bc00af585568000ec34 Mon Sep 17 00:00:00 2001 From: fatkodima Date: Fri, 19 Jan 2024 22:57:22 +0200 Subject: [PATCH] Fix `finalize_column_type_change` to not recreate already existing indexes on the temporary column --- CHANGELOG.md | 1 + .../change_column_type_helpers.rb | 7 ------- lib/online_migrations/schema_statements.rb | 20 +++++++------------ .../changing_column_type_test.rb | 15 ++++++++++++++ 4 files changed, 23 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 706ad00..7e70e61 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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) diff --git a/lib/online_migrations/change_column_type_helpers.rb b/lib/online_migrations/change_column_type_helpers.rb index 8349766..c326303 100644 --- a/lib/online_migrations/change_column_type_helpers.rb +++ b/lib/online_migrations/change_column_type_helpers.rb @@ -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, } diff --git a/lib/online_migrations/schema_statements.rb b/lib/online_migrations/schema_statements.rb index 3582ae5..ebda73d 100644 --- a/lib/online_migrations/schema_statements.rb +++ b/lib/online_migrations/schema_statements.rb @@ -681,24 +681,18 @@ 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 @@ -706,7 +700,7 @@ def add_index(table_name, column_name, **options) # "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. @@ -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 diff --git a/test/schema_statements/changing_column_type_test.rb b/test/schema_statements/changing_column_type_test.rb index 55690aa..96e780a 100644 --- a/test/schema_statements/changing_column_type_test.rb +++ b/test/schema_statements/changing_column_type_test.rb @@ -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)