Merge #1363

1363: Make recent downloads fast r=ashleygwilliams

Every aspect of crates.io is extremely fast except one. Calculating
recent downloads is a moderately expensive operation that we are doing
too often. It's not "slow" per-se. The query takes 500ms to complete on
average, which is not unreasonable.

However, this query does put a good bit of load on the DB server. The
root cause of #1304 was an irresponsible bot hitting /crates (which
includes recent downloads) enough that it caused our DB server's CPU
load to reach 100%. We're on a very low tier database server right now,
so we could fix this by just upgrading to the next tier if we wanted to.
However, as the number of crates grows, this problem will just get
worse.

Recent downloads appear in 3 places. Summary, search, and crate details.
Summary and search are the two endpoints that crawlers want to hit, so
we can't have slow queries on those pages.

There are other solutions we could take here. We could just not include
recent downloads on those pages, and include them only if a flag is set.
We could also move recent downloads to another endpoint which gets hit
separately. Both of these require an annoying amount of code though, and
we need a crawler policy that says "don't hit these endpoints", which
requires work for us to monitor those endpoints, etc.

Ultimately we don't need this data to be real time (it already is
slightly delayed anyway). We can just store this particular piece of
data in a materialized view, which we refresh periodically. Right now
I'm refreshing it whenever update-downloads runs, but I think we can go
even further and update it daily if this becomes a scaling problem in
the future.

Even though repopulating the view currently only takes ~500ms (same
amount of time as the query), I've opted to do it concurrently to avoid
locking the table for long periods of time as we continue to scale.

Crate details is still using the `crate_downloads` table, so it's a bit
closer to real time. When we're selecting this for a single crate, the
query is already fast enough. We may want to change this in the future.
We could also consider getting rid of `crate_downloads` entirely, and
populate the view from `version_downloads`. This will cause more CPU
load when we refresh the view, but it probably doesn't matter.

Author: bors-voyager[bot]

migrations/2018-04-24-145128_create_recent_crate_downloads/down.sql

@@ -0,0 +1,3 @@
+DROP FUNCTION refresh_recent_crate_downloads();
+DROP INDEX recent_crate_downloads_crate_id;
+DROP MATERIALIZED VIEW recent_crate_downloads;

migrations/2018-04-24-145128_create_recent_crate_downloads/up.sql

@@ -0,0 +1,9 @@
+CREATE MATERIALIZED VIEW recent_crate_downloads (crate_id, downloads) AS
+  SELECT crate_id, SUM(downloads) FROM crate_downloads
+    WHERE date > date(CURRENT_TIMESTAMP - INTERVAL '90 days')
+    GROUP BY crate_id;
+CREATE UNIQUE INDEX recent_crate_downloads_crate_id ON recent_crate_downloads (crate_id);
+
+CREATE FUNCTION refresh_recent_crate_downloads() RETURNS VOID AS $$
+  REFRESH MATERIALIZED VIEW CONCURRENTLY recent_crate_downloads;
+$$ LANGUAGE SQL;

src/bin/update-downloads.rs

@@ -2,6 +2,7 @@
 
 extern crate cargo_registry;
 extern crate chrono;
+#[macro_use]
 extern crate diesel;
 
 use diesel::prelude::*;
@@ -29,6 +30,7 @@ fn main() {
 }
 
 fn update(conn: &PgConnection) -> QueryResult<()> {
+    use diesel::select;
     use version_downloads::dsl::*;
 
     let mut max = Some(0);
@@ -42,6 +44,9 @@ fn update(conn: &PgConnection) -> QueryResult<()> {
         collect(conn, &rows)?;
         max = rows.last().map(|d| d.id);
     }
+
+    no_arg_sql_function!(refresh_recent_crate_downloads, ());
+    select(refresh_recent_crate_downloads).execute(conn)?;
     Ok(())
 }
 

src/controllers/krate/metadata.rs

@@ -14,7 +14,6 @@ use models::krate::ALL_COLUMNS;
 
 /// Handles the `GET /summary` route.
 pub fn summary(req: &mut Request) -> CargoResult<Response> {
-    use diesel::sql_query;
     use schema::crates::dsl::*;
 
     let conn = req.db_conn()?;
@@ -54,23 +53,12 @@ pub fn summary(req: &mut Request) -> CargoResult<Response> {
         .limit(10)
         .load(&*conn)?;
 
-    // This query needs to be structured in this way to have the LIMIT
-    // happen before the joining/sorting for performance reasons.
-    // It needs to use sql_query because Diesel doesn't have a great way
-    // to join on subselects right now :(
-    let most_recently_downloaded = sql_query(
-        "SELECT crates.* \
-         FROM crates \
-         JOIN ( \
-         SELECT crate_downloads.crate_id, SUM(crate_downloads.downloads) \
-         FROM crate_downloads \
-         WHERE crate_downloads.date > date(CURRENT_TIMESTAMP - INTERVAL '90 days') \
-         GROUP BY crate_downloads.crate_id \
-         ORDER BY SUM(crate_downloads.downloads) DESC NULLS LAST \
-         LIMIT 10 \
-         ) cd ON crates.id = cd.crate_id \
-         ORDER BY cd.sum DESC NULLS LAST",
-    ).load::<Crate>(&*conn)?;
+    let most_recently_downloaded = crates
+        .inner_join(recent_crate_downloads::table)
+        .order(recent_crate_downloads::downloads.desc())
+        .select(ALL_COLUMNS)
+        .limit(10)
+        .load(&*conn)?;
 
     let popular_keywords = keywords::table
         .order(keywords::crates_cnt.desc())

src/controllers/krate/search.rs

@@ -32,8 +32,7 @@ use models::krate::{canon_crate_name, ALL_COLUMNS};
 /// function out to cover the different use cases, and create unit tests
 /// for them.
 pub fn search(req: &mut Request) -> CargoResult<Response> {
-    use diesel::dsl::*;
-    use diesel::sql_types::{BigInt, Bool, Nullable};
+    use diesel::sql_types::Bool;
 
     let conn = req.db_conn()?;
     let (offset, limit) = req.pagination(10, 100)?;
@@ -43,30 +42,15 @@ pub fn search(req: &mut Request) -> CargoResult<Response> {
         .map(|s| &**s)
         .unwrap_or("recent-downloads");
 
-    let recent_downloads = sql::<Nullable<BigInt>>("SUM(crate_downloads.downloads)");
-
     let mut query = crates::table
-        .left_join(
-            crate_downloads::table.on(crates::id
-                .eq(crate_downloads::crate_id)
-                .and(crate_downloads::date.gt(date(now - 90.days())))),
-        )
-        .group_by(crates::id)
+        .left_join(recent_crate_downloads::table)
         .select((
             ALL_COLUMNS,
             false.into_sql::<Bool>(),
-            recent_downloads.clone(),
+            recent_crate_downloads::downloads.nullable(),
         ))
         .into_boxed();
 
-    if sort == "downloads" {
-        query = query.order(crates::downloads.desc())
-    } else if sort == "recent-downloads" {
-        query = query.order(recent_downloads.clone().desc().nulls_last())
-    } else {
-        query = query.order(crates::name.asc())
-    }
-
     if let Some(q_string) = params.get("q") {
         let sort = params.get("sort").map(|s| &**s).unwrap_or("relevance");
         let q = plainto_tsquery(q_string);
@@ -78,16 +62,13 @@ pub fn search(req: &mut Request) -> CargoResult<Response> {
         query = query.select((
             ALL_COLUMNS,
             Crate::with_name(q_string),
-            recent_downloads.clone(),
+            recent_crate_downloads::downloads.nullable(),
         ));
-        let perfect_match = Crate::with_name(q_string).desc();
-        if sort == "downloads" {
-            query = query.order((perfect_match, crates::downloads.desc()));
-        } else if sort == "recent-downloads" {
-            query = query.order((perfect_match, recent_downloads.clone().desc().nulls_last()));
-        } else {
+        query = query.order(Crate::with_name(q_string).desc());
+
+        if sort == "relevance" {
             let rank = ts_rank_cd(crates::textsearchable_index_col, q);
-            query = query.order((perfect_match, rank.desc()))
+            query = query.then_order_by(rank.desc())
         }
     }
 
@@ -156,6 +137,14 @@ pub fn search(req: &mut Request) -> CargoResult<Response> {
         );
     }
 
+    if sort == "downloads" {
+        query = query.then_order_by(crates::downloads.desc())
+    } else if sort == "recent-downloads" {
+        query = query.then_order_by(recent_crate_downloads::downloads.desc().nulls_last())
+    } else {
+        query = query.then_order_by(crates::name.asc())
+    }
+
     // The database query returns a tuple within a tuple , with the root
     // tuple containing 3 items.
     let data = query

src/schema.rs

@@ -591,6 +591,24 @@ table! {
     }
 }
 
+table! {
+    /// Representation of the `recent_crate_downloads` view.
+    ///
+    /// This data represents the downloads in the last 90 days.
+    /// This view does not contain realtime data.
+    /// It is refreshed by the `update-downloads` script.
+    recent_crate_downloads (crate_id) {
+        /// The `crate_id` column of the `recent_crate_downloads` view.
+        ///
+        /// Its SQL type is `Integer`.
+        crate_id -> Integer,
+        /// The `downloads` column of the `recent_crate_downloads` table.
+        ///
+        /// Its SQL type is `BigInt`.
+        downloads -> BigInt,
+    }
+}
+
 table! {
     use diesel::sql_types::*;
     use diesel_full_text_search::{TsVector as Tsvector};
@@ -865,6 +883,7 @@ joinable!(emails -> users (user_id));
 joinable!(follows -> crates (crate_id));
 joinable!(follows -> users (user_id));
 joinable!(readme_renderings -> versions (version_id));
+joinable!(recent_crate_downloads -> crates (crate_id));
 joinable!(version_authors -> users (user_id));
 joinable!(version_authors -> versions (version_id));
 joinable!(version_downloads -> versions (version_id));
@@ -886,6 +905,7 @@ allow_tables_to_appear_in_same_query!(
     keywords,
     metadata,
     readme_renderings,
+    recent_crate_downloads,
     reserved_crate_names,
     teams,
     users,

src/tests/all.rs

@@ -408,7 +408,7 @@ impl<'a> CrateBuilder<'a> {
     }
 
     fn build(mut self, connection: &PgConnection) -> CargoResult<Crate> {
-        use diesel::{insert_into, update};
+        use diesel::{insert_into, select, update};
 
         let mut krate = self.krate
             .create_or_update(connection, None, self.owner_id)?;
@@ -444,6 +444,9 @@ impl<'a> CrateBuilder<'a> {
             insert_into(crate_downloads::table)
                 .values(&crate_download)
                 .execute(connection)?;
+
+            no_arg_sql_function!(refresh_recent_crate_downloads, ());
+            select(refresh_recent_crate_downloads).execute(connection)?;
         }
 
         if self.versions.is_empty() {