Merge branch 'add_remove_concurrent_index_to_database_helper' into 'master'

Add remove_concurrent_index to database helper

Closes #30376

See merge request !10441

changelogs/unreleased/add_remove_concurrent_index_to_database_helper.yml

+---
+title: Add remove_concurrent_index to database helper
+merge_request: 10441
+author: blackst0ne

db/migrate/20160615142710_add_index_on_requested_at_to_members.rb

+# rubocop:disable RemoveIndex
 class AddIndexOnRequestedAtToMembers < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20160616103005_remove_keys_fingerprint_index_if_exists.rb

+# rubocop:disable RemoveIndex
 class RemoveKeysFingerprintIndexIfExists < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20160616103948_add_unique_index_to_keys_fingerprint.rb

+# rubocop:disable RemoveIndex
 class AddUniqueIndexToKeysFingerprint < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20160620115026_add_index_on_runners_locked.rb

 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 
+# rubocop:disable RemoveIndex
 class AddIndexOnRunnersLocked < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20160715134306_add_index_for_pipeline_user_id.rb

+# rubocop:disable RemoveIndex
 class AddIndexForPipelineUserId < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20160725104020_merge_request_diff_remove_uniq.rb

 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 
+# rubocop:disable RemoveIndex
 class MergeRequestDiffRemoveUniq < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
   disable_ddl_transaction!

db/migrate/20160725104452_merge_request_diff_add_index.rb

+# rubocop:disable RemoveIndex
 class MergeRequestDiffAddIndex < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
   disable_ddl_transaction!

db/migrate/20160802010328_remove_builds_enable_index_on_projects.rb

+# rubocop:disable RemoveIndex
 class RemoveBuildsEnableIndexOnProjects < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20160803161903_add_unique_index_to_lists_label_id.rb

+# rubocop:disable RemoveIndex
 class AddUniqueIndexToListsLabelId < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20160805041956_add_deleted_at_to_namespaces.rb

+# rubocop:disable RemoveIndex
 class AddDeletedAtToNamespaces < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20160808085602_add_index_for_build_token.rb

+# rubocop:disable RemoveIndex
 class AddIndexForBuildToken < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20160810142633_remove_redundant_indexes.rb

 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 
+# rubocop:disable RemoveIndex
 class RemoveRedundantIndexes < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20160819221631_add_index_to_note_discussion_id.rb

 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 
+# rubocop:disable RemoveIndex
 class AddIndexToNoteDiscussionId < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20160819232256_add_incoming_email_token_to_users.rb

 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 
+# rubocop:disable RemoveIndex
 class AddIncomingEmailTokenToUsers < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20160919145149_add_group_id_to_labels.rb

+# rubocop:disable RemoveIndex
 class AddGroupIdToLabels < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20160920160832_add_index_to_labels_title.rb

+# rubocop:disable RemoveIndex
 class AddIndexToLabelsTitle < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20161017125927_add_unique_index_to_labels.rb

+# rubocop:disable RemoveIndex
 class AddUniqueIndexToLabels < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20161020083353_add_pipeline_id_to_merge_request_metrics.rb

 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 
+# rubocop:disable RemoveIndex
 class AddPipelineIdToMergeRequestMetrics < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20161031181638_add_unique_index_to_subscriptions.rb

+# rubocop:disable RemoveIndex
 class AddUniqueIndexToSubscriptions < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20161106185620_add_project_import_data_project_index.rb

+# rubocop:disable RemoveIndex
 class AddProjectImportDataProjectIndex < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20161124111395_add_index_to_parent_id.rb

 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 
+# rubocop:disable RemoveIndex
 class AddIndexToParentId < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20161128142110_remove_unnecessary_indexes.rb

 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 
+# rubocop:disable RemoveIndex
 class RemoveUnnecessaryIndexes < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
   disable_ddl_transaction!

db/migrate/20161202152035_add_index_to_routes.rb

 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 
+# rubocop:disable RemoveIndex
 class AddIndexToRoutes < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20161206153749_remove_uniq_path_index_from_namespace.rb

 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 
+# rubocop:disable RemoveIndex
 class RemoveUniqPathIndexFromNamespace < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20161206153751_add_path_index_to_namespace.rb

 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 
+# rubocop:disable RemoveIndex
 class AddPathIndexToNamespace < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20161206153753_remove_uniq_name_index_from_namespace.rb

 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 
+# rubocop:disable RemoveIndex
 class RemoveUniqNameIndexFromNamespace < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20161206153754_add_name_index_to_namespace.rb

 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 
+# rubocop:disable RemoveIndex
 class AddNameIndexToNamespace < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20161207231621_create_environment_name_unique_index.rb

+# rubocop:disable RemoveIndex
 class CreateEnvironmentNameUniqueIndex < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20161209153400_add_unique_index_for_environment_slug.rb

 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 
+# rubocop:disable RemoveIndex
 class AddUniqueIndexForEnvironmentSlug < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20161212142807_add_lower_path_index_to_routes.rb

 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 
+# rubocop:disable RemoveIndex
 class AddLowerPathIndexToRoutes < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20170121123724_add_index_to_ci_builds_for_status_runner_id_and_type.rb

+# rubocop:disable RemoveIndex
 class AddIndexToCiBuildsForStatusRunnerIdAndType < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20170121130655_add_index_to_ci_runners_for_is_shared.rb

+# rubocop:disable RemoveIndex
 class AddIndexToCiRunnersForIsShared < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20170130204620_add_index_to_project_authorizations.rb

+# rubocop:disable RemoveIndex
 class AddIndexToProjectAuthorizations < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20170131221752_add_relative_position_to_issues.rb

 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 
+# rubocop:disable RemoveIndex
 class AddRelativePositionToIssues < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20170204181513_add_index_to_labels_for_type_and_project.rb

+# rubocop:disable RemoveIndex
 class AddIndexToLabelsForTypeAndProject < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20170210062829_add_index_to_labels_for_title_and_project.rb

+# rubocop:disable RemoveIndex
 class AddIndexToLabelsForTitleAndProject < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20170210075922_add_index_to_ci_trigger_requests_for_commit_id.rb

+# rubocop:disable RemoveIndex
 class AddIndexToCiTriggerRequestsForCommitId < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20170210103609_add_index_to_user_agent_detail.rb

 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 
+# rubocop:disable RemoveIndex
 class AddIndexToUserAgentDetail < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20170216135621_add_index_for_latest_successful_pipeline.rb

+# rubocop:disable RemoveIndex
 class AddIndexForLatestSuccessfulPipeline < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
   DOWNTIME = false

db/migrate/20170216141440_drop_index_for_builds_project_status.rb

+# rubocop:disable RemoveIndex
 class DropIndexForBuildsProjectStatus < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
   DOWNTIME = false

db/migrate/20170222143500_remove_old_project_id_columns.rb

+# rubocop:disable RemoveIndex
 class RemoveOldProjectIdColumns < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
   disable_ddl_transaction!

db/migrate/20170313213916_add_index_to_user_ghost.rb

 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 
+# rubocop:disable RemoveIndex
 class AddIndexToUserGhost < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

doc/development/migration_style_guide.md

@@ -58,10 +58,22 @@ migration was tested.
 
 ## Removing indices
 
-If you need to remove index, please add a condition like in following example:
+When removing an index make sure to use the method `remove_concurrent_index` instead
+of the regular `remove_index` method. The `remove_concurrent_index` method
+automatically drops concurrent indexes when using PostgreSQL, removing the
+need for downtime. To use this method you must disable transactions by calling
+the method `disable_ddl_transaction!` in the body of your migration class like
+so:
 
 ```ruby
-remove_index :namespaces, column: :name if index_exists?(:namespaces, :name)
+class MyMigration < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+  disable_ddl_transaction!
+
+  def up
+    remove_concurrent_index :table_name, :column_name if index_exists?(:table_name, :column_name)
+  end
+end
 ```
 
 ## Adding indices

generator_templates/active_record/migration/create_table_migration.rb

@@ -12,12 +12,14 @@ class <%= migration_class_name %> < ActiveRecord::Migration
   # migration requires downtime.
   # DOWNTIME_REASON = ''
 
-  # When using the methods "add_concurrent_index" or "add_column_with_default"
-  # you must disable the use of transactions as these methods can not run in an
-  # existing transaction. When using "add_concurrent_index" make sure that this
-  # method is the _only_ method called in the migration, any other changes
-  # should go in a separate migration. This ensures that upon failure _only_ the
-  # index creation fails and can be retried or reverted easily.
+  # When using the methods "add_concurrent_index", "remove_concurrent_index" or
+  # "add_column_with_default" you must disable the use of transactions
+  # as these methods can not run in an existing transaction.
+  # When using "add_concurrent_index" or "remove_concurrent_index" methods make sure
+  # that either of them is the _only_ method called in the migration,
+  # any other changes should go in a separate migration.
+  # This ensures that upon failure _only_ the index creation or removing fails
+  # and can be retried or reverted easily.
   #
   # To disable transactions uncomment the following line and remove these
   # comments:

generator_templates/active_record/migration/migration.rb

@@ -12,12 +12,14 @@ class <%= migration_class_name %> < ActiveRecord::Migration
   # migration requires downtime.
   # DOWNTIME_REASON = ''
 
-  # When using the methods "add_concurrent_index" or "add_column_with_default"
-  # you must disable the use of transactions as these methods can not run in an
-  # existing transaction. When using "add_concurrent_index" make sure that this
-  # method is the _only_ method called in the migration, any other changes
-  # should go in a separate migration. This ensures that upon failure _only_ the
-  # index creation fails and can be retried or reverted easily.
+  # When using the methods "add_concurrent_index", "remove_concurrent_index" or
+  # "add_column_with_default" you must disable the use of transactions
+  # as these methods can not run in an existing transaction.
+  # When using "add_concurrent_index" or "remove_concurrent_index" methods make sure
+  # that either of them is the _only_ method called in the migration,
+  # any other changes should go in a separate migration.
+  # This ensures that upon failure _only_ the index creation or removing fails
+  # and can be retried or reverted easily.
   #
   # To disable transactions uncomment the following line and remove these
   # comments:

generator_templates/rails/post_deployment_migration/migration.rb

@@ -6,12 +6,14 @@ class <%= migration_class_name %> < ActiveRecord::Migration
 
   DOWNTIME = false
 
-  # When using the methods "add_concurrent_index" or "add_column_with_default"
-  # you must disable the use of transactions as these methods can not run in an
-  # existing transaction. When using "add_concurrent_index" make sure that this
-  # method is the _only_ method called in the migration, any other changes
-  # should go in a separate migration. This ensures that upon failure _only_ the
-  # index creation fails and can be retried or reverted easily.
+  # When using the methods "add_concurrent_index", "remove_concurrent_index" or
+  # "add_column_with_default" you must disable the use of transactions
+  # as these methods can not run in an existing transaction.
+  # When using "add_concurrent_index" or "remove_concurrent_index" methods make sure
+  # that either of them is the _only_ method called in the migration,
+  # any other changes should go in a separate migration.
+  # This ensures that upon failure _only_ the index creation or removing fails
+  # and can be retried or reverted easily.
   #
   # To disable transactions uncomment the following line and remove these
   # comments:

lib/gitlab/database/migration_helpers.rb

@@ -26,6 +26,30 @@ module Gitlab
         add_index(table_name, column_name, options)
       end
 
+      # Removes an existed index, concurrently when supported
+      #
+      # On PostgreSQL this method removes an index concurrently.
+      #
+      # Example:
+      #
+      #     remove_concurrent_index :users, :some_column
+      #
+      # See Rails' `remove_index` for more info on the available arguments.
+      def remove_concurrent_index(table_name, column_name, options = {})
+        if transaction_open?
+          raise 'remove_concurrent_index can not be run inside a transaction, ' \
+            'you can disable transactions by calling disable_ddl_transaction! ' \
+            'in the body of your migration class'
+        end
+
+        if Database.postgresql?
+          options = options.merge({ algorithm: :concurrently })
+          disable_statement_timeout
+        end
+
+        remove_index(table_name, options.merge({ column: column_name }))
+      end
+
       # Adds a foreign key with only minimal locking on the tables involved.
       #
       # This method only requires minimal locking when using PostgreSQL. When

rubocop/cop/migration/add_concurrent_index.rb

@@ -9,7 +9,7 @@ module RuboCop
         include MigrationHelpers
 
         MSG = '`add_concurrent_index` is not reversible so you must manually define ' \
-          'the `up` and `down` methods in your migration class, using `remove_index` in `down`'.freeze
+          'the `up` and `down` methods in your migration class, using `remove_concurrent_index` in `down`'.freeze
 
         def on_send(node)
           return unless in_migration?(node)

rubocop/cop/migration/remove_concurrent_index.rb

+require_relative '../../migration_helpers'
+
+module RuboCop
+  module Cop
+    module Migration
+      # Cop that checks if `remove_concurrent_index` is used with `up`/`down` methods
+      # and not `change`.
+      class RemoveConcurrentIndex < RuboCop::Cop::Cop
+        include MigrationHelpers
+
+        MSG = '`remove_concurrent_index` is not reversible so you must manually define ' \
+          'the `up` and `down` methods in your migration class, using `add_concurrent_index` in `down`'.freeze
+
+        def on_send(node)
+          return unless in_migration?(node)
+          return unless node.children[1] == :remove_concurrent_index
+
+          node.each_ancestor(:def) do |def_node|
+            add_offense(def_node, :name) if method_name(def_node) == :change
+          end
+        end
+
+        def method_name(node)
+          node.children[0]
+        end
+      end
+    end
+  end
+end

rubocop/cop/migration/remove_index.rb

+require_relative '../../migration_helpers'
+
+module RuboCop
+  module Cop
+    module Migration
+      # Cop that checks if indexes are removed in a concurrent manner.
+      class RemoveIndex < RuboCop::Cop::Cop
+        include MigrationHelpers
+
+        MSG = '`remove_index` requires downtime, use `remove_concurrent_index` instead'.freeze
+
+        def on_def(node)
+          return unless in_migration?(node)
+
+          node.each_descendant(:send) do |send_node|
+            add_offense(send_node, :selector) if method_name(send_node) == :remove_index
+          end
+        end
+
+        def method_name(node)
+          node.children[1]
+        end
+      end
+    end
+  end
+end

rubocop/rubocop.rb

@@ -5,3 +5,5 @@ require_relative 'cop/migration/add_column_with_default'
 require_relative 'cop/migration/add_concurrent_foreign_key'
 require_relative 'cop/migration/add_concurrent_index'
 require_relative 'cop/migration/add_index'
+require_relative 'cop/migration/remove_concurrent_index'
+require_relative 'cop/migration/remove_index'

spec/lib/gitlab/database/migration_helpers_spec.rb

@@ -58,6 +58,48 @@ describe Gitlab::Database::MigrationHelpers, lib: true do
     end
   end
 
+  describe '#remove_concurrent_index' do
+    context 'outside a transaction' do
+      before do
+        allow(model).to receive(:transaction_open?).and_return(false)
+      end
+
+      context 'using PostgreSQL' do
+        before do
+          allow(Gitlab::Database).to receive(:postgresql?).and_return(true)
+          allow(model).to receive(:disable_statement_timeout)
+        end
+
+        it 'removes the index concurrently' do
+          expect(model).to receive(:remove_index).
+            with(:users, { algorithm: :concurrently, column: :foo })
+
+          model.remove_concurrent_index(:users, :foo)
+        end
+      end
+
+      context 'using MySQL' do
+        it 'removes an index' do
+          expect(Gitlab::Database).to receive(:postgresql?).and_return(false)
+
+          expect(model).to receive(:remove_index).
+            with(:users, { column: :foo })
+
+          model.remove_concurrent_index(:users, :foo)
+        end
+      end
+    end
+
+    context 'inside a transaction' do
+      it 'raises RuntimeError' do
+        expect(model).to receive(:transaction_open?).and_return(true)
+
+        expect { model.remove_concurrent_index(:users, :foo) }.
+          to raise_error(RuntimeError)
+      end
+    end
+  end
+
   describe '#add_concurrent_foreign_key' do
     context 'inside a transaction' do
       it 'raises an error' do

spec/rubocop/cop/migration/remove_concurrent_index_spec.rb

+require 'spec_helper'
+
+require 'rubocop'
+require 'rubocop/rspec/support'
+
+require_relative '../../../../rubocop/cop/migration/remove_concurrent_index'
+
+describe RuboCop::Cop::Migration::RemoveConcurrentIndex do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  context 'in migration' do
+    before do
+      allow(cop).to receive(:in_migration?).and_return(true)
+    end
+
+    it 'registers an offense when remove_concurrent_index is used inside a change method' do
+      inspect_source(cop, 'def change; remove_concurrent_index :table, :column; end')
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(1)
+        expect(cop.offenses.map(&:line)).to eq([1])
+      end
+    end
+
+    it 'registers no offense when remove_concurrent_index is used inside an up method' do
+      inspect_source(cop, 'def up; remove_concurrent_index :table, :column; end')
+
+      expect(cop.offenses.size).to eq(0)
+    end
+  end
+
+  context 'outside of migration' do
+    it 'registers no offense' do
+      inspect_source(cop, 'def change; remove_concurrent_index :table, :column; end')
+
+      expect(cop.offenses.size).to eq(0)
+    end
+  end
+end

spec/rubocop/cop/migration/remove_index_spec.rb

+require 'spec_helper'
+
+require 'rubocop'
+require 'rubocop/rspec/support'
+
+require_relative '../../../../rubocop/cop/migration/remove_index'
+
+describe RuboCop::Cop::Migration::RemoveIndex do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  context 'in migration' do
+    before do
+      allow(cop).to receive(:in_migration?).and_return(true)
+    end
+
+    it 'registers an offense when remove_index is used' do
+      inspect_source(cop, 'def change; remove_index :table, :column; end')
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(1)
+        expect(cop.offenses.map(&:line)).to eq([1])
+      end
+    end
+  end
+
+  context 'outside of migration' do
+    it 'registers no offense' do
+      inspect_source(cop, 'def change; remove_index :table, :column; end')
+
+      expect(cop.offenses.size).to eq(0)
+    end
+  end
+end