Use reindex instead of removing index

CHANGELOG.md

@@ -1,7 +1,6 @@
 ## 2.1.0 (unreleased)
 
 - Added `skip_databases` option
-- Added experimental `remove_invalid_indexes` option
 - Added warning for unsupported adapters
 - Improved output for `db:forward`, `db:rollback`, `db:migrate:up`, and `db:migrate:down`
 

lib/strong_migrations.rb

@@ -29,7 +29,7 @@ class << self
       :target_postgresql_version, :target_mysql_version, :target_mariadb_version,
       :enabled_checks, :lock_timeout, :statement_timeout, :check_down, :target_version,
       :safe_by_default, :target_sql_mode, :lock_timeout_retries, :lock_timeout_retry_delay,
-      :alphabetize_schema, :skip_databases, :remove_invalid_indexes
+      :alphabetize_schema, :skip_databases
     attr_writer :lock_timeout_limit
   end
   self.auto_analyze = false
@@ -41,7 +41,6 @@ class << self
   self.check_down = false
   self.alphabetize_schema = false
   self.skip_databases = []
-  self.remove_invalid_indexes = false
 
   # private
   def self.developer_env?

lib/strong_migrations/adapters/postgresql_adapter.rb

@@ -127,7 +127,7 @@ def change_type_safe?(table, column, type, options, existing_column, existing_ty
         safe
       end
 
-      # TODO use indexes method when Active Record < 7.1 is no longer supported
+      # TODO remove when Active Record < 7.1 is no longer supported
       def index_invalid?(table_name, index_name)
         query = <<~SQL
           SELECT

lib/strong_migrations/checks.rb

@@ -461,17 +461,5 @@ def migration_suffix
     def model_name(table)
       table.to_s.classify
     end
-
-    def remove_invalid_index_if_needed(*args, **options)
-      return unless StrongMigrations.remove_invalid_indexes && postgresql? && direction == :up
-
-      table, columns = args
-      index_name = options.fetch(:name, connection.index_name(table, columns))
-      if adapter.index_invalid?(table, index_name)
-        @migration.say("Removing invalid index")
-        # TODO pass index schema for extra safety?
-        @migration.remove_index(table, **{name: index_name}.merge(options.slice(:algorithm)))
-      end
-    end
   end
 end

lib/strong_migrations/safe_methods.rb

@@ -6,8 +6,14 @@ def safe_by_default_method?(method)
 
     def safe_add_index(*args, **options)
       disable_transaction
-      remove_invalid_index_if_needed(*args, **options.merge(algorithm: :concurrently))
-      @migration.add_index(*args, **options.merge(algorithm: :concurrently))
+      if direction == :up && (index_name = invalid_index_name(*args, **options))
+        @migration.safety_assured do
+          # TODO pass index schema for extra safety?
+          @migration.execute("REINDEX INDEX CONCURRENTLY #{connection.quote_table_name(index_name)}")
+        end
+      else
+        @migration.add_index(*args, **options.merge(algorithm: :concurrently))
+      end
     end
 
     def safe_remove_index(*args, **options)
@@ -119,5 +125,17 @@ def disable_transaction
     def in_transaction?
       connection.open_transactions > 0
     end
+
+    def invalid_index_name(*args, **options)
+      return nil unless connection.index_exists?(*args, **options.merge(valid: false))
+
+      table, columns = args
+      index_name = options.fetch(:name, connection.index_name(table, columns))
+
+      # valid option is ignored for Active Record < 7.1, so need to check name as well
+      return nil unless ar_version >= 7.1 || adapter.index_invalid?(table, index_name)
+
+      index_name
+    end
   end
 end

test/safe_by_default_test.rb

@@ -24,13 +24,14 @@ def test_add_index_invalid
       end
     end
 
+    migrate AddIndex
+
+    # fail if trying to add the same index in a future migration
     assert_raises(ActiveRecord::StatementInvalid) do
       migrate AddIndex
     end
 
-    StrongMigrations.stub(:remove_invalid_indexes, true) do
-      assert_safe AddIndex
-    end
+    migrate AddIndex, direction: :down
   end
 
   def test_add_index_extra_arguments