Add latest changes from gitlab-org/gitlab@master

3 files changed, 122 insertions, 40 deletions

diff --git a/spec/lib/gitlab/database/reindexing/index_spec.rb b/spec/lib/gitlab/database/postgres_index_spec.rb
index d467178815b..2a9598736ed 100644
--- a/spec/lib/gitlab/database/reindexing/index_spec.rb
+++ b/spec/lib/gitlab/database/postgres_index_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::Database::Reindexing::Index do
+RSpec.describe Gitlab::Database::PostgresIndex do
   before do
     ActiveRecord::Base.connection.execute(<<~SQL)
       CREATE INDEX foo_idx ON public.users (name);
@@ -13,16 +13,16 @@ RSpec.describe Gitlab::Database::Reindexing::Index do
   end
 
   def find(name)
-    described_class.find_with_schema(name)
+    described_class.by_identifier(name)
   end
 
-  describe '.find_with_schema' do
-    it 'returns an instance of Gitlab::Database::Reindexing::Index when the index is present' do
-      expect(find('public.foo_idx')).to be_a(Gitlab::Database::Reindexing::Index)
+  describe '.by_identifier' do
+    it 'finds the index' do
+      expect(find('public.foo_idx')).to be_a(Gitlab::Database::PostgresIndex)
     end
 
-    it 'returns nil if the index is not present' do
-      expect(find('public.idontexist')).to be_nil
+    it 'raises an error if not found' do
+      expect { find('public.idontexist') }.to raise_error(ActiveRecord::RecordNotFound)
     end
 
     it 'raises ArgumentError if given a non-fully qualified index name' do
@@ -30,6 +30,26 @@ RSpec.describe Gitlab::Database::Reindexing::Index do
     end
   end
 
+  describe '#regular' do
+    it 'only non-unique indexes' do
+      expect(described_class.regular).to all(have_attributes(unique: false))
+    end
+
+    it 'only non partitioned indexes ' do
+      expect(described_class.regular).to all(have_attributes(partitioned: false))
+    end
+
+    it 'only indexes that dont serve an exclusion constraint' do
+      expect(described_class.regular).to all(have_attributes(exclusion: false))
+    end
+  end
+
+  describe '#random_few' do
+    it 'limits to two records by default' do
+      expect(described_class.random_few(2).size).to eq(2)
+    end
+  end
+
   describe '#unique?' do
     it 'returns true for a unique index' do
       expect(find('public.bar_key')).to be_unique
@@ -44,9 +64,9 @@ RSpec.describe Gitlab::Database::Reindexing::Index do
     end
   end
 
-  describe '#valid?' do
-    it 'returns true if the index is valid' do
-      expect(find('public.foo_idx')).to be_valid
+  describe '#valid_index?' do
+    it 'returns true if the index is invalid' do
+      expect(find('public.foo_idx')).to be_valid_index
     end
 
     it 'returns false if the index is marked as invalid' do
@@ -56,7 +76,7 @@ RSpec.describe Gitlab::Database::Reindexing::Index do
         WHERE pg_class.relname = 'foo_idx' AND pg_index.indexrelid = pg_class.oid
       SQL
 
-      expect(find('public.foo_idx')).not_to be_valid
+      expect(find('public.foo_idx')).not_to be_valid_index
     end
   end
 
diff --git a/spec/lib/gitlab/database/reindexing/concurrent_reindex_spec.rb b/spec/lib/gitlab/database/reindexing/concurrent_reindex_spec.rb
index c113a89deca..49087e7ff2c 100644
--- a/spec/lib/gitlab/database/reindexing/concurrent_reindex_spec.rb
+++ b/spec/lib/gitlab/database/reindexing/concurrent_reindex_spec.rb
@@ -8,7 +8,7 @@ RSpec.describe Gitlab::Database::Reindexing::ConcurrentReindex, '#perform' do
   let(:table_name) { '_test_reindex_table' }
   let(:column_name) { '_test_column' }
   let(:index_name) { '_test_reindex_index' }
-  let(:index) { double('index', name: index_name, schema: 'public', unique?: false, definition: 'CREATE INDEX _test_reindex_index ON public._test_reindex_table USING btree (_test_column)') }
+  let(:index) { instance_double(Gitlab::Database::PostgresIndex, indexrelid: 42, name: index_name, schema: 'public', partitioned?: false, unique?: false, exclusion?: false, definition: 'CREATE INDEX _test_reindex_index ON public._test_reindex_table USING btree (_test_column)') }
   let(:logger) { double('logger', debug: nil, info: nil, error: nil ) }
   let(:connection) { ActiveRecord::Base.connection }
 
@@ -23,7 +23,9 @@ RSpec.describe Gitlab::Database::Reindexing::ConcurrentReindex, '#perform' do
   end
 
   context 'when the index is unique' do
-    let(:index) { double('index', name: index_name, unique?: true, definition: 'CREATE INDEX _test_reindex_index ON public._test_reindex_table USING btree (_test_column)') }
+    before do
+      allow(index).to receive(:unique?).and_return(true)
+    end
 
     it 'raises an error' do
       expect do
@@ -32,12 +34,41 @@ RSpec.describe Gitlab::Database::Reindexing::ConcurrentReindex, '#perform' do
     end
   end
 
+  context 'when the index is partitioned' do
+    before do
+      allow(index).to receive(:partitioned?).and_return(true)
+    end
+
+    it 'raises an error' do
+      expect do
+        subject.perform
+      end.to raise_error(described_class::ReindexError, /partitioned indexes are currently not supported/)
+    end
+  end
+
+  context 'when the index serves an exclusion constraint' do
+    before do
+      allow(index).to receive(:exclusion?).and_return(true)
+    end
+
+    it 'raises an error' do
+      expect do
+        subject.perform
+      end.to raise_error(described_class::ReindexError, /indexes serving an exclusion constraint are currently not supported/)
+    end
+  end
+
   context 'replacing the original index with a rebuilt copy' do
-    let(:replacement_name) { 'tmp_reindex__test_reindex_index' }
-    let(:replaced_name) { 'old_reindex__test_reindex_index' }
+    let(:replacement_name) { 'tmp_reindex_42' }
+    let(:replaced_name) { 'old_reindex_42' }
 
     let(:create_index) { "CREATE INDEX CONCURRENTLY #{replacement_name} ON public.#{table_name} USING btree (#{column_name})" }
-    let(:drop_index) { "DROP INDEX CONCURRENTLY IF EXISTS #{replacement_name}" }
+    let(:drop_index) do
+      <<~SQL
+        DROP INDEX CONCURRENTLY
+        IF EXISTS "public"."#{replacement_name}"
+      SQL
+    end
 
     let!(:original_index) { find_index_create_statement }
 
@@ -58,18 +89,17 @@ RSpec.describe Gitlab::Database::Reindexing::ConcurrentReindex, '#perform' do
       end
 
       it 'replaces the existing index with an identical index' do
-        expect(subject).to receive(:disable_statement_timeout).exactly(3).times.and_yield
+        expect(subject).to receive(:disable_statement_timeout).twice.and_yield
 
-        expect_to_execute_concurrently_in_order(drop_index)
         expect_to_execute_concurrently_in_order(create_index)
 
         expect_next_instance_of(::Gitlab::Database::WithLockRetries) do |instance|
           expect(instance).to receive(:run).with(raise_on_exhaustion: true).and_yield
         end
 
-        expect_to_execute_in_order("ALTER INDEX #{index.name} RENAME TO #{replaced_name}")
-        expect_to_execute_in_order("ALTER INDEX #{replacement_name} RENAME TO #{index.name}")
-        expect_to_execute_in_order("ALTER INDEX #{replaced_name} RENAME TO #{replacement_name}")
+        expect_index_rename(index.name, replaced_name)
+        expect_index_rename(replacement_name, index.name)
+        expect_index_rename(replaced_name, replacement_name)
 
         expect_to_execute_concurrently_in_order(drop_index)
 
@@ -93,9 +123,9 @@ RSpec.describe Gitlab::Database::Reindexing::ConcurrentReindex, '#perform' do
             expect(instance).to receive(:run).with(raise_on_exhaustion: true).and_yield
           end
 
-          expect_to_execute_in_order("ALTER INDEX #{index.name} RENAME TO #{replaced_name}")
-          expect_to_execute_in_order("ALTER INDEX #{replacement_name} RENAME TO #{index.name}")
-          expect_to_execute_in_order("ALTER INDEX #{replaced_name} RENAME TO #{replacement_name}")
+          expect_index_rename(index.name, replaced_name)
+          expect_index_rename(replacement_name, index.name)
+          expect_index_rename(replaced_name, replacement_name)
 
           expect_to_execute_concurrently_in_order(drop_index)
 
@@ -107,14 +137,12 @@ RSpec.describe Gitlab::Database::Reindexing::ConcurrentReindex, '#perform' do
 
       context 'when it fails to create the replacement index' do
         it 'safely cleans up and signals the error' do
-          expect_to_execute_concurrently_in_order(drop_index)
-
           expect(connection).to receive(:execute).with(create_index).ordered
             .and_raise(ActiveRecord::ConnectionTimeoutError, 'connect timeout')
 
           expect_to_execute_concurrently_in_order(drop_index)
 
-          expect { subject.perform }.to raise_error(described_class::ReindexError, /connect timeout/)
+          expect { subject.perform }.to raise_error(ActiveRecord::ConnectionTimeoutError, /connect timeout/)
 
           check_index_exists
         end
@@ -122,11 +150,10 @@ RSpec.describe Gitlab::Database::Reindexing::ConcurrentReindex, '#perform' do
 
       context 'when the replacement index is not valid' do
         it 'safely cleans up and signals the error' do
-          expect_to_execute_concurrently_in_order(drop_index)
-          expect_to_execute_concurrently_in_order(create_index)
+          replacement_index = double('replacement index', valid_index?: false)
+          allow(Gitlab::Database::PostgresIndex).to receive(:find_by).with(schema: 'public', name: replacement_name).and_return(nil, replacement_index)
 
-          replacement_index = double('replacement index', valid?: false)
-          allow(Gitlab::Database::Reindexing::Index).to receive(:find_with_schema).with("public.#{replacement_name}").and_return(replacement_index)
+          expect_to_execute_concurrently_in_order(create_index)
 
           expect_to_execute_concurrently_in_order(drop_index)
 
@@ -138,20 +165,20 @@ RSpec.describe Gitlab::Database::Reindexing::ConcurrentReindex, '#perform' do
 
       context 'when a database error occurs while swapping the indexes' do
         it 'safely cleans up and signals the error' do
-          expect_to_execute_concurrently_in_order(drop_index)
+          replacement_index = double('replacement index', valid_index?: true)
+          allow(Gitlab::Database::PostgresIndex).to receive(:find_by).with(schema: 'public', name: replacement_name).and_return(nil, replacement_index)
+
           expect_to_execute_concurrently_in_order(create_index)
 
           expect_next_instance_of(::Gitlab::Database::WithLockRetries) do |instance|
             expect(instance).to receive(:run).with(raise_on_exhaustion: true).and_yield
           end
 
-          expect(connection).to receive(:execute).ordered
-          .with("ALTER INDEX #{index.name} RENAME TO #{replaced_name}")
-          .and_raise(ActiveRecord::ConnectionTimeoutError, 'connect timeout')
+          expect_index_rename(index.name, replaced_name).and_raise(ActiveRecord::ConnectionTimeoutError, 'connect timeout')
 
           expect_to_execute_concurrently_in_order(drop_index)
 
-          expect { subject.perform }.to raise_error(described_class::ReindexError, /connect timeout/)
+          expect { subject.perform }.to raise_error(ActiveRecord::ConnectionTimeoutError, /connect timeout/)
 
           check_index_exists
         end
@@ -159,7 +186,6 @@ RSpec.describe Gitlab::Database::Reindexing::ConcurrentReindex, '#perform' do
 
       context 'when with_lock_retries fails to acquire the lock' do
         it 'safely cleans up and signals the error' do
-          expect_to_execute_concurrently_in_order(drop_index)
           expect_to_execute_concurrently_in_order(create_index)
 
           expect_next_instance_of(::Gitlab::Database::WithLockRetries) do |instance|
@@ -169,7 +195,7 @@ RSpec.describe Gitlab::Database::Reindexing::ConcurrentReindex, '#perform' do
 
           expect_to_execute_concurrently_in_order(drop_index)
 
-          expect { subject.perform }.to raise_error(described_class::ReindexError, /exhausted/)
+          expect { subject.perform }.to raise_error(::Gitlab::Database::WithLockRetries::AttemptsExhaustedError, /exhausted/)
 
           check_index_exists
         end
@@ -185,8 +211,11 @@ RSpec.describe Gitlab::Database::Reindexing::ConcurrentReindex, '#perform' do
     end
   end
 
-  def expect_to_execute_in_order(sql)
-    expect(connection).to receive(:execute).with(sql).ordered.and_call_original
+  def expect_index_rename(from, to)
+    expect(connection).to receive(:execute).with(<<~SQL).ordered
+      ALTER INDEX "public"."#{from}"
+      RENAME TO "#{to}"
+    SQL
   end
 
   def find_index_create_statement
diff --git a/spec/lib/gitlab/database/reindexing_spec.rb b/spec/lib/gitlab/database/reindexing_spec.rb
new file mode 100644
index 00000000000..089da92b49d
--- /dev/null
+++ b/spec/lib/gitlab/database/reindexing_spec.rb
@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Database::Reindexing do
+  describe '.perform' do
+    context 'multiple indexes' do
+      let(:indexes) { [double, double] }
+      let(:reindexers) { [double, double] }
+
+      it 'performs concurrent reindexing for each index' do
+        indexes.zip(reindexers).each do |index, reindexer|
+          expect(Gitlab::Database::Reindexing::ConcurrentReindex).to receive(:new).with(index).ordered.and_return(reindexer)
+          expect(reindexer).to receive(:perform)
+        end
+
+        described_class.perform(indexes)
+      end
+    end
+
+    context 'single index' do
+      let(:index) { double }
+      let(:reindexer) { double }
+
+      it 'performs concurrent reindexing for single index' do
+        expect(Gitlab::Database::Reindexing::ConcurrentReindex).to receive(:new).with(index).and_return(reindexer)
+        expect(reindexer).to receive(:perform)
+
+        described_class.perform(index)
+      end
+    end
+  end
+end