From 157f47cd149c1a811c12850bbf76cc05dd805ad9 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 20 Oct 2017 15:37:19 -0700
Subject: [PATCH] Rewrite CommitQuery to use UNION for performance

Summary:
Ref T12680. See PHI167. See that task for discussion.

Rewrite `DiffusionCommitQuery` to work more like `DifferentialRevisionQuery`, and use a UNION to find "all revisions you need to audit OR respond to".

I tried to get this working a little more cleanly than RevisionQuery does, and can probably simplify that now.

Test Plan: Poked at the UI locally without hitting any apparent issues, but my local data is pretty garbage at this point. I'll take a look at how the query plans work on `secure`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12680

Differential Revision: https://secure.phabricator.com/D18722
---
 .../diffusion/query/DiffusionCommitQuery.php  | 83 +++++++++++++------
 ...PhabricatorCursorPagedPolicyAwareQuery.php | 35 ++++++--
 2 files changed, 85 insertions(+), 33 deletions(-)

diff --git a/src/applications/diffusion/query/DiffusionCommitQuery.php b/src/applications/diffusion/query/DiffusionCommitQuery.php
index da8937910e..8996bab628 100644
--- a/src/applications/diffusion/query/DiffusionCommitQuery.php
+++ b/src/applications/diffusion/query/DiffusionCommitQuery.php
@@ -177,7 +177,63 @@ final class DiffusionCommitQuery
   }
 
   protected function loadPage() {
-    return $this->loadStandardPage($this->newResultObject());
+    $table = $this->newResultObject();
+    $conn = $table->establishConnection('r');
+
+    $subqueries = array();
+    if ($this->responsiblePHIDs) {
+      $base_authors = $this->authorPHIDs;
+      $base_auditors = $this->auditorPHIDs;
+
+      $responsible_phids = $this->responsiblePHIDs;
+      if ($base_authors) {
+        $all_authors = array_merge($base_authors, $responsible_phids);
+      } else {
+        $all_authors = $responsible_phids;
+      }
+
+      if ($base_auditors) {
+        $all_auditors = array_merge($base_auditors, $responsible_phids);
+      } else {
+        $all_auditors = $responsible_phids;
+      }
+
+      $this->authorPHIDs = $all_authors;
+      $this->auditorPHIDs = $base_auditors;
+      $subqueries[] = $this->buildStandardPageQuery(
+        $conn,
+        $table->getTableName());
+
+      $this->authorPHIDs = $base_authors;
+      $this->auditorPHIDs = $all_auditors;
+      $subqueries[] = $this->buildStandardPageQuery(
+        $conn,
+        $table->getTableName());
+    } else {
+      $subqueries[] = $this->buildStandardPageQuery(
+        $conn,
+        $table->getTableName());
+    }
+
+    if (count($subqueries) > 1) {
+      foreach ($subqueries as $key => $subquery) {
+        $subqueries[$key] = '('.$subquery.')';
+      }
+
+      $query = qsprintf(
+        $conn,
+        '%Q %Q %Q',
+        implode(' UNION DISTINCT ', $subqueries),
+        $this->buildOrderClause($conn, true),
+        $this->buildLimitClause($conn));
+    } else {
+      $query = head($subqueries);
+    }
+
+    $rows = queryfx_all($conn, '%Q', $query);
+    $rows = $this->didLoadRawRows($rows);
+
+    return $table->loadAllFromArray($rows);
   }
 
   protected function willFilterPage(array $commits) {
@@ -487,18 +543,10 @@ final class DiffusionCommitQuery
         $this->auditorPHIDs);
     }
 
-    if ($this->responsiblePHIDs !== null) {
-      $where[] = qsprintf(
-        $conn,
-        '(audit.auditorPHID IN (%Ls) OR commit.authorPHID IN (%Ls))',
-        $this->responsiblePHIDs,
-        $this->responsiblePHIDs);
-    }
-
     if ($this->statuses !== null) {
       $where[] = qsprintf(
         $conn,
-        'commit.auditStatus IN (%Ls)',
+        'commit.auditStatus IN (%Ld)',
         $this->statuses);
     }
 
@@ -541,10 +589,6 @@ final class DiffusionCommitQuery
     return ($this->auditIDs || $this->auditorPHIDs);
   }
 
-  private function shouldJoinAudit() {
-    return (bool)$this->responsiblePHIDs;
-  }
-
   private function shouldJoinOwners() {
     return (bool)$this->packagePHIDs;
   }
@@ -560,13 +604,6 @@ final class DiffusionCommitQuery
         $audit_request->getTableName());
     }
 
-    if ($this->shouldJoinAudit()) {
-      $join[] = qsprintf(
-        $conn,
-        'LEFT JOIN %T audit ON commit.phid = audit.commitPHID',
-        $audit_request->getTableName());
-    }
-
     if ($this->shouldJoinOwners()) {
       $join[] = qsprintf(
         $conn,
@@ -584,10 +621,6 @@ final class DiffusionCommitQuery
       return true;
     }
 
-    if ($this->shouldJoinAudit()) {
-      return true;
-    }
-
     if ($this->shouldJoinOwners()) {
       return true;
     }
diff --git a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php
index 9bebfe6957..19d6ba07f4 100644
--- a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php
+++ b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php
@@ -111,7 +111,19 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
     AphrontDatabaseConnection $conn,
     $table_name) {
 
-    $rows = queryfx_all(
+    $query = $this->buildStandardPageQuery($conn, $table_name);
+
+    $rows = queryfx_all($conn, '%Q', $query);
+    $rows = $this->didLoadRawRows($rows);
+
+    return $rows;
+  }
+
+  protected function buildStandardPageQuery(
+    AphrontDatabaseConnection $conn,
+    $table_name) {
+
+    return qsprintf(
       $conn,
       '%Q FROM %T %Q %Q %Q %Q %Q %Q %Q',
       $this->buildSelectClause($conn),
@@ -123,10 +135,6 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
       $this->buildHavingClause($conn),
       $this->buildOrderClause($conn),
       $this->buildLimitClause($conn));
-
-    $rows = $this->didLoadRawRows($rows);
-
-    return $rows;
   }
 
   protected function didLoadRawRows(array $rows) {
@@ -1032,7 +1040,10 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
   /**
    * @task order
    */
-  final protected function buildOrderClause(AphrontDatabaseConnection $conn) {
+  final protected function buildOrderClause(
+    AphrontDatabaseConnection $conn,
+    $for_union = false) {
+
     $orderable = $this->getOrderableColumns();
     $vector = $this->getOrderVector();
 
@@ -1045,7 +1056,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
       $parts[] = $part;
     }
 
-    return $this->formatOrderClause($conn, $parts);
+    return $this->formatOrderClause($conn, $parts, $for_union);
   }
 
 
@@ -1054,7 +1065,8 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
    */
   protected function formatOrderClause(
     AphrontDatabaseConnection $conn,
-    array $parts) {
+    array $parts,
+    $for_union = false) {
 
     $is_query_reversed = false;
     if ($this->getBeforeID()) {
@@ -1075,6 +1087,13 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
       }
 
       $table = idx($part, 'table');
+
+      // When we're building an ORDER BY clause for a sequence of UNION
+      // statements, we can't refer to tables from the subqueries.
+      if ($for_union) {
+        $table = null;
+      }
+
       $column = $part['column'];
 
       if ($table !== null) {