From a8a4518099cceec5826bb9ea5d9e3198c5a10698 Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Thu, 28 Feb 2019 19:15:30 +0800 Subject: Clean up `noteable_id` for notes on commits This was incorrectly set by a bug in: https://gitlab.com/gitlab-org/gitlab-ce/issues/54924 Also adds a `batch_size` option to `update_column_in_batches` --- ...16_clean_up_noteable_id_for_notes_on_commits.rb | 33 +++++++++++++++++++++ lib/gitlab/database/migration_helpers.rb | 19 +++++++----- ...ean_up_noteable_id_for_notes_on_commits_spec.rb | 34 ++++++++++++++++++++++ 3 files changed, 78 insertions(+), 8 deletions(-) create mode 100644 db/post_migrate/20190313092516_clean_up_noteable_id_for_notes_on_commits.rb create mode 100644 spec/migrations/clean_up_noteable_id_for_notes_on_commits_spec.rb diff --git a/db/post_migrate/20190313092516_clean_up_noteable_id_for_notes_on_commits.rb b/db/post_migrate/20190313092516_clean_up_noteable_id_for_notes_on_commits.rb new file mode 100644 index 00000000000..fcd63f42b0e --- /dev/null +++ b/db/post_migrate/20190313092516_clean_up_noteable_id_for_notes_on_commits.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class CleanUpNoteableIdForNotesOnCommits < ActiveRecord::Migration[5.0] + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + TEMP_INDEX_NAME = 'index_notes_on_commit_with_null_noteable_id' + + disable_ddl_transaction! + + def up + remove_concurrent_index_by_name(:notes, TEMP_INDEX_NAME) + + add_concurrent_index(:notes, :id, where: "noteable_type = 'Commit' AND noteable_id IS NOT NULL", name: TEMP_INDEX_NAME) + + # rubocop:disable Migration/UpdateLargeTable + update_column_in_batches(:notes, :noteable_id, nil, batch_size: 300) do |table, query| + query.where( + table[:noteable_type].eq('Commit').and(table[:noteable_id].not_eq(nil)) + ) + end + + remove_concurrent_index_by_name(:notes, TEMP_INDEX_NAME) + end + + def down + end +end diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb index 3abd0600e9d..7f5eb1188fc 100644 --- a/lib/gitlab/database/migration_helpers.rb +++ b/lib/gitlab/database/migration_helpers.rb @@ -282,6 +282,7 @@ module Gitlab # Updates the value of a column in batches. # # This method updates the table in batches of 5% of the total row count. + # A `batch_size` option can also be passed to set this to a fixed number. # This method will continue updating rows until no rows remain. # # When given a block this method will yield two values to the block: @@ -320,7 +321,7 @@ module Gitlab # make things _more_ complex). # # rubocop: disable Metrics/AbcSize - def update_column_in_batches(table, column, value) + def update_column_in_batches(table, column, value, batch_size: nil) if transaction_open? raise 'update_column_in_batches can not be run inside a transaction, ' \ 'you can disable transactions by calling disable_ddl_transaction! ' \ @@ -336,14 +337,16 @@ module Gitlab return if total == 0 - # Update in batches of 5% until we run out of any rows to update. - batch_size = ((total / 100.0) * 5.0).ceil - max_size = 1000 + if batch_size.nil? + # Update in batches of 5% until we run out of any rows to update. + batch_size = ((total / 100.0) * 5.0).ceil + max_size = 1000 - # The upper limit is 1000 to ensure we don't lock too many rows. For - # example, for "merge_requests" even 1% of the table is around 35 000 - # rows for GitLab.com. - batch_size = max_size if batch_size > max_size + # The upper limit is 1000 to ensure we don't lock too many rows. For + # example, for "merge_requests" even 1% of the table is around 35 000 + # rows for GitLab.com. + batch_size = max_size if batch_size > max_size + end start_arel = table.project(table[:id]).order(table[:id].asc).take(1) start_arel = yield table, start_arel if block_given? diff --git a/spec/migrations/clean_up_noteable_id_for_notes_on_commits_spec.rb b/spec/migrations/clean_up_noteable_id_for_notes_on_commits_spec.rb new file mode 100644 index 00000000000..572b7dfd0c8 --- /dev/null +++ b/spec/migrations/clean_up_noteable_id_for_notes_on_commits_spec.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20190313092516_clean_up_noteable_id_for_notes_on_commits.rb') + +describe CleanUpNoteableIdForNotesOnCommits, :migration do + let(:notes) { table(:notes) } + + before do + notes.create!(noteable_type: 'Commit', commit_id: '3d0a182204cece4857f81c6462720e0ad1af39c9', noteable_id: 3, note: 'Test') + notes.create!(noteable_type: 'Commit', commit_id: '3d0a182204cece4857f81c6462720e0ad1af39c9', noteable_id: 3, note: 'Test') + notes.create!(noteable_type: 'Commit', commit_id: '3d0a182204cece4857f81c6462720e0ad1af39c9', noteable_id: 3, note: 'Test') + + notes.create!(noteable_type: 'Issue', noteable_id: 1, note: 'Test') + notes.create!(noteable_type: 'MergeRequest', noteable_id: 1, note: 'Test') + notes.create!(noteable_type: 'Snippet', noteable_id: 1, note: 'Test') + end + + it 'clears noteable_id for notes on commits' do + expect { migrate! }.to change { dirty_notes_on_commits.count }.from(3).to(0) + end + + it 'does not clear noteable_id for other notes' do + expect { migrate! }.not_to change { other_notes.count } + end + + def dirty_notes_on_commits + notes.where(noteable_type: 'Commit').where('noteable_id IS NOT NULL') + end + + def other_notes + notes.where("noteable_type != 'Commit' AND noteable_id IS NOT NULL") + end +end -- cgit v1.2.1