Commit 3e5a944e authored by Nick Thomas's avatar Nick Thomas

Fix timeouts when destroying a project with many notes

Generally, we like to use foreign key associations to destroy dependent
records. Where this is possible, it's faster and uses fewer resources
than doing it in GitLab.

In 028db84c , the notes association was
converted from `dependent: :destroy` to using foreign keys. However,
when destroying a specific project with > 60K notes, we're experiencing
statement timeouts in the staging environment. Additionally, notes have
a deprecated `attachments` uploader that requires Ruby intervention to
avoid orphaning files on disk - there are at least some notes on
GitLab.com that still use this deprecated uploader.

Since 2017, we've introduced the BatchDestroyDependentAssociations
concern, which takes care of processing large numbers of dependent
records in batches. Re-introducing `dependent: :destroy` on notes will
automatically make use of this code.

Changelog: fixed
parent 7b2791be
......@@ -96,7 +96,9 @@ class Note < ApplicationRecord
validate :does_not_exceed_notes_limit?, on: :create, unless: [:system?, :importing?]
# @deprecated attachments are handler by the MarkdownUploader
# @deprecated attachments are handled by the Upload model.
#
# https://gitlab.com/gitlab-org/gitlab/-/issues/20830
mount_uploader :attachment, AttachmentUploader
# Scopes
......
......@@ -260,7 +260,15 @@ class Project < ApplicationRecord
has_many :events
has_many :milestones
has_many :iterations
has_many :notes
# Projects with a very large number of notes may time out destroying them
# through the foreign key. Additionally, the deprecated attachment uploader
# for notes requires us to use dependent: :destroy to avoid orphaning uploaded
# files.
#
# https://gitlab.com/gitlab-org/gitlab/-/issues/207222
has_many :notes, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
has_many :snippets, class_name: 'ProjectSnippet'
has_many :hooks, class_name: 'ProjectHook'
has_many :protected_branches
......
......@@ -1522,4 +1522,16 @@ RSpec.describe Note do
it { is_expected.to be_truthy }
end
end
describe '#attachment' do
it 'is cleaned up correctly when project is destroyed' do
note = create(:note_on_issue, :with_attachment)
attachment = note.attachment
note.project.destroy!
expect(attachment).not_to be_exist
end
end
end
......@@ -28,7 +28,7 @@ RSpec.describe Project, factory_default: :keep do
it { is_expected.to have_many(:project_members).dependent(:delete_all) }
it { is_expected.to have_many(:users).through(:project_members) }
it { is_expected.to have_many(:requesters).dependent(:delete_all) }
it { is_expected.to have_many(:notes) }
it { is_expected.to have_many(:notes).dependent(:destroy) }
it { is_expected.to have_many(:snippets).class_name('ProjectSnippet') }
it { is_expected.to have_many(:deploy_keys_projects) }
it { is_expected.to have_many(:deploy_keys) }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment