Commit ed693eec authored by Dylan Griffith's avatar Dylan Griffith

Remove N+1 queries from indexing issues

The `#as_indexed_json` method is used to convert individual documents to
the JSON that is sent to Elasticsearch during indexing. We need to
preload the data before it gets to this method. We implemented the
`.preload_indexing_data` method in
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/56808 so we just
need to add this to the various classes now. This MR updates it for
`Issue` only.

We also slightly refactor the issue_instance_proxy since it's more
efficient to just use the `user_id` from the `issue_assignees` join
table than to join again to `users` and also since it wasn't possible to
get the preload to work without loading all of `users.*` which would be
wasteful for a wide table and large numbers of records.
parent 193c4447
...@@ -447,6 +447,10 @@ class Issue < ApplicationRecord ...@@ -447,6 +447,10 @@ class Issue < ApplicationRecord
issue_email_participants.pluck(IssueEmailParticipant.arel_table[:email].lower) issue_email_participants.pluck(IssueEmailParticipant.arel_table[:email].lower)
end end
def issue_assignee_user_ids
issue_assignees.pluck(:user_id)
end
private private
# Ensure that the metrics association is safely created and respecting the unique constraint on issue_id # Ensure that the metrics association is safely created and respecting the unique constraint on issue_id
......
---
title: Remove N+1 DB queries from indexing issues in Elasticsearch
merge_request: 57788
author:
type: performance
...@@ -28,6 +28,12 @@ module Elastic ...@@ -28,6 +28,12 @@ module Elastic
search(query_hash, options) search(query_hash, options)
end end
# rubocop: disable CodeReuse/ActiveRecord
def preload_indexing_data(relation)
relation.includes(:issue_assignees, project: [:project_feature])
end
# rubocop: enable CodeReuse/ActiveRecord
private private
# Builds an elasticsearch query that will select documents from a # Builds an elasticsearch query that will select documents from a
......
...@@ -12,7 +12,11 @@ module Elastic ...@@ -12,7 +12,11 @@ module Elastic
data[attr.to_s] = safely_read_attribute_for_elasticsearch(attr) data[attr.to_s] = safely_read_attribute_for_elasticsearch(attr)
end end
data['assignee_id'] = safely_read_attribute_for_elasticsearch(:assignee_ids) # Load them through the issue_assignees table since calling
# assignee_ids can't be easily preloaded and does
# unnecessary joins
data['assignee_id'] = safely_read_attribute_for_elasticsearch(:issue_assignee_user_ids)
data['visibility_level'] = target.project.visibility_level data['visibility_level'] = target.project.visibility_level
data['issues_access_level'] = safely_read_project_feature_for_elasticsearch(:issues) data['issues_access_level'] = safely_read_project_feature_for_elasticsearch(:issues)
......
...@@ -221,7 +221,7 @@ RSpec.describe Elastic::ProcessBookkeepingService, :clean_gitlab_redis_shared_st ...@@ -221,7 +221,7 @@ RSpec.describe Elastic::ProcessBookkeepingService, :clean_gitlab_redis_shared_st
described_class.track!(*notes) described_class.track!(*notes)
control = ActiveRecord::QueryRecorder.new { described_class.new.execute } control = ActiveRecord::QueryRecorder.new(skip_cached: false) { described_class.new.execute }
3.times do 3.times do
notes << create(:note) notes << create(:note)
...@@ -234,6 +234,20 @@ RSpec.describe Elastic::ProcessBookkeepingService, :clean_gitlab_redis_shared_st ...@@ -234,6 +234,20 @@ RSpec.describe Elastic::ProcessBookkeepingService, :clean_gitlab_redis_shared_st
expect { described_class.new.execute }.not_to exceed_all_query_limit(control) expect { described_class.new.execute }.not_to exceed_all_query_limit(control)
end end
it 'does not have N+1 queries for issues' do
issues = create_list(:issue, 2)
described_class.track!(*issues)
control = ActiveRecord::QueryRecorder.new(skip_cached: false) { described_class.new.execute }
issues += create_list(:issue, 3)
described_class.track!(*issues)
expect { described_class.new.execute }.not_to exceed_all_query_limit(control)
end
end end
def expect_processing(*refs, failures: []) def expect_processing(*refs, failures: [])
......
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