Auto merge of #1694 - sgrif:sg-remove-pk-from-version-downloads, r=jtgeibel

Remove the `id` column from `version_downloads`

:warning: This PR contains destructive migrations. Each commit must be deployed separately :warning:

The `version_downloads` table and its indexes account for a whopping 69% of our total database size. The primary key alone is nearly 1GB, or 25% of our total database. This is never really used for anything besides fetching in batches in `update-downloads`. We could just do batching based on crate_id and date, but I've opted to remove the batching entirely. We usually process somewhere between 2k and 3k rows every time it runs -- we'd have to process over 50k rows to even consume 1MB of memory, so we can just load them all at once.

With our code changed to never remove the column, we can just completely remove it. This brings the size of the table down to 1600MB total, or 1256MB after a full vacuum.

Since Diesel always runs migrations in a transaction, and we can't add an index concurrently inside a transaction, the `down.sql` included is only for development/tests. If we need to roll this migration back in production, the column will need to be added/populated and the indexes created manually in a way that doesn't lock the table. If we were to run the blocking version included in down.sql against production, the table would be locked for multiple minutes, which would take some pages down and make the download endpoint painfully slow.

I don't anticipate needing to roll this migration back.

Author: bors

migrations/2019-03-22-174825_remove_version_downloads_pk/down.sql

@@ -0,0 +1,28 @@
+-- Diesel always runs migrations in a transaction, and we can't create indexes
+-- concurrently in a transaction. We can't block crate downloads on this, so
+-- just raise if we try to run this in production
+DO $$
+DECLARE
+  tmp_index_exists boolean;
+BEGIN
+  tmp_index_exists := (SELECT EXISTS (
+      SELECT * FROM pg_index
+        INNER JOIN pg_class
+          ON pg_class.oid = pg_index.indexrelid
+        WHERE relname = 'version_downloads_tmp'
+  ));
+  IF NOT tmp_index_exists THEN
+    IF (SELECT COUNT(*) FROM version_downloads) > 1000 THEN
+      RAISE EXCEPTION 'Indexes need to be created concurrently in production, manually create them and try again';
+    ELSE
+      ALTER TABLE version_downloads ADD COLUMN id SERIAL;
+      CREATE UNIQUE INDEX version_downloads_tmp ON version_downloads (id);
+      CREATE UNIQUE INDEX version_downloads_unique ON version_downloads (version_id, date);
+      CREATE UNIQUE INDEX index_version_downloads_not_processed ON version_downloads (id) WHERE processed = false;
+    END IF;
+  END IF;
+END $$;
+
+ALTER TABLE version_downloads
+  DROP CONSTRAINT version_downloads_pkey,
+  ADD CONSTRAINT version_downloads_pkey PRIMARY KEY USING INDEX version_downloads_tmp;

migrations/2019-03-22-174825_remove_version_downloads_pk/up.sql

@@ -0,0 +1,6 @@
+-- ALERT! DESTRUCTIVE MIGRATION
+-- DO NOT DEPLOY UNTIL PREVIOUS COMMIT IS DEPLOYED
+ALTER TABLE version_downloads
+  DROP CONSTRAINT version_downloads_pkey,
+  ADD CONSTRAINT version_downloads_pkey PRIMARY KEY USING INDEX version_downloads_unique,
+  DROP COLUMN id;

mirage/fixtures/version-downloads.js

@@ -3,19 +3,16 @@ export default [
     {
         date: '2017-02-10T00:00:00Z',
         downloads: 2,
-        id: 3023900,
         version: 40905,
     },
     {
         date: '2017-02-10T00:00:00Z',
         downloads: 1,
-        id: 3024236,
         version: 18445,
     },
     {
         date: '2017-02-11T00:00:00Z',
         downloads: 1,
-        id: 3027018,
         version: 40905,
     },
 ];

src/bin/update-downloads.rs

@@ -12,8 +12,6 @@ use cargo_registry::{
 
 use diesel::prelude::*;
 
-static LIMIT: i64 = 1000;
-
 fn main() -> CargoResult<()> {
     let conn = db::connect_now()?;
     update(&conn)?;
@@ -25,18 +23,11 @@ fn update(conn: &PgConnection) -> QueryResult<()> {
     use diesel::dsl::now;
     use diesel::select;
 
-    let mut max = Some(0);
-    while let Some(m) = max {
-        let rows = version_downloads
-            .filter(processed.eq(false))
-            .filter(id.gt(m))
-            .filter(downloads.ne(counted))
-            .order(id)
-            .limit(LIMIT)
-            .load(conn)?;
-        collect(conn, &rows)?;
-        max = rows.last().map(|d| d.id);
-    }
+    let rows = version_downloads
+        .filter(processed.eq(false))
+        .filter(downloads.ne(counted))
+        .load(conn)?;
+    collect(conn, &rows)?;
 
     // Anything older than 24 hours ago will be frozen and will not be queried
     // against again.
@@ -62,7 +53,7 @@ fn collect(conn: &PgConnection, rows: &[VersionDownload]) -> QueryResult<()> {
 
         conn.transaction::<_, diesel::result::Error, _>(|| {
             // increment the number of counted downloads
-            update(version_downloads::table.find(download.id))
+            update(version_downloads::table.find(download.id()))
                 .set(version_downloads::counted.eq(version_downloads::counted + amt))
                 .execute(conn)?;
 

src/models/download.rs

@@ -7,8 +7,8 @@ use crate::views::EncodableVersionDownload;
 
 #[derive(Queryable, Identifiable, Associations, Debug, Clone, Copy)]
 #[belongs_to(Version)]
+#[primary_key(version_id, date)]
 pub struct VersionDownload {
-    pub id: i32,
     pub version_id: i32,
     pub downloads: i32,
     pub counted: i32,
@@ -34,7 +34,6 @@ impl VersionDownload {
 
     pub fn encodable(self) -> EncodableVersionDownload {
         EncodableVersionDownload {
-            id: self.id,
             version: self.version_id,
             downloads: self.downloads,
             date: self.date.to_string(),

src/schema.rs

@@ -786,13 +786,7 @@ table! {
     /// Representation of the `version_downloads` table.
     ///
     /// (Automatically generated by Diesel.)
-    version_downloads (id) {
-        /// The `id` column of the `version_downloads` table.
-        ///
-        /// Its SQL type is `Int4`.
-        ///
-        /// (Automatically generated by Diesel.)
-        id -> Int4,
+    version_downloads (version_id, date) {
         /// The `version_id` column of the `version_downloads` table.
         ///
         /// Its SQL type is `Int4`.

src/views.rs

@@ -66,7 +66,6 @@ pub struct EncodableDependency {
 
 #[derive(Serialize, Deserialize, Debug)]
 pub struct EncodableVersionDownload {
-    pub id: i32,
     pub version: i32,
     pub downloads: i32,
     pub date: String,