From d6878da9c7b32711677b2fb38687a6a694c7ed61 Mon Sep 17 00:00:00 2001
From: Andrew Kane <andrew@ankane.org>
Date: Thu, 7 Nov 2024 00:29:52 -0800
Subject: [PATCH] Use reindex instead of removing index

---
 CHANGELOG.md                                  |  1 -
 lib/strong_migrations.rb                      |  3 +--
 .../adapters/postgresql_adapter.rb            |  2 +-
 lib/strong_migrations/checks.rb               | 12 ----------
 lib/strong_migrations/safe_methods.rb         | 22 +++++++++++++++++--
 test/safe_by_default_test.rb                  |  7 +++---
 6 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 396db7f2..f4698480 100644
--- a/CHANGELOG.md
+++ b/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`
 
diff --git a/lib/strong_migrations.rb b/lib/strong_migrations.rb
index 0348b0a3..76ffb14d 100644
--- a/lib/strong_migrations.rb
+++ b/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?
diff --git a/lib/strong_migrations/adapters/postgresql_adapter.rb b/lib/strong_migrations/adapters/postgresql_adapter.rb
index 52e92223..8bfd81d9 100644
--- a/lib/strong_migrations/adapters/postgresql_adapter.rb
+++ b/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
diff --git a/lib/strong_migrations/checks.rb b/lib/strong_migrations/checks.rb
index a566b78e..7aab849c 100644
--- a/lib/strong_migrations/checks.rb
+++ b/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
diff --git a/lib/strong_migrations/safe_methods.rb b/lib/strong_migrations/safe_methods.rb
index b542ffd6..550ed661 100644
--- a/lib/strong_migrations/safe_methods.rb
+++ b/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
diff --git a/test/safe_by_default_test.rb b/test/safe_by_default_test.rb
index ee7a6e51..2c02dd7d 100644
--- a/test/safe_by_default_test.rb
+++ b/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