From 5db49f7664660cebed21a6fc25fb835a95ead375 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 14 Oct 2022 09:59:01 -0700 Subject: [PATCH] Nullify and delete some user associations in batches When a user has a lot of associated events, Todos, or other items, deletion of the user can timeout due to the foreign key cleanup taking longer than the allowed 15 seconds. To avoid this, we do what we did in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/100962 and https://gitlab.com/gitlab-org/gitlab/-/merge_requests/101016 and mark these associations as `dependent: :nullify` or `dependent: destroy` in order to ensure `Users::DestroyService` cleans them up in batches. Relates to https://gitlab.com/gitlab-org/gitlab/-/issues/358760 Changelog: performance --- app/models/user.rb | 9 ++- spec/services/users/destroy_service_spec.rb | 75 ++++++++++++++++++--- 2 files changed, 70 insertions(+), 14 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index b2f75959f15b0d..6c469c770e75c3 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -214,14 +214,15 @@ def update_tracked_fields!(request) has_many :spam_logs, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :builds, class_name: 'Ci::Build' has_many :pipelines, class_name: 'Ci::Pipeline' - has_many :todos + has_many :todos, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + has_many :authored_todos, class_name: 'Todo', dependent: :destroy, foreign_key: :author_id # rubocop:disable Cop/ActiveRecordDependent has_many :notification_settings has_many :award_emoji, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :triggers, class_name: 'Ci::Trigger', foreign_key: :owner_id has_many :issue_assignees, inverse_of: :assignee - has_many :merge_request_assignees, inverse_of: :assignee - has_many :merge_request_reviewers, inverse_of: :reviewer + has_many :merge_request_assignees, inverse_of: :assignee, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + has_many :merge_request_reviewers, inverse_of: :reviewer, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :assigned_issues, class_name: "Issue", through: :issue_assignees, source: :issue has_many :assigned_merge_requests, class_name: "MergeRequest", through: :merge_request_assignees, source: :merge_request has_many :created_custom_emoji, class_name: 'CustomEmoji', inverse_of: :creator @@ -255,6 +256,8 @@ def update_tracked_fields!(request) has_many :timelogs has_many :resource_label_events, dependent: :nullify # rubocop:disable Cop/ActiveRecordDependent + has_many :resource_state_events, dependent: :nullify # rubocop:disable Cop/ActiveRecordDependent + has_many :authored_events, class_name: 'Event', dependent: :destroy, foreign_key: :author_id # rubocop:disable Cop/ActiveRecordDependent # # Validations diff --git a/spec/services/users/destroy_service_spec.rb b/spec/services/users/destroy_service_spec.rb index efaac06c1f28e9..03e1811c8a5c4b 100644 --- a/spec/services/users/destroy_service_spec.rb +++ b/spec/services/users/destroy_service_spec.rb @@ -388,42 +388,95 @@ context 'batched nullify' do let(:other_user) { create(:user) } + # rubocop:disable Layout/LineLength + def nullify_in_batches_regexp(table, column, user, batch_size: 100) + %r{^UPDATE "#{table}" SET "#{column}" = NULL WHERE "#{table}"."id" IN \(SELECT "#{table}"."id" FROM "#{table}" WHERE "#{table}"."#{column}" = #{user.id} LIMIT #{batch_size}\)} + end + + def delete_in_batches_regexps(table, column, user, items, batch_size: 1000) + select_query = %r{^SELECT "#{table}".* FROM "#{table}" WHERE "#{table}"."#{column}" = #{user.id}.*ORDER BY "#{table}"."id" ASC LIMIT #{batch_size}} + + [select_query] + items.map { |item| %r{^DELETE FROM "#{table}" WHERE "#{table}"."id" = #{item.id}} } + end + # rubocop:enable Layout/LineLength + it 'nullifies related associations in batches' do expect(other_user).to receive(:nullify_dependent_associations_in_batches).and_call_original described_class.new(user).execute(other_user, skip_authorization: true) end - it 'nullifies last_updated_issues, closed_issues, resource_label_events' do + it 'nullifies issues and resource associations', :aggregate_failures do issue = create(:issue, closed_by: other_user, updated_by: other_user) resource_label_event = create(:resource_label_event, user: other_user) + resource_state_event = create(:resource_state_event, user: other_user) + todos = create_list(:todo, 2, project: issue.project, user: other_user, author: other_user, target: issue) + event = create(:event, project: issue.project, author: other_user) - described_class.new(user).execute(other_user, skip_authorization: true) + query_recorder = ActiveRecord::QueryRecorder.new do + described_class.new(user).execute(other_user, skip_authorization: true) + end issue.reload resource_label_event.reload + resource_state_event.reload expect(issue.closed_by).to be_nil expect(issue.updated_by).to be_nil expect(resource_label_event.user).to be_nil + expect(resource_state_event.user).to be_nil + expect(other_user.authored_todos).to be_empty + expect(other_user.todos).to be_empty + expect(other_user.authored_events).to be_empty + + expected_queries = [ + nullify_in_batches_regexp(:issues, :updated_by_id, other_user), + nullify_in_batches_regexp(:issues, :closed_by_id, other_user), + nullify_in_batches_regexp(:resource_label_events, :user_id, other_user), + nullify_in_batches_regexp(:resource_state_events, :user_id, other_user) + ] + + expected_queries += delete_in_batches_regexps(:todos, :user_id, other_user, todos) + expected_queries += delete_in_batches_regexps(:todos, :author_id, other_user, todos) + expected_queries += delete_in_batches_regexps(:events, :author_id, other_user, [event]) + + expect(query_recorder.log).to include(*expected_queries) end - it 'nullifies merge request associations' do + it 'nullifies merge request associations', :aggregate_failures do merge_request = create(:merge_request, source_project: project, target_project: project, assignee: other_user, updated_by: other_user, merge_user: other_user) merge_request.metrics.update!(merged_by: other_user, latest_closed_by: other_user) + merge_request.reviewers = [other_user] + merge_request.assignees = [other_user] - described_class.new(user).execute(other_user, skip_authorization: true) + query_recorder = ActiveRecord::QueryRecorder.new do + described_class.new(user).execute(other_user, skip_authorization: true) + end merge_request.reload - aggregate_failures do - expect(merge_request.updated_by).to be_nil - expect(merge_request.assignee).to be_nil - expect(merge_request.assignee_id).to be_nil - expect(merge_request.metrics.merged_by).to be_nil - expect(merge_request.metrics.latest_closed_by).to be_nil - end + expect(merge_request.updated_by).to be_nil + expect(merge_request.assignee).to be_nil + expect(merge_request.assignee_id).to be_nil + expect(merge_request.metrics.merged_by).to be_nil + expect(merge_request.metrics.latest_closed_by).to be_nil + expect(merge_request.reviewers).to be_empty + expect(merge_request.assignees).to be_empty + + expected_queries = [ + nullify_in_batches_regexp(:merge_requests, :updated_by_id, other_user), + nullify_in_batches_regexp(:merge_requests, :assignee_id, other_user), + nullify_in_batches_regexp(:merge_request_metrics, :merged_by_id, other_user), + nullify_in_batches_regexp(:merge_request_metrics, :latest_closed_by_id, other_user) + ] + + expected_queries += delete_in_batches_regexps(:merge_request_assignees, :user_id, other_user, + merge_request.assignees) + expected_queries += delete_in_batches_regexps(:merge_request_reviewers, :user_id, other_user, + merge_request.reviewers) + + expect(query_recorder.log).to include(*expected_queries) end end end -- GitLab