Merge branch 'fix/gb/improve-updating-column-in-batches-helper' into 'master'

Improve `update_column_in_batches` migration helper

Closes #34064

See merge request !12350

19 files changed, 77 insertions, 30 deletions

diff --git a/db/migrate/20160615191922_set_missing_stage_on_ci_builds.rb b/db/migrate/20160615191922_set_missing_stage_on_ci_builds.rb
index 4d6a61bd614..5336b036bca 100644
--- a/db/migrate/20160615191922_set_missing_stage_on_ci_builds.rb
+++ b/db/migrate/20160615191922_set_missing_stage_on_ci_builds.rb
@@ -2,6 +2,8 @@
 class SetMissingStageOnCiBuilds < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 
+  disable_ddl_transaction!
+
   def up
     update_column_in_batches(:ci_builds, :stage, :test) do |table, query|
       query.where(table[:stage].eq(nil))
diff --git a/db/migrate/20160721081015_drop_and_readd_has_external_wiki_in_projects.rb b/db/migrate/20160721081015_drop_and_readd_has_external_wiki_in_projects.rb
index b2a2ce41391..abe8e701e23 100644
--- a/db/migrate/20160721081015_drop_and_readd_has_external_wiki_in_projects.rb
+++ b/db/migrate/20160721081015_drop_and_readd_has_external_wiki_in_projects.rb
@@ -5,6 +5,8 @@ class DropAndReaddHasExternalWikiInProjects < ActiveRecord::Migration
   # Set this constant to true if this migration requires downtime.
   DOWNTIME = false
 
+  disable_ddl_transaction!
+
   def up
     update_column_in_batches(:projects, :has_external_wiki, nil) do |table, query|
       query.where(table[:has_external_wiki].not_eq(nil))
diff --git a/db/migrate/20160901141443_set_confidential_issues_events_on_webhooks.rb b/db/migrate/20160901141443_set_confidential_issues_events_on_webhooks.rb
index febd2c0e65e..f8486e3e1a6 100644
--- a/db/migrate/20160901141443_set_confidential_issues_events_on_webhooks.rb
+++ b/db/migrate/20160901141443_set_confidential_issues_events_on_webhooks.rb
@@ -4,6 +4,8 @@ class SetConfidentialIssuesEventsOnWebhooks < ActiveRecord::Migration
 
   DOWNTIME = false
 
+  disable_ddl_transaction!
+
   def up
     update_column_in_batches(:web_hooks, :confidential_issues_events, true) do |table, query|
       query.where(table[:issues_events].eq(true))
diff --git a/db/migrate/20160919144305_add_type_to_labels.rb b/db/migrate/20160919144305_add_type_to_labels.rb
index 2d2725ccf59..d08b339cd27 100644
--- a/db/migrate/20160919144305_add_type_to_labels.rb
+++ b/db/migrate/20160919144305_add_type_to_labels.rb
@@ -5,6 +5,8 @@ class AddTypeToLabels < ActiveRecord::Migration
   DOWNTIME = true
   DOWNTIME_REASON = 'Labels will not work as expected until this migration is complete.'
 
+  disable_ddl_transaction!
+
   def change
     add_column :labels, :type, :string
 
diff --git a/db/migrate/20161018124658_make_project_owners_masters.rb b/db/migrate/20161018124658_make_project_owners_masters.rb
index fe11699c196..cb93b449067 100644
--- a/db/migrate/20161018124658_make_project_owners_masters.rb
+++ b/db/migrate/20161018124658_make_project_owners_masters.rb
@@ -4,6 +4,8 @@ class MakeProjectOwnersMasters < ActiveRecord::Migration
 
   DOWNTIME = false
 
+  disable_ddl_transaction!
+
   def up
     update_column_in_batches(:members, :access_level, 40) do |table, query|
       query.where(table[:access_level].eq(50).and(table[:source_type].eq('Project')))
diff --git a/db/migrate/20161227192806_rename_slack_and_mattermost_notification_services.rb b/db/migrate/20161227192806_rename_slack_and_mattermost_notification_services.rb
index c7cada6dfc5..6b15e5caccf 100644
--- a/db/migrate/20161227192806_rename_slack_and_mattermost_notification_services.rb
+++ b/db/migrate/20161227192806_rename_slack_and_mattermost_notification_services.rb
@@ -4,6 +4,8 @@ class RenameSlackAndMattermostNotificationServices < ActiveRecord::Migration
 
   DOWNTIME = false
 
+  disable_ddl_transaction!
+
   def up
     update_column_in_batches(:services, :type, 'SlackService') do |table, query|
       query.where(table[:type].eq('SlackNotificationService'))
diff --git a/db/post_migrate/20170309171644_reset_relative_position_for_issue.rb b/db/post_migrate/20170309171644_reset_relative_position_for_issue.rb
index b1c9eed1148..01fff680183 100644
--- a/db/post_migrate/20170309171644_reset_relative_position_for_issue.rb
+++ b/db/post_migrate/20170309171644_reset_relative_position_for_issue.rb
@@ -4,6 +4,8 @@ class ResetRelativePositionForIssue < ActiveRecord::Migration
 
   DOWNTIME = false
 
+  disable_ddl_transaction!
+
   def up
     update_column_in_batches(:issues, :relative_position, nil) do |table, query|
       query.where(table[:relative_position].not_eq(nil))
@@ -11,5 +13,6 @@ class ResetRelativePositionForIssue < ActiveRecord::Migration
   end
 
   def down
+    # noop
   end
 end
diff --git a/db/post_migrate/20170317162059_update_upload_paths_to_system.rb b/db/post_migrate/20170317162059_update_upload_paths_to_system.rb
index 9a77b0bbdfb..ca2912f8dce 100644
--- a/db/post_migrate/20170317162059_update_upload_paths_to_system.rb
+++ b/db/post_migrate/20170317162059_update_upload_paths_to_system.rb
@@ -7,6 +7,8 @@ class UpdateUploadPathsToSystem < ActiveRecord::Migration
   DOWNTIME = false
   AFFECTED_MODELS = %w(User Project Note Namespace Appearance)
 
+  disable_ddl_transaction!
+
   def up
     update_column_in_batches(:uploads, :path, replace_sql(arel_table[:path], base_directory, new_upload_dir)) do |_table, query|
       query.where(uploads_to_switch_to_new_path)
diff --git a/db/post_migrate/20170406142253_migrate_user_project_view.rb b/db/post_migrate/20170406142253_migrate_user_project_view.rb
index 22f0f2ac200..c4e910b3b44 100644
--- a/db/post_migrate/20170406142253_migrate_user_project_view.rb
+++ b/db/post_migrate/20170406142253_migrate_user_project_view.rb
@@ -7,6 +7,8 @@ class MigrateUserProjectView < ActiveRecord::Migration
   # Set this constant to true if this migration requires downtime.
   DOWNTIME = false
 
+  disable_ddl_transaction!
+
   def up
     update_column_in_batches(:users, :project_view, 2) do |table, query|
       query.where(table[:project_view].eq(0))
diff --git a/db/post_migrate/20170508170547_add_head_pipeline_for_each_merge_request.rb b/db/post_migrate/20170508170547_add_head_pipeline_for_each_merge_request.rb
index 0a4a2d3867a..f77078ddd70 100644
--- a/db/post_migrate/20170508170547_add_head_pipeline_for_each_merge_request.rb
+++ b/db/post_migrate/20170508170547_add_head_pipeline_for_each_merge_request.rb
@@ -3,6 +3,8 @@ class AddHeadPipelineForEachMergeRequest < ActiveRecord::Migration
 
   DOWNTIME = false
 
+  disable_ddl_transaction!
+
   def up
     disable_statement_timeout
 
diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb
index 60cce9c6d9e..0643c56db9b 100644
--- a/lib/gitlab/database/migration_helpers.rb
+++ b/lib/gitlab/database/migration_helpers.rb
@@ -222,6 +222,12 @@ module Gitlab
       #
       # rubocop: disable Metrics/AbcSize
       def update_column_in_batches(table, column, value)
+        if transaction_open?
+          raise 'update_column_in_batches can not be run inside a transaction, ' \
+            'you can disable transactions by calling disable_ddl_transaction! ' \
+            'in the body of your migration class'
+        end
+
         table = Arel::Table.new(table)
 
         count_arel = table.project(Arel.star.count.as('count'))
diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb
index 6a0485112c1..4259be3f522 100644
--- a/spec/lib/gitlab/database/migration_helpers_spec.rb
+++ b/spec/lib/gitlab/database/migration_helpers_spec.rb
@@ -262,39 +262,53 @@ describe Gitlab::Database::MigrationHelpers, lib: true do
   end
 
   describe '#update_column_in_batches' do
-    before do
-      create_list(:empty_project, 5)
-    end
+    context 'when running outside of a transaction' do
+      before do
+        expect(model).to receive(:transaction_open?).and_return(false)
 
-    it 'updates all the rows in a table' do
-      model.update_column_in_batches(:projects, :import_error, 'foo')
+        create_list(:empty_project, 5)
+      end
 
-      expect(Project.where(import_error: 'foo').count).to eq(5)
-    end
+      it 'updates all the rows in a table' do
+        model.update_column_in_batches(:projects, :import_error, 'foo')
 
-    it 'updates boolean values correctly' do
-      model.update_column_in_batches(:projects, :archived, true)
+        expect(Project.where(import_error: 'foo').count).to eq(5)
+      end
 
-      expect(Project.where(archived: true).count).to eq(5)
-    end
+      it 'updates boolean values correctly' do
+        model.update_column_in_batches(:projects, :archived, true)
+
+        expect(Project.where(archived: true).count).to eq(5)
+      end
+
+      context 'when a block is supplied' do
+        it 'yields an Arel table and query object to the supplied block' do
+          first_id = Project.first.id
 
-    context 'when a block is supplied' do
-      it 'yields an Arel table and query object to the supplied block' do
-        first_id = Project.first.id
+          model.update_column_in_batches(:projects, :archived, true) do |t, query|
+            query.where(t[:id].eq(first_id))
+          end
 
-        model.update_column_in_batches(:projects, :archived, true) do |t, query|
-          query.where(t[:id].eq(first_id))
+          expect(Project.where(archived: true).count).to eq(1)
         end
+      end
+
+      context 'when the value is Arel.sql (Arel::Nodes::SqlLiteral)' do
+        it 'updates the value as a SQL expression' do
+          model.update_column_in_batches(:projects, :star_count, Arel.sql('1+1'))
 
-        expect(Project.where(archived: true).count).to eq(1)
+          expect(Project.sum(:star_count)).to eq(2 * Project.count)
+        end
       end
     end
 
-    context 'when the value is Arel.sql (Arel::Nodes::SqlLiteral)' do
-      it 'updates the value as a SQL expression' do
-        model.update_column_in_batches(:projects, :star_count, Arel.sql('1+1'))
+    context 'when running inside the transaction' do
+      it 'raises RuntimeError' do
+        expect(model).to receive(:transaction_open?).and_return(true)
 
-        expect(Project.sum(:star_count)).to eq(2 * Project.count)
+        expect do
+          model.update_column_in_batches(:projects, :star_count, Arel.sql('1+1'))
+        end.to raise_error(RuntimeError)
       end
     end
   end
@@ -303,7 +317,9 @@ describe Gitlab::Database::MigrationHelpers, lib: true do
     context 'outside of a transaction' do
       context 'when a column limit is not set' do
         before do
-          expect(model).to receive(:transaction_open?).and_return(false)
+          expect(model).to receive(:transaction_open?)
+            .and_return(false)
+            .at_least(:once)
 
           expect(model).to receive(:transaction).and_yield
 
@@ -810,7 +826,11 @@ describe Gitlab::Database::MigrationHelpers, lib: true do
       let!(:user) { create(:user, name: 'Kathy Alice Aliceson') }
 
       it 'replaces the correct part of the string' do
-        model.update_column_in_batches(:users, :name, model.replace_sql(Arel::Table.new(:users)[:name], 'Alice', 'Eve'))
+        allow(model).to receive(:transaction_open?).and_return(false)
+        query = model.replace_sql(Arel::Table.new(:users)[:name], 'Alice', 'Eve')
+
+        model.update_column_in_batches(:users, :name, query)
+
         expect(user.reload.name).to eq('Kathy Eve Aliceson')
       end
     end
diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb
index a3ab4e3dd9e..5653cfee686 100644
--- a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb
+++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameBase do
+describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameBase, :truncate do
   let(:migration) { FakeRenameReservedPathMigrationV1.new }
   let(:subject) { described_class.new(['the-path'], migration) }
 
diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces_spec.rb
index aa63f6f9805..8125dedd3fc 100644
--- a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces_spec.rb
+++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameNamespaces do
+describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameNamespaces, :truncate do
   let(:migration) { FakeRenameReservedPathMigrationV1.new }
   let(:subject) { described_class.new(['the-path'], migration) }
 
diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects_spec.rb
index 9a6ed98898d..802f77ad430 100644
--- a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects_spec.rb
+++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameProjects do
+describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameProjects, :truncate do
   let(:migration) { FakeRenameReservedPathMigrationV1.new }
   let(:subject) { described_class.new(['the-path'], migration) }
 
diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1_spec.rb
index bdd3af4ad44..1d5e58855c1 100644
--- a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1_spec.rb
+++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1_spec.rb
@@ -13,7 +13,7 @@ shared_examples 'renames child namespaces' do |type|
   end
 end
 
-describe Gitlab::Database::RenameReservedPathsMigration::V1 do
+describe Gitlab::Database::RenameReservedPathsMigration::V1, :truncate do
   let(:subject) { FakeRenameReservedPathMigrationV1.new }
 
   before do
diff --git a/spec/migrations/add_head_pipeline_for_each_merge_request_spec.rb b/spec/migrations/add_head_pipeline_for_each_merge_request_spec.rb
index bd5f85b901d..65bea662b02 100644
--- a/spec/migrations/add_head_pipeline_for_each_merge_request_spec.rb
+++ b/spec/migrations/add_head_pipeline_for_each_merge_request_spec.rb
@@ -1,7 +1,7 @@
 require 'spec_helper'
 require Rails.root.join('db', 'post_migrate', '20170508170547_add_head_pipeline_for_each_merge_request.rb')
 
-describe AddHeadPipelineForEachMergeRequest do
+describe AddHeadPipelineForEachMergeRequest, :truncate do
   let(:migration) { described_class.new }
 
   let!(:project) { create(:empty_project) }
diff --git a/spec/migrations/migrate_user_activities_to_users_last_activity_on_spec.rb b/spec/migrations/migrate_user_activities_to_users_last_activity_on_spec.rb
index 1db9bc002ae..e3b42b5eac8 100644
--- a/spec/migrations/migrate_user_activities_to_users_last_activity_on_spec.rb
+++ b/spec/migrations/migrate_user_activities_to_users_last_activity_on_spec.rb
@@ -3,7 +3,7 @@
 require 'spec_helper'
 require Rails.root.join('db', 'post_migrate', '20170324160416_migrate_user_activities_to_users_last_activity_on.rb')
 
-describe MigrateUserActivitiesToUsersLastActivityOn, :redis do
+describe MigrateUserActivitiesToUsersLastActivityOn, :redis, :truncate do
   let(:migration) { described_class.new }
   let!(:user_active_1) { create(:user) }
   let!(:user_active_2) { create(:user) }
diff --git a/spec/migrations/migrate_user_project_view_spec.rb b/spec/migrations/migrate_user_project_view_spec.rb
index 70f8e0d6082..afaa5d836a7 100644
--- a/spec/migrations/migrate_user_project_view_spec.rb
+++ b/spec/migrations/migrate_user_project_view_spec.rb
@@ -3,7 +3,7 @@
 require 'spec_helper'
 require Rails.root.join('db', 'post_migrate', '20170406142253_migrate_user_project_view.rb')
 
-describe MigrateUserProjectView do
+describe MigrateUserProjectView, :truncate do
   let(:migration) { described_class.new }
   let!(:user) { create(:user) }