Merge branch 'pb-add-concurrent-index-check-invalid' into 'master'

Check invalid indexes in add_concurrent_index

See merge request gitlab-org/gitlab!71761

lib/gitlab/database/migration_helpers.rb

@@ -147,8 +147,18 @@ module Gitlab
         options = options.merge({ algorithm: :concurrently })
 
         if index_exists?(table_name, column_name, **options)
-          Gitlab::AppLogger.warn "Index not created because it already exists (this may be due to an aborted migration or similar): table_name: #{table_name}, column_name: #{column_name}"
-          return
+          name = options[:name] || index_name(table_name, column_name)
+          _, schema = table_name.to_s.split('.').reverse
+
+          if index_invalid?(name, schema: schema)
+            say "Index being recreated because the existing version was INVALID: table_name: #{table_name}, column_name: #{column_name}"
+
+            remove_concurrent_index_by_name(table_name, name)
+          else
+            say "Index not created because it already exists (this may be due to an aborted migration or similar): table_name: #{table_name}, column_name: #{column_name}"
+
+            return
+          end
         end
 
         disable_statement_timeout do
@@ -159,6 +169,23 @@ module Gitlab
         unprepare_async_index(table_name, column_name, **options)
       end
 
+      def index_invalid?(index_name, schema: nil)
+        index_name = connection.quote(index_name)
+        schema = connection.quote(schema) if schema
+        schema ||= 'current_schema()'
+
+        connection.select_value(<<~SQL)
+          select not i.indisvalid
+          from pg_class c
+          inner join pg_index i
+            on c.oid = i.indexrelid
+          inner join pg_namespace n
+            on n.oid = c.relnamespace
+          where n.nspname = #{schema}
+            and c.relname = #{index_name}
+        SQL
+      end
+
       # Removes an existed index, concurrently
       #
       # Example:

spec/lib/gitlab/database/migration_helpers_spec.rb

@@ -271,12 +271,92 @@ RSpec.describe Gitlab::Database::MigrationHelpers do
         model.add_concurrent_index(:users, :foo, unique: true)
       end
 
-      it 'does nothing if the index exists already' do
-        expect(model).to receive(:index_exists?)
-          .with(:users, :foo, { algorithm: :concurrently, unique: true }).and_return(true)
-        expect(model).not_to receive(:add_index)
+      context 'when the index exists and is valid' do
+        before do
+          model.add_index :users, :id, unique: true
+        end
 
-        model.add_concurrent_index(:users, :foo, unique: true)
+        it 'does leaves the existing index' do
+          expect(model).to receive(:index_exists?)
+            .with(:users, :id, { algorithm: :concurrently, unique: true }).and_call_original
+
+          expect(model).not_to receive(:remove_index)
+          expect(model).not_to receive(:add_index)
+
+          model.add_concurrent_index(:users, :id, unique: true)
+        end
+      end
+
+      context 'when an invalid copy of the index exists' do
+        before do
+          model.add_index :users, :id, unique: true, name: index_name
+
+          model.connection.execute(<<~SQL)
+            UPDATE pg_index
+            SET indisvalid = false
+            WHERE indexrelid = '#{index_name}'::regclass
+          SQL
+        end
+
+        context 'when the default name is used' do
+          let(:index_name) { model.index_name(:users, :id) }
+
+          it 'drops and recreates the index' do
+            expect(model).to receive(:index_exists?)
+              .with(:users, :id, { algorithm: :concurrently, unique: true }).and_call_original
+            expect(model).to receive(:index_invalid?).with(index_name, schema: nil).and_call_original
+
+            expect(model).to receive(:remove_concurrent_index_by_name).with(:users, index_name)
+
+            expect(model).to receive(:add_index)
+              .with(:users, :id, { algorithm: :concurrently, unique: true })
+
+            model.add_concurrent_index(:users, :id, unique: true)
+          end
+        end
+
+        context 'when a custom name is used' do
+          let(:index_name) { 'my_test_index' }
+
+          it 'drops and recreates the index' do
+            expect(model).to receive(:index_exists?)
+              .with(:users, :id, { algorithm: :concurrently, unique: true, name: index_name }).and_call_original
+            expect(model).to receive(:index_invalid?).with(index_name, schema: nil).and_call_original
+
+            expect(model).to receive(:remove_concurrent_index_by_name).with(:users, index_name)
+
+            expect(model).to receive(:add_index)
+              .with(:users, :id, { algorithm: :concurrently, unique: true, name: index_name })
+
+            model.add_concurrent_index(:users, :id, unique: true, name: index_name)
+          end
+        end
+
+        context 'when a qualified table name is used' do
+          let(:other_schema) { 'foo_schema' }
+          let(:index_name) { 'my_test_index' }
+          let(:table_name) { "#{other_schema}.users" }
+
+          before do
+            model.connection.execute(<<~SQL)
+              CREATE SCHEMA #{other_schema};
+              ALTER TABLE users SET SCHEMA #{other_schema};
+            SQL
+          end
+
+          it 'drops and recreates the index' do
+            expect(model).to receive(:index_exists?)
+              .with(table_name, :id, { algorithm: :concurrently, unique: true, name: index_name }).and_call_original
+            expect(model).to receive(:index_invalid?).with(index_name, schema: other_schema).and_call_original
+
+            expect(model).to receive(:remove_concurrent_index_by_name).with(table_name, index_name)
+
+            expect(model).to receive(:add_index)
+              .with(table_name, :id, { algorithm: :concurrently, unique: true, name: index_name })
+
+            model.add_concurrent_index(table_name, :id, unique: true, name: index_name)
+          end
+        end
       end
 
       it 'unprepares the async index creation' do