Merge branch '337907-fix-reindexing-cleanup' into 'master'

Drop reindexing leftovers only if exclusive lease is granted

See merge request gitlab-org/gitlab!75290

lib/gitlab/database/reindexing.rb

@@ -76,20 +76,7 @@ module Gitlab
 
       def self.cleanup_leftovers!
         PostgresIndex.reindexing_leftovers.each do |index|
-          Gitlab::AppLogger.info("Removing index #{index.identifier} which is a leftover, temporary index from previous reindexing activity")
-
-          retries = Gitlab::Database::WithLockRetriesOutsideTransaction.new(
-            connection: index.connection,
-            timing_configuration: REMOVE_INDEX_RETRY_CONFIG,
-            klass: self.class,
-            logger: Gitlab::AppLogger
-          )
-
-          retries.run(raise_on_exhaustion: false) do
-            index.connection.tap do |conn|
-              conn.execute("DROP INDEX CONCURRENTLY IF EXISTS #{conn.quote_table_name(index.schema)}.#{conn.quote_table_name(index.name)}")
-            end
-          end
+          Coordinator.new(index).drop
         end
       end
     end

lib/gitlab/database/reindexing/coordinator.rb

@@ -31,6 +31,25 @@ module Gitlab
           end
         end
 
+        def drop
+          try_obtain_lease do
+            Gitlab::AppLogger.info("Removing index #{index.identifier} which is a leftover, temporary index from previous reindexing activity")
+
+            retries = Gitlab::Database::WithLockRetriesOutsideTransaction.new(
+              connection: index.connection,
+              timing_configuration: REMOVE_INDEX_RETRY_CONFIG,
+              klass: self.class,
+              logger: Gitlab::AppLogger
+            )
+
+            retries.run(raise_on_exhaustion: false) do
+              index.connection.tap do |conn|
+                conn.execute("DROP INDEX CONCURRENTLY IF EXISTS #{conn.quote_table_name(index.schema)}.#{conn.quote_table_name(index.name)}")
+              end
+            end
+          end
+        end
+
         private
 
         def with_notifications(action)

spec/lib/gitlab/database/reindexing/coordinator_spec.rb

@@ -6,30 +6,34 @@ RSpec.describe Gitlab::Database::Reindexing::Coordinator do
   include Database::DatabaseHelpers
   include ExclusiveLeaseHelpers
 
-  describe '.perform' do
-    subject { described_class.new(index, notifier).perform }
-
-    let(:index) { create(:postgres_index) }
-    let(:notifier) { instance_double(Gitlab::Database::Reindexing::GrafanaNotifier, notify_start: nil, notify_end: nil) }
-    let(:reindexer) { instance_double(Gitlab::Database::Reindexing::ReindexConcurrently, perform: nil) }
-    let(:action) { create(:reindex_action, index: index) }
+  let(:notifier) { instance_double(Gitlab::Database::Reindexing::GrafanaNotifier, notify_start: nil, notify_end: nil) }
+  let(:index) { create(:postgres_index) }
+  let(:connection) { index.connection }
 
-    let!(:lease) { stub_exclusive_lease(lease_key, uuid, timeout: lease_timeout) }
-    let(:lease_key) { "gitlab/database/reindexing/coordinator/#{Gitlab::Database::PRIMARY_DATABASE_NAME}" }
-    let(:lease_timeout) { 1.day }
-    let(:uuid) { 'uuid' }
+  let!(:lease) { stub_exclusive_lease(lease_key, uuid, timeout: lease_timeout) }
+  let(:lease_key) { "gitlab/database/reindexing/coordinator/#{Gitlab::Database::PRIMARY_DATABASE_NAME}" }
+  let(:lease_timeout) { 1.day }
+  let(:uuid) { 'uuid' }
 
-    around do |example|
-      model = Gitlab::Database.database_base_models[Gitlab::Database::PRIMARY_DATABASE_NAME]
+  around do |example|
+    model = Gitlab::Database.database_base_models[Gitlab::Database::PRIMARY_DATABASE_NAME]
 
-      Gitlab::Database::SharedModel.using_connection(model.connection) do
-        example.run
-      end
+    Gitlab::Database::SharedModel.using_connection(model.connection) do
+      example.run
     end
+  end
 
-    before do
-      swapout_view_for_table(:postgres_indexes)
+  before do
+    swapout_view_for_table(:postgres_indexes)
+  end
 
+  describe '#perform' do
+    subject { described_class.new(index, notifier).perform }
+
+    let(:reindexer) { instance_double(Gitlab::Database::Reindexing::ReindexConcurrently, perform: nil) }
+    let(:action) { create(:reindex_action, index: index) }
+
+    before do
       allow(Gitlab::Database::Reindexing::ReindexConcurrently).to receive(:new).with(index).and_return(reindexer)
       allow(Gitlab::Database::Reindexing::ReindexAction).to receive(:create_for).with(index).and_return(action)
     end
@@ -87,4 +91,40 @@ RSpec.describe Gitlab::Database::Reindexing::Coordinator do
       end
     end
   end
+
+  describe '#drop' do
+    let(:connection) { index.connection }
+
+    subject(:drop) { described_class.new(index, notifier).drop }
+
+    context 'when exclusive lease is granted' do
+      it 'drops the index with lock retries' do
+        expect(lease).to receive(:try_obtain).ordered.and_return(uuid)
+
+        expect_query("SET lock_timeout TO '60000ms'")
+        expect_query("DROP INDEX CONCURRENTLY IF EXISTS \"public\".\"#{index.name}\"")
+        expect_query("RESET idle_in_transaction_session_timeout; RESET lock_timeout")
+
+        expect(Gitlab::ExclusiveLease).to receive(:cancel).ordered.with(lease_key, uuid)
+
+        drop
+      end
+
+      def expect_query(sql)
+        expect(connection).to receive(:execute).ordered.with(sql).and_wrap_original do |method, sql|
+          method.call(sql.sub(/CONCURRENTLY/, ''))
+        end
+      end
+    end
+
+    context 'when exclusive lease is not granted' do
+      it 'does not drop the index' do
+        expect(lease).to receive(:try_obtain).ordered.and_return(false)
+        expect(Gitlab::Database::WithLockRetriesOutsideTransaction).not_to receive(:new)
+        expect(connection).not_to receive(:execute)
+
+        drop
+      end
+    end
+  end
 end