From b3ae48d8ca2dfe5d2fa28bdad9f86af0251a7cd9 Mon Sep 17 00:00:00 2001 From: epriestley <git@epriestley.com> Date: Wed, 17 Jun 2015 11:25:01 -0700 Subject: [PATCH] Improve Differential query plans Summary: Ref T8575. We run a big "(A) UNION (B)" query on the home page and on the main Differential page. "A" can always be improved by using `%Ls`, so it can use the second half of the `(authorPHID, status)` key. "B" can sometimes be improved if the fraction of open revisions is smaller than the fraction of revisions you are reviewing. This is true for me on secure.phabricator.com (I'm a reviewer, either directly or via 'Blessed Reviewers', on about 80% of revisions, but <5% are open). In these cases, a `(status, phid)` key is more efficient. Test Plan: Tweaked queries and added keys on this server, saw less derpy query plans and performance. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T8575 Differential Revision: https://secure.phabricator.com/D13325 --- .../query/DifferentialRevisionQuery.php | 16 ++++++++++------ .../storage/DifferentialRevision.php | 8 ++++++++ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/applications/differential/query/DifferentialRevisionQuery.php b/src/applications/differential/query/DifferentialRevisionQuery.php index 69e9477e0d..30731b6fed 100644 --- a/src/applications/differential/query/DifferentialRevisionQuery.php +++ b/src/applications/differential/query/DifferentialRevisionQuery.php @@ -771,19 +771,23 @@ final class DifferentialRevisionQuery $this->updatedEpochMax); } + // NOTE: Although the status constants are integers in PHP, the column is a + // string column in MySQL, and MySQL won't use keys on string columns if + // you put integers in the query. + switch ($this->status) { case self::STATUS_ANY: break; case self::STATUS_OPEN: $where[] = qsprintf( $conn_r, - 'r.status IN (%Ld)', + 'r.status IN (%Ls)', DifferentialRevisionStatus::getOpenStatuses()); break; case self::STATUS_NEEDS_REVIEW: $where[] = qsprintf( $conn_r, - 'r.status IN (%Ld)', + 'r.status IN (%Ls)', array( ArcanistDifferentialRevisionStatus::NEEDS_REVIEW, )); @@ -791,7 +795,7 @@ final class DifferentialRevisionQuery case self::STATUS_NEEDS_REVISION: $where[] = qsprintf( $conn_r, - 'r.status IN (%Ld)', + 'r.status IN (%Ls)', array( ArcanistDifferentialRevisionStatus::NEEDS_REVISION, )); @@ -799,7 +803,7 @@ final class DifferentialRevisionQuery case self::STATUS_ACCEPTED: $where[] = qsprintf( $conn_r, - 'r.status IN (%Ld)', + 'r.status IN (%Ls)', array( ArcanistDifferentialRevisionStatus::ACCEPTED, )); @@ -807,13 +811,13 @@ final class DifferentialRevisionQuery case self::STATUS_CLOSED: $where[] = qsprintf( $conn_r, - 'r.status IN (%Ld)', + 'r.status IN (%Ls)', DifferentialRevisionStatus::getClosedStatuses()); break; case self::STATUS_ABANDONED: $where[] = qsprintf( $conn_r, - 'r.status IN (%Ld)', + 'r.status IN (%Ls)', array( ArcanistDifferentialRevisionStatus::ABANDONED, )); diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index 5f2131475b..affa0c4d9a 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -102,6 +102,14 @@ final class DifferentialRevision extends DifferentialDAO 'repositoryPHID' => array( 'columns' => array('repositoryPHID'), ), + // If you (or a project you are a member of) is reviewing a significant + // fraction of the revisions on an install, the result set of open + // revisions may be smaller than the result set of revisions where you + // are a reviewer. In these cases, this key is better than keys on the + // edge table. + 'key_status' => array( + 'columns' => array('status', 'phid'), + ), ), ) + parent::getConfiguration(); }