From f934106bec76e5cd4d77b86ad116aeb180f3c516 Mon Sep 17 00:00:00 2001 From: Pavel Shutsin Date: Fri, 8 Apr 2022 12:34:09 +0200 Subject: [PATCH] Nullify dependent associations in batches on user deletion Nullify related associations in small portions to avoid query timouts for users with lots of activity and data in DB. Changelog: added --- .../batch_nullify_dependent_associations.rb | 27 ++++++++++ app/models/user.rb | 3 ++ app/services/users/destroy_service.rb | 4 ++ .../users/migrate_to_ghost_user_service.rb | 4 +- .../nullify_in_batches_on_user_deletion.yml | 8 +++ ...tch_nullify_dependent_associations_spec.rb | 49 +++++++++++++++++++ spec/services/users/destroy_service_spec.rb | 35 +++++++++++++ 7 files changed, 128 insertions(+), 2 deletions(-) create mode 100644 app/models/concerns/batch_nullify_dependent_associations.rb create mode 100644 config/feature_flags/development/nullify_in_batches_on_user_deletion.yml create mode 100644 spec/models/concerns/batch_nullify_dependent_associations_spec.rb diff --git a/app/models/concerns/batch_nullify_dependent_associations.rb b/app/models/concerns/batch_nullify_dependent_associations.rb new file mode 100644 index 00000000000000..c95b5b64a43cc0 --- /dev/null +++ b/app/models/concerns/batch_nullify_dependent_associations.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +# Provides a way to execute nullify behaviour in batches +# to avoid query timeouts for really big tables +# Assumes that associations have `dependent: :nullify` statement +module BatchNullifyDependentAssociations + extend ActiveSupport::Concern + + class_methods do + def dependent_associations_to_nullify + reflect_on_all_associations(:has_many).select { |assoc| assoc.options[:dependent] == :nullify } + end + end + + def nullify_dependent_associations_in_batches(exclude: [], batch_size: 100) + self.class.dependent_associations_to_nullify.each do |association| + next if association.name.in?(exclude) + + loop do + # rubocop:disable GitlabSecurity/PublicSend + update_count = public_send(association.name).limit(batch_size).update_all(association.foreign_key => nil) + # rubocop:enable GitlabSecurity/PublicSend + break if update_count < batch_size + end + end + end +end diff --git a/app/models/user.rb b/app/models/user.rb index f229ffef18c18b..4caa6ebb9c2194 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -21,6 +21,7 @@ class User < ApplicationRecord include OptionallySearch include FromUnion include BatchDestroyDependentAssociations + include BatchNullifyDependentAssociations include HasUniqueInternalUsers include IgnorableColumns include UpdateHighestRole @@ -189,6 +190,8 @@ def update_tracked_fields!(request) has_many :snippets, dependent: :destroy, foreign_key: :author_id # rubocop:disable Cop/ActiveRecordDependent has_many :notes, dependent: :destroy, foreign_key: :author_id # rubocop:disable Cop/ActiveRecordDependent has_many :issues, dependent: :destroy, foreign_key: :author_id # rubocop:disable Cop/ActiveRecordDependent + has_many :updated_issues, class_name: 'Issue', dependent: :nullify, foreign_key: :updated_by_id # rubocop:disable Cop/ActiveRecordDependent + has_many :closed_issues, class_name: 'Issue', dependent: :nullify, foreign_key: :closed_by_id # rubocop:disable Cop/ActiveRecordDependent has_many :merge_requests, dependent: :destroy, foreign_key: :author_id # rubocop:disable Cop/ActiveRecordDependent has_many :events, dependent: :delete_all, foreign_key: :author_id # rubocop:disable Cop/ActiveRecordDependent has_many :releases, dependent: :nullify, foreign_key: :author_id # rubocop:disable Cop/ActiveRecordDependent diff --git a/app/services/users/destroy_service.rb b/app/services/users/destroy_service.rb index 46eec08212587b..1ea65049dc22e8 100644 --- a/app/services/users/destroy_service.rb +++ b/app/services/users/destroy_service.rb @@ -64,6 +64,10 @@ def execute(user, options = {}) # This ensures we delete records in batches. user.destroy_dependent_associations_in_batches(exclude: [:snippets]) + if Feature.enabled?(:nullify_in_batches_on_user_deletion, default_enabled: :yaml) + user.nullify_dependent_associations_in_batches + end + # Destroy the namespace after destroying the user since certain methods may depend on the namespace existing user_data = user.destroy namespace.destroy diff --git a/app/services/users/migrate_to_ghost_user_service.rb b/app/services/users/migrate_to_ghost_user_service.rb index 604b83f621f833..3eb220c0e408d1 100644 --- a/app/services/users/migrate_to_ghost_user_service.rb +++ b/app/services/users/migrate_to_ghost_user_service.rb @@ -100,9 +100,9 @@ def migrate_reviews end # rubocop:disable CodeReuse/ActiveRecord - def batched_migrate(base_scope, column) + def batched_migrate(base_scope, column, batch_size: 50) loop do - update_count = base_scope.where(column => user.id).limit(100).update_all(column => ghost_user.id) + update_count = base_scope.where(column => user.id).limit(batch_size).update_all(column => ghost_user.id) break if update_count == 0 end end diff --git a/config/feature_flags/development/nullify_in_batches_on_user_deletion.yml b/config/feature_flags/development/nullify_in_batches_on_user_deletion.yml new file mode 100644 index 00000000000000..d97b4974f27b42 --- /dev/null +++ b/config/feature_flags/development/nullify_in_batches_on_user_deletion.yml @@ -0,0 +1,8 @@ +--- +name: 'nullify_in_batches_on_user_deletion' +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/84709 +rollout_issue_url: +milestone: '14.10' +type: development +group: group::optimize +default_enabled: true diff --git a/spec/models/concerns/batch_nullify_dependent_associations_spec.rb b/spec/models/concerns/batch_nullify_dependent_associations_spec.rb new file mode 100644 index 00000000000000..933464f301ad03 --- /dev/null +++ b/spec/models/concerns/batch_nullify_dependent_associations_spec.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe BatchNullifyDependentAssociations do + before do + test_user = Class.new(ActiveRecord::Base) do + self.table_name = 'users' + + has_many :closed_issues, foreign_key: :closed_by_id, class_name: 'Issue', dependent: :nullify + has_many :issues, foreign_key: :author_id, class_name: 'Issue', dependent: :nullify + has_many :updated_issues, foreign_key: :updated_by_id, class_name: 'Issue' + + include BatchNullifyDependentAssociations + end + + stub_const('TestUser', test_user) + end + + describe '.dependent_associations_to_nullify' do + it 'returns only associations with `dependent: :nullify` associations' do + expect(TestUser.dependent_associations_to_nullify.map(&:name)).to match_array([:closed_issues, :issues]) + end + end + + describe '#nullify_dependent_associations_in_batches' do + let_it_be(:user) { create(:user) } + let_it_be(:updated_by_issue) { create(:issue, updated_by: user) } + + before do + create(:issue, closed_by: user) + create(:issue, closed_by: user) + end + + it 'nullifies multiple settings' do + expect do + test_user = TestUser.find(user.id) + test_user.nullify_dependent_associations_in_batches + end.to change { Issue.where(closed_by_id: user.id).count }.by(-2) + end + + it 'excludes associations' do + expect do + test_user = TestUser.find(user.id) + test_user.nullify_dependent_associations_in_batches(exclude: [:closed_issues]) + end.not_to change { Issue.where(closed_by_id: user.id).count } + end + end +end diff --git a/spec/services/users/destroy_service_spec.rb b/spec/services/users/destroy_service_spec.rb index 602db66dba1708..80a506bb1d611f 100644 --- a/spec/services/users/destroy_service_spec.rb +++ b/spec/services/users/destroy_service_spec.rb @@ -332,4 +332,39 @@ expect(User.exists?(other_user.id)).to be(false) end end + + context 'batched nullify' do + let(:other_user) { create(:user) } + + context 'when :nullify_in_batches_on_user_deletion feature flag is enabled' do + 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 and closed_issues' do + issue = create(:issue, closed_by: other_user, updated_by: other_user) + + described_class.new(user).execute(other_user, skip_authorization: true) + + issue.reload + + expect(issue.closed_by).to be_nil + expect(issue.updated_by).to be_nil + end + end + + context 'when :nullify_in_batches_on_user_deletion feature flag is disabled' do + before do + stub_feature_flags(nullify_in_batches_on_user_deletion: false) + end + + it 'does not use batching' do + expect(other_user).not_to receive(:nullify_dependent_associations_in_batches) + + described_class.new(user).execute(other_user, skip_authorization: true) + end + end + end end -- GitLab