Merge branch 'speed-up-approvers-queries-again' into 'master'

Speed up counting approvers when some are specified

See merge request !2196

app/models/concerns/approvable.rb

@@ -36,23 +36,28 @@ module Approvable
     #
     def number_of_potential_approvers
       has_access = ['access_level > ?', Member::REPORTER]
+      users_with_access = { id: project.project_authorizations.where(has_access).select(:user_id) }
       all_approvers = all_approvers_including_groups
 
-      wheres = [
-        "id IN (#{project.project_authorizations.where(has_access).select(:user_id).to_sql})"
-      ]
+      users_relation = User.active.where.not(id: approvals.select(:user_id))
+      users_relation = users_relation.where.not(id: author.id) if author
 
+      # This is an optimisation for large instances. Instead of getting the
+      # count of all users who meet the conditions in a single query, which
+      # produces a slow query plan, we get the union of all users with access
+      # and all users in the approvers list, and count them.
       if all_approvers.any?
-        wheres << "id IN (#{all_approvers.map(&:id).join(', ')})"
-      end
-
-      users = User
-        .active
-        .where("(#{wheres.join(' OR ')}) AND id NOT IN (#{approvals.select(:user_id).to_sql})")
+        specific_approvers = { id: all_approvers.map(&:id) }
 
-      users = users.where.not(id: author.id) if author
+        union = Gitlab::SQL::Union.new([
+          users_relation.where(users_with_access).select(:id),
+          users_relation.where(specific_approvers).select(:id)
+        ])
 
-      users.count
+        User.from("(#{union.to_sql}) subquery").count
+      else
+        users_relation.where(users_with_access).count
+      end
     end
 
     # Users in the list of approvers who have not already approved this MR.

changelogs/unreleased-ee/speed-up-approvers-queries-again.yml

+---
+title: Speed up checking for approvers when approvers are specified on the MR
+merge_request:
+author:

spec/models/merge_request_spec.rb

@@ -536,10 +536,16 @@ describe MergeRequest, models: true do
 
     it "includes project members with developer access and up" do
       expect do
+        developer = create(:user)
+
         project.add_guest(create(:user))
         project.add_reporter(create(:user))
-        project.add_developer(create(:user))
+        project.add_developer(developer)
         project.add_master(create(:user))
+
+        # Add this user as both someone with access, and an explicit approver,
+        # to ensure they aren't double-counted.
+        create(:approver, user: developer, target: merge_request)
       end.to change { merge_request.reload.number_of_potential_approvers }.by(2)
     end