From 63d84e0b44b761e03cdbbe1615fa2cefcfb4327d Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 19 Nov 2019 10:05:00 -0800
Subject: [PATCH] Improve use of keys when iterating over commits in "bin/audit
 delete" and "bin/repository rebuild-identities"

Summary:
Fixes T13457. Ref T13444. When we iterate over commits in a particular repository, the default iteration strategy can't effectively use the keys on the table.

Tweak the ordering so the "<repositoryID, epoch, [id]>" key can be used.

Test Plan:
  - Ran `bin/audit delete --repository X` and `bin/repository rebuild-identities --repository X` before and after changes.
    - With just the key changes, performance was slightly better. My local data isn't large enough to really emphasize the key changes.
    - With the page size changes, performance was a bit better (~30%, but on 1-3 second run durations).
  - Used `--trace` and ran `EXPLAIN ...` on the new queries, saw them select the "<repositoryID, epoch, [id]>" key and report a bare "Using index condition" in the "Extra" column.

Maniphest Tasks: T13457, T13444

Differential Revision: https://secure.phabricator.com/D20921
---
 .../PhabricatorAuditManagementDeleteWorkflow.php     | 12 +++++++++++-
 ...RepositoryManagementRebuildIdentitiesWorkflow.php |  7 ++++++-
 .../storage/lisk/PhabricatorQueryIterator.php        |  7 ++++++-
 3 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php b/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php
index 7d14df7239..cd621ce821 100644
--- a/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php
+++ b/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php
@@ -93,6 +93,12 @@ final class PhabricatorAuditManagementDeleteWorkflow
 
     if ($repos) {
       $query->withRepositoryIDs(mpull($repos, 'getID'));
+
+      // See T13457. If we're iterating over commits in a single large
+      // repository, the lack of a "<repositoryID, [id]>" key can slow things
+      // down. Iterate in a specific order to use a key which is present
+      // on the table ("<repositoryID, epoch, [id]>").
+      $query->setOrderVector(array('-epoch', '-id'));
     }
 
     $auditor_map = array();
@@ -105,7 +111,11 @@ final class PhabricatorAuditManagementDeleteWorkflow
       $query->withPHIDs(mpull($commits, 'getPHID'));
     }
 
-    $commit_iterator = new PhabricatorQueryIterator($query);
+    $commit_iterator = id(new PhabricatorQueryIterator($query));
+
+    // See T13457. We may be examining many commits; each commit is small so
+    // we can safely increase the page size to improve performance a bit.
+    $commit_iterator->setPageSize(1000);
 
     $audits = array();
     foreach ($commit_iterator as $commit) {
diff --git a/src/applications/repository/management/PhabricatorRepositoryManagementRebuildIdentitiesWorkflow.php b/src/applications/repository/management/PhabricatorRepositoryManagementRebuildIdentitiesWorkflow.php
index c1f92e8371..513d7a3805 100644
--- a/src/applications/repository/management/PhabricatorRepositoryManagementRebuildIdentitiesWorkflow.php
+++ b/src/applications/repository/management/PhabricatorRepositoryManagementRebuildIdentitiesWorkflow.php
@@ -98,7 +98,12 @@ final class PhabricatorRepositoryManagementRebuildIdentitiesWorkflow
           ->needCommitData(true)
           ->withRepositoryIDs(array($repository->getID()));
 
-        $commit_iterator = new PhabricatorQueryIterator($commit_query);
+        // See T13457. Adjust ordering to hit keys better and tweak page size
+        // to improve performance slightly, since these records are small.
+        $commit_query->setOrderVector(array('-epoch', '-id'));
+
+        $commit_iterator = id(new PhabricatorQueryIterator($commit_query))
+          ->setPageSize(1000);
 
         $this->rebuildCommits($commit_iterator);
       }
diff --git a/src/infrastructure/storage/lisk/PhabricatorQueryIterator.php b/src/infrastructure/storage/lisk/PhabricatorQueryIterator.php
index 648b83863a..cc88678cdf 100644
--- a/src/infrastructure/storage/lisk/PhabricatorQueryIterator.php
+++ b/src/infrastructure/storage/lisk/PhabricatorQueryIterator.php
@@ -10,7 +10,12 @@ final class PhabricatorQueryIterator extends PhutilBufferedIterator {
   }
 
   protected function didRewind() {
-    $this->pager = new AphrontCursorPagerView();
+    $pager = new AphrontCursorPagerView();
+
+    $page_size = $this->getPageSize();
+    $pager->setPageSize($page_size);
+
+    $this->pager = $pager;
   }
 
   public function key() {