From 5c8ce94052fab07c979f6b1ecef9f7b807909b94 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sun, 4 Nov 2018 05:59:13 -0800 Subject: [PATCH 1/2] Fix statement timeouts in RemoveRestrictedTodos migration On GitLab.com, the RemoveRestrictedTodos background migration encountered about 700+ failures a day due to statement timeouts. PostgreSQL might perform badly with a LIMIT 1 because the planner is guessing that scanning the index in ID order will come across the desired row in less time it will take the planner than using another index. The order_hint does not affect the search results. For example, `ORDER BY id ASC, updated_at ASC` means the same thing as `ORDER BY id ASC`. Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/52649 --- app/models/concerns/each_batch.rb | 17 ++++- changelogs/unreleased/sh-fix-issue-52649.yml | 5 ++ .../remove_restricted_todos.rb | 2 +- spec/models/concerns/each_batch_spec.rb | 69 ++++++++++--------- 4 files changed, 58 insertions(+), 35 deletions(-) create mode 100644 changelogs/unreleased/sh-fix-issue-52649.yml diff --git a/app/models/concerns/each_batch.rb b/app/models/concerns/each_batch.rb index 8cf0b8b154d..6314b46a7e3 100644 --- a/app/models/concerns/each_batch.rb +++ b/app/models/concerns/each_batch.rb @@ -39,7 +39,15 @@ module EachBatch # # of - The number of rows to retrieve per batch. # column - The column to use for ordering the batches. - def each_batch(of: 1000, column: primary_key) + # order_hint - An optional column to append to the `ORDER BY id` + # clause to help the query planner. PostgreSQL might perform badly + # with a LIMIT 1 because the planner is guessing that scanning the + # index in ID order will come across the desired row in less time + # it will take the planner than using another index. The + # order_hint does not affect the search results. For example, + # `ORDER BY id ASC, updated_at ASC` means the same thing as `ORDER + # BY id ASC`. + def each_batch(of: 1000, column: primary_key, order_hint: nil) unless column raise ArgumentError, 'the column: argument must be set to a column name to use for ordering rows' @@ -48,7 +56,9 @@ module EachBatch start = except(:select) .select(column) .reorder(column => :asc) - .take + + start = start.order(order_hint) if order_hint + start = start.take return unless start @@ -60,6 +70,9 @@ module EachBatch .select(column) .where(arel_table[column].gteq(start_id)) .reorder(column => :asc) + + stop = stop.order(order_hint) if order_hint + stop = stop .offset(of) .limit(1) .take diff --git a/changelogs/unreleased/sh-fix-issue-52649.yml b/changelogs/unreleased/sh-fix-issue-52649.yml new file mode 100644 index 00000000000..34b7f74a345 --- /dev/null +++ b/changelogs/unreleased/sh-fix-issue-52649.yml @@ -0,0 +1,5 @@ +--- +title: Fix statement timeouts in RemoveRestrictedTodos migration +merge_request: 22795 +author: +type: other diff --git a/lib/gitlab/background_migration/remove_restricted_todos.rb b/lib/gitlab/background_migration/remove_restricted_todos.rb index 9941c2fe6d9..47579d46c1b 100644 --- a/lib/gitlab/background_migration/remove_restricted_todos.rb +++ b/lib/gitlab/background_migration/remove_restricted_todos.rb @@ -67,7 +67,7 @@ module Gitlab .where('access_level >= ?', 20) confidential_issues = Issue.select(:id, :author_id).where(confidential: true, project_id: project_id) - confidential_issues.each_batch(of: 100) do |batch| + confidential_issues.each_batch(of: 100, order_hint: :confidential) do |batch| batch.each do |issue| assigned_users = IssueAssignee.select(:user_id).where(issue_id: issue.id) diff --git a/spec/models/concerns/each_batch_spec.rb b/spec/models/concerns/each_batch_spec.rb index 951690a217b..17224c09693 100644 --- a/spec/models/concerns/each_batch_spec.rb +++ b/spec/models/concerns/each_batch_spec.rb @@ -14,40 +14,45 @@ describe EachBatch do 5.times { create(:user, updated_at: 1.day.ago) } end - it 'yields an ActiveRecord::Relation when a block is given' do - model.each_batch do |relation| - expect(relation).to be_a_kind_of(ActiveRecord::Relation) + shared_examples 'each_batch handling' do |kwargs| + it 'yields an ActiveRecord::Relation when a block is given' do + model.each_batch(kwargs) do |relation| + expect(relation).to be_a_kind_of(ActiveRecord::Relation) + end + end + + it 'yields a batch index as the second argument' do + model.each_batch(kwargs) do |_, index| + expect(index).to eq(1) + end + end + + it 'accepts a custom batch size' do + amount = 0 + + model.each_batch(kwargs.merge({ of: 1 })) { amount += 1 } + + expect(amount).to eq(5) + end + + it 'does not include ORDER BYs in the yielded relations' do + model.each_batch do |relation| + expect(relation.to_sql).not_to include('ORDER BY') + end + end + + it 'allows updating of the yielded relations' do + time = Time.now + + model.each_batch do |relation| + relation.update_all(updated_at: time) + end + + expect(model.where(updated_at: time).count).to eq(5) end end - it 'yields a batch index as the second argument' do - model.each_batch do |_, index| - expect(index).to eq(1) - end - end - - it 'accepts a custom batch size' do - amount = 0 - - model.each_batch(of: 1) { amount += 1 } - - expect(amount).to eq(5) - end - - it 'does not include ORDER BYs in the yielded relations' do - model.each_batch do |relation| - expect(relation.to_sql).not_to include('ORDER BY') - end - end - - it 'allows updating of the yielded relations' do - time = Time.now - - model.each_batch do |relation| - relation.update_all(updated_at: time) - end - - expect(model.where(updated_at: time).count).to eq(5) - end + it_behaves_like 'each_batch handling', {} + it_behaves_like 'each_batch handling', { order_hint: :updated_at } end end From bb55bcddb80adf017d82758587761f413674d985 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Tue, 6 Nov 2018 21:46:23 -0800 Subject: [PATCH 2/2] Reschedule another instance of RemoveRestrictedTodos migration --- ...107054254_remove_restricted_todos_again.rb | 32 +++++++++++++++++++ db/schema.rb | 2 +- 2 files changed, 33 insertions(+), 1 deletion(-) create mode 100644 db/post_migrate/20181107054254_remove_restricted_todos_again.rb diff --git a/db/post_migrate/20181107054254_remove_restricted_todos_again.rb b/db/post_migrate/20181107054254_remove_restricted_todos_again.rb new file mode 100644 index 00000000000..644e0074c46 --- /dev/null +++ b/db/post_migrate/20181107054254_remove_restricted_todos_again.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +# rescheduling of the revised RemoveRestrictedTodosWithCte background migration +class RemoveRestrictedTodosAgain < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + disable_ddl_transaction! + + MIGRATION = 'RemoveRestrictedTodos'.freeze + BATCH_SIZE = 1000 + DELAY_INTERVAL = 5.minutes.to_i + + class Project < ActiveRecord::Base + include EachBatch + + self.table_name = 'projects' + end + + def up + Project.where('EXISTS (SELECT 1 FROM todos WHERE todos.project_id = projects.id)') + .each_batch(of: BATCH_SIZE) do |batch, index| + range = batch.pluck('MIN(id)', 'MAX(id)').first + + BackgroundMigrationWorker.perform_in(index * DELAY_INTERVAL, MIGRATION, range) + end + end + + def down + # nothing to do + end +end diff --git a/db/schema.rb b/db/schema.rb index 1a8b556228d..0a97a76df48 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20181101144347) do +ActiveRecord::Schema.define(version: 20181107054254) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql"