Merge branch 'ab/reindexing-remove-pg12-flag' into 'master'

Support reindexing unique indexes

See merge request gitlab-org/gitlab!65966

config/feature_flags/development/database_reindexing_pg12.yml

----
-name: database_reindexing_pg12
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/64695
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/334372
-milestone: '14.1'
-type: development
-group: group::database
-default_enabled: false

doc/administration/raketasks/maintenance.md

@@ -330,7 +330,7 @@ migrations are completed (have an `up` status).
 ## Rebuild database indexes
 
 WARNING:
-This is an experimental feature that isn't enabled by default.
+This is an experimental feature that isn't enabled by default. It requires PostgreSQL 12 or later.
 
 Database indexes can be rebuilt regularly to reclaim space and maintain healthy levels of index bloat over time.
 
@@ -348,7 +348,6 @@ sudo gitlab-rake gitlab:db:reindex['public.a_specific_index']
 
 The following index types are not supported:
 
-1. Unique and primary key indexes
 1. Indexes used for constraint exclusion
 1. Partitioned indexes
 1. Expression indexes

lib/gitlab/database/postgres_index.rb

@@ -18,19 +18,12 @@ module Gitlab
         find(identifier)
       end
 
-      # A 'regular' index is a non-unique index,
-      # that does not serve an exclusion constraint and
-      # is defined on a table that is not partitioned.
-      #
-      # Deprecated: Switch to scope .reindexing_support
-      scope :regular, -> { where(unique: false, partitioned: false, exclusion: false, expression: false, type: Gitlab::Database::Reindexing::SUPPORTED_TYPES)}
-
-      # Indexes for reindexing with PG12
+      # Indexes with reindexing support
       scope :reindexing_support, -> { where(partitioned: false, exclusion: false, expression: false, type: Gitlab::Database::Reindexing::SUPPORTED_TYPES) }
 
-      scope :not_match, ->(regex) { where("name !~ ?", regex)}
+      scope :not_match, ->(regex) { where("name !~ ?", regex) }
 
-      scope :match, ->(regex) { where("name ~* ?", regex)}
+      scope :match, ->(regex) { where("name ~* ?", regex) }
 
       scope :not_recently_reindexed, -> do
         recent_actions = Reindexing::ReindexAction.recent.where('index_identifier = identifier')

lib/gitlab/database/reindexing.rb

@@ -16,16 +16,9 @@ module Gitlab
       end
 
       def self.candidate_indexes
-        indexes = Gitlab::Database::PostgresIndex
-          .not_match("^#{ConcurrentReindex::TEMPORARY_INDEX_PREFIX}")
-          .not_match("^#{ConcurrentReindex::REPLACED_INDEX_PREFIX}")
+        Gitlab::Database::PostgresIndex
           .not_match("#{ReindexConcurrently::TEMPORARY_INDEX_PATTERN}$")
-
-        if Feature.enabled?(:database_reindexing_pg12, type: :development)
-          indexes.reindexing_support
-        else
-          indexes.regular
-        end
+          .reindexing_support
       end
     end
   end

lib/gitlab/database/reindexing/concurrent_reindex.rb

-# frozen_string_literal: true
-
-module Gitlab
-  module Database
-    module Reindexing
-      class ConcurrentReindex
-        include Gitlab::Utils::StrongMemoize
-
-        ReindexError = Class.new(StandardError)
-
-        PG_IDENTIFIER_LENGTH = 63
-        TEMPORARY_INDEX_PREFIX = 'tmp_reindex_'
-        REPLACED_INDEX_PREFIX = 'old_reindex_'
-        STATEMENT_TIMEOUT = 9.hours
-
-        # When dropping an index, we acquire a SHARE UPDATE EXCLUSIVE lock,
-        # which only conflicts with DDL and vacuum. We therefore execute this with a rather
-        # high lock timeout and a long pause in between retries. This is an alternative to
-        # setting a high statement timeout, which would lead to a long running query with effects
-        # on e.g. vacuum.
-        REMOVE_INDEX_RETRY_CONFIG = [[1.minute, 9.minutes]] * 30
-
-        attr_reader :index, :logger
-
-        def initialize(index, logger: Gitlab::AppLogger)
-          @index = index
-          @logger = logger
-        end
-
-        def perform
-          raise ReindexError, 'UNIQUE indexes are currently not supported' if index.unique?
-          raise ReindexError, 'partitioned indexes are currently not supported' if index.partitioned?
-          raise ReindexError, 'indexes serving an exclusion constraint are currently not supported' if index.exclusion?
-          raise ReindexError, 'index is a left-over temporary index from a previous reindexing run' if index.name.start_with?(TEMPORARY_INDEX_PREFIX, REPLACED_INDEX_PREFIX)
-
-          logger.info "Starting reindex of #{index}"
-
-          with_rebuilt_index do |replacement_index|
-            swap_index(replacement_index)
-          end
-        end
-
-        private
-
-        def with_rebuilt_index
-          if Gitlab::Database::PostgresIndex.find_by(schema: index.schema, name: replacement_index_name)
-            logger.debug("dropping dangling index from previous run (if it exists): #{replacement_index_name}")
-            remove_index(index.schema, replacement_index_name)
-          end
-
-          create_replacement_index_statement = index.definition
-            .sub(/CREATE INDEX #{index.name}/, "CREATE INDEX CONCURRENTLY #{replacement_index_name}")
-
-          logger.info("creating replacement index #{replacement_index_name}")
-          logger.debug("replacement index definition: #{create_replacement_index_statement}")
-
-          set_statement_timeout do
-            connection.execute(create_replacement_index_statement)
-          end
-
-          replacement_index = Gitlab::Database::PostgresIndex.find_by(schema: index.schema, name: replacement_index_name)
-
-          unless replacement_index.valid_index?
-            message = 'replacement index was created as INVALID'
-            logger.error("#{message}, cleaning up")
-            raise ReindexError, "failed to reindex #{index}: #{message}"
-          end
-
-          # Some expression indexes (aka functional indexes)
-          # require additional statistics. The existing statistics
-          # are tightly bound to the original index. We have to
-          # rebuild statistics for the new index before dropping
-          # the original one.
-          rebuild_statistics if index.expression?
-
-          yield replacement_index
-        ensure
-          begin
-            remove_index(index.schema, replacement_index_name)
-          rescue StandardError => e
-            logger.error(e)
-          end
-        end
-
-        def swap_index(replacement_index)
-          logger.info("swapping replacement index #{replacement_index} with #{index}")
-
-          with_lock_retries do
-            rename_index(index.schema, index.name, replaced_index_name)
-            rename_index(replacement_index.schema, replacement_index.name, index.name)
-            rename_index(index.schema, replaced_index_name, replacement_index.name)
-          end
-        end
-
-        def rename_index(schema, old_index_name, new_index_name)
-          connection.execute(<<~SQL)
-            ALTER INDEX #{quote_table_name(schema)}.#{quote_table_name(old_index_name)}
-            RENAME TO #{quote_table_name(new_index_name)}
-          SQL
-        end
-
-        def remove_index(schema, name)
-          logger.info("Removing index #{schema}.#{name}")
-
-          retries = Gitlab::Database::WithLockRetriesOutsideTransaction.new(
-            timing_configuration: REMOVE_INDEX_RETRY_CONFIG,
-            klass: self.class,
-            logger: logger
-          )
-
-          retries.run(raise_on_exhaustion: false) do
-            connection.execute(<<~SQL)
-              DROP INDEX CONCURRENTLY
-              IF EXISTS #{quote_table_name(schema)}.#{quote_table_name(name)}
-            SQL
-          end
-        end
-
-        def rebuild_statistics
-          logger.info("rebuilding table statistics for #{index.schema}.#{index.tablename}")
-
-          connection.execute(<<~SQL)
-            ANALYZE #{quote_table_name(index.schema)}.#{quote_table_name(index.tablename)}
-          SQL
-        end
-
-        def replacement_index_name
-          @replacement_index_name ||= "#{TEMPORARY_INDEX_PREFIX}#{index.indexrelid}"
-        end
-
-        def replaced_index_name
-          @replaced_index_name ||= "#{REPLACED_INDEX_PREFIX}#{index.indexrelid}"
-        end
-
-        def with_lock_retries(&block)
-          arguments = { klass: self.class, logger: logger }
-          Gitlab::Database::WithLockRetries.new(**arguments).run(raise_on_exhaustion: true, &block)
-        end
-
-        def set_statement_timeout
-          execute("SET statement_timeout TO '%ds'" % STATEMENT_TIMEOUT)
-          yield
-        ensure
-          execute('RESET statement_timeout')
-        end
-
-        delegate :execute, :quote_table_name, to: :connection
-        def connection
-          @connection ||= ActiveRecord::Base.connection
-        end
-      end
-    end
-  end
-end

lib/gitlab/database/reindexing/coordinator.rb

@@ -41,13 +41,7 @@ module Gitlab
         end
 
         def perform_for(index, action)
-          strategy = if Feature.enabled?(:database_reindexing_pg12, type: :development)
-                       ReindexConcurrently
-                     else
-                       ConcurrentReindex
-                     end
-
-          strategy.new(index).perform
+          ReindexConcurrently.new(index).perform
         rescue StandardError
           action.state = :failed
 

lib/tasks/gitlab/db.rake

@@ -154,7 +154,7 @@ namespace :gitlab do
       Rake::Task['gitlab:db:create_dynamic_partitions'].invoke
     end
 
-    desc 'reindex a regular (non-unique) index without downtime to eliminate bloat'
+    desc 'reindex a regular index without downtime to eliminate bloat'
     task :reindex, [:index_name] => :environment do |_, args|
       unless Feature.enabled?(:database_reindexing, type: :ops)
         puts "This feature (database_reindexing) is currently disabled.".color(:yellow)

spec/lib/gitlab/database/postgres_index_spec.rb

@@ -22,30 +22,6 @@ RSpec.describe Gitlab::Database::PostgresIndex do
 
   it_behaves_like 'a postgres model'
 
-  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
-
-    it 'only non-expression indexes' do
-      expect(described_class.regular).to all(have_attributes(expression: false))
-    end
-
-    it 'only btree and gist indexes' do
-      types = described_class.regular.map(&:type).uniq
-
-      expect(types & %w(btree gist)).to eq(types)
-    end
-  end
-
   describe '.reindexing_support' do
     it 'only non partitioned indexes' do
       expect(described_class.reindexing_support).to all(have_attributes(partitioned: false))

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

@@ -11,7 +11,7 @@ RSpec.describe Gitlab::Database::Reindexing::Coordinator do
 
     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::ConcurrentReindex, perform: nil) }
+    let(:reindexer) { instance_double(Gitlab::Database::Reindexing::ReindexConcurrently, perform: nil) }
     let(:action) { create(:reindex_action, index: index) }
 
     let!(:lease) { stub_exclusive_lease(lease_key, uuid, timeout: lease_timeout) }
@@ -19,83 +19,64 @@ RSpec.describe Gitlab::Database::Reindexing::Coordinator do
     let(:lease_timeout) { 1.day }
     let(:uuid) { 'uuid' }
 
-    shared_examples_for 'reindexing coordination' do
-      context 'locking' do
-        it 'acquires a lock while reindexing' do
-          expect(lease).to receive(:try_obtain).ordered.and_return(uuid)
+    before do
+      swapout_view_for_table(:postgres_indexes)
 
-          expect(reindexer).to receive(:perform).ordered
+      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
 
-          expect(Gitlab::ExclusiveLease).to receive(:cancel).ordered.with(lease_key, uuid)
+    context 'locking' do
+      it 'acquires a lock while reindexing' do
+        expect(lease).to receive(:try_obtain).ordered.and_return(uuid)
 
-          subject
-        end
+        expect(reindexer).to receive(:perform).ordered
 
-        it 'does not perform reindexing actions if lease is not granted' do
-          expect(lease).to receive(:try_obtain).ordered.and_return(false)
-          expect(Gitlab::Database::Reindexing::ConcurrentReindex).not_to receive(:new)
+        expect(Gitlab::ExclusiveLease).to receive(:cancel).ordered.with(lease_key, uuid)
 
-          subject
-        end
+        subject
       end
 
-      context 'notifications' do
-        it 'sends #notify_start before reindexing' do
-          expect(notifier).to receive(:notify_start).with(action).ordered
-          expect(reindexer).to receive(:perform).ordered
-
-          subject
-        end
+      it 'does not perform reindexing actions if lease is not granted' do
+        expect(lease).to receive(:try_obtain).ordered.and_return(false)
+        expect(Gitlab::Database::Reindexing::ReindexConcurrently).not_to receive(:new)
 
-        it 'sends #notify_end after reindexing and updating the action is done' do
-          expect(action).to receive(:finish).ordered
-          expect(notifier).to receive(:notify_end).with(action).ordered
-
-          subject
-        end
+        subject
       end
+    end
 
-      context 'action tracking' do
-        it 'calls #finish on the action' do
-          expect(reindexer).to receive(:perform).ordered
-          expect(action).to receive(:finish).ordered
-
-          subject
-        end
+    context 'notifications' do
+      it 'sends #notify_start before reindexing' do
+        expect(notifier).to receive(:notify_start).with(action).ordered
+        expect(reindexer).to receive(:perform).ordered
 
-        it 'upon error, it still calls finish and raises the error' do
-          expect(reindexer).to receive(:perform).ordered.and_raise('something went wrong')
-          expect(action).to receive(:finish).ordered
+        subject
+      end
 
-          expect { subject }.to raise_error(/something went wrong/)
+      it 'sends #notify_end after reindexing and updating the action is done' do
+        expect(action).to receive(:finish).ordered
+        expect(notifier).to receive(:notify_end).with(action).ordered
 
-          expect(action).to be_failed
-        end
+        subject
       end
     end
 
-    context 'legacy reindexing method (< PG12) - to be removed' do
-      before do
-        stub_feature_flags(database_reindexing_pg12: false)
-        swapout_view_for_table(:postgres_indexes)
+    context 'action tracking' do
+      it 'calls #finish on the action' do
+        expect(reindexer).to receive(:perform).ordered
+        expect(action).to receive(:finish).ordered
 
-        allow(Gitlab::Database::Reindexing::ConcurrentReindex).to receive(:new).with(index).and_return(reindexer)
-        allow(Gitlab::Database::Reindexing::ReindexAction).to receive(:create_for).with(index).and_return(action)
+        subject
       end
 
-      it_behaves_like 'reindexing coordination'
-    end
+      it 'upon error, it still calls finish and raises the error' do
+        expect(reindexer).to receive(:perform).ordered.and_raise('something went wrong')
+        expect(action).to receive(:finish).ordered
 
-    context 'PG12 reindexing method' do
-      before do
-        stub_feature_flags(database_reindexing_pg12: true)
-        swapout_view_for_table(:postgres_indexes)
+        expect { subject }.to raise_error(/something went wrong/)
 
-        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)
+        expect(action).to be_failed
       end
-
-      it_behaves_like 'reindexing coordination'
     end
   end
 end

spec/lib/gitlab/database/reindexing_spec.rb

@@ -29,30 +29,11 @@ RSpec.describe Gitlab::Database::Reindexing do
   describe '.candidate_indexes' do
     subject { described_class.candidate_indexes }
 
-    context 'with deprecated method for < PG12' do
-      before do
-        stub_feature_flags(database_reindexing_pg12: false)
-      end
-
-      it 'retrieves regular indexes that are no left-overs from previous runs' do
-        result = double
-        expect(Gitlab::Database::PostgresIndex).to receive_message_chain('not_match.not_match.not_match.regular').with('^tmp_reindex_').with('^old_reindex_').with('\_ccnew[0-9]*$').with(no_args).and_return(result)
-
-        expect(subject).to eq(result)
-      end
-    end
+    it 'retrieves regular indexes that are no left-overs from previous runs' do
+      result = double
+      expect(Gitlab::Database::PostgresIndex).to receive_message_chain('not_match.reindexing_support').with('\_ccnew[0-9]*$').with(no_args).and_return(result)
 
-    context 'with deprecated method for >= PG12' do
-      before do
-        stub_feature_flags(database_reindexing_pg12: true)
-      end
-
-      it 'retrieves regular indexes that are no left-overs from previous runs' do
-        result = double
-        expect(Gitlab::Database::PostgresIndex).to receive_message_chain('not_match.not_match.not_match.reindexing_support').with('^tmp_reindex_').with('^old_reindex_').with('\_ccnew[0-9]*$').with(no_args).and_return(result)
-
-        expect(subject).to eq(result)
-      end
+      expect(subject).to eq(result)
     end
   end
 end