Speed up counting approvers when some are specified

When some approvers are specified, we still need to check the potential set of
approvers, including people with developer access and up. Doing that with an OR
took a long time (over 500ms) on GitLab.com, even when only one approver was
specified.

By doing a UNION instead, we get much faster results (less than 10ms). Also, as
we are using UNION and not UNION ALL, we don't need to exclude the approvers
specified from the query to get everyone with access.

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