Bug 1588329 - Chunk bound params in bookmarks and history. r?mak! draft
authorLina Cambridge <lina@yakshaving.ninja>
Sat, 12 Oct 2019 23:21:50 +0000
changeset 2378200 88129ea10ff613651ea38d79d988985c24e0d784
parent 2378199 c7b03d292e49fffbeedf3ffdbd356a5de3837f8c
child 2378201 55835b18a8566e6d30d2dd6758a10aeec2c372e6
push id434587
push userreviewbot
push dateSat, 12 Oct 2019 23:22:06 +0000
treeherdertry@55835b18a856 [default view] [failures only]
reviewersmak
bugs1588329
milestone71.0a1
Bug 1588329 - Chunk bound params in bookmarks and history. r?mak! This commit builds on the last one to chunk all bound parameters. In most cases, we just move the statements into a loop that iterates over the chunks instead of the array. There are two exceptions which need a bit more work: `cleanupPages` and `remove`. In `cleanupPages`, we chunk `pagesToRemove`, and clean up `moz_updateoriginsdelete_temp` and orphan icons after the loop. This doesn't change functionality, because none of the statements that follow depend on `moz_origins`, `moz_icons`, or frecency stats. In `remove`, we now fetch page info for URLs and GUIDs in separate statements. This makes chunking URL params easier. Differential Diff: PHID-DIFF-dfcidgy5brxda6x537xg
toolkit/components/places/Bookmarks.jsm
toolkit/components/places/History.jsm
toolkit/components/places/tests/history/test_removeByFilter.js
--- a/toolkit/components/places/Bookmarks.jsm
+++ b/toolkit/components/places/Bookmarks.jsm
@@ -3013,23 +3013,25 @@ var updateFrecency = async function(db, 
   let frecencyClause = "CALCULATE_FRECENCY(id)";
   if (!collapseNotifications) {
     frecencyClause =
       "NOTIFY_FRECENCY(" +
       frecencyClause +
       ", url, guid, hidden, last_visit_date)";
   }
   // We just use the hashes, since updating a few additional urls won't hurt.
-  await db.execute(
-    `UPDATE moz_places
-     SET hidden = (url_hash BETWEEN hash("place", "prefix_lo") AND hash("place", "prefix_hi")),
-         frecency = ${frecencyClause}
-     WHERE url_hash IN (${sqlBindPlaceholders(hrefs, "hash(", ")")})`,
-    hrefs
-  );
+  for (let chunk of PlacesUtils.chunkArray(hrefs, db.variableLimit)) {
+    await db.execute(
+      `UPDATE moz_places
+       SET hidden = (url_hash BETWEEN hash("place", "prefix_lo") AND hash("place", "prefix_hi")),
+           frecency = ${frecencyClause}
+       WHERE url_hash IN (${sqlBindPlaceholders(chunk, "hash(", ")")})`,
+      chunk
+    );
+  }
 
   // Trigger frecency updates for all affected origins.
   await db.executeCached(`DELETE FROM moz_updateoriginsupdate_temp`);
 
   if (collapseNotifications) {
     let observers = PlacesUtils.history.getObservers();
     notify(observers, "onManyFrecenciesChanged");
   }
--- a/toolkit/components/places/History.jsm
+++ b/toolkit/components/places/History.jsm
@@ -845,30 +845,32 @@ function sqlBindPlaceholders(values, pre
  * @param {OpenConnection} db an Sqlite connection
  * @param {Array} idList The `moz_places` identifiers to invalidate.
  * @returns {Promise} resolved when done
  */
 var invalidateFrecencies = async function(db, idList) {
   if (!idList.length) {
     return;
   }
-  await db.execute(
-    `UPDATE moz_places
-     SET frecency = NOTIFY_FRECENCY(
-       CALCULATE_FRECENCY(id), url, guid, hidden, last_visit_date
-     ) WHERE id in (${sqlBindPlaceholders(idList)})`,
-    idList
-  );
-  await db.execute(
-    `UPDATE moz_places
-     SET hidden = 0
-     WHERE id in (${sqlBindPlaceholders(idList)})
-     AND frecency <> 0`,
-    idList
-  );
+  for (let chunk of PlacesUtils.chunkArray(idList, db.variableLimit)) {
+    await db.execute(
+      `UPDATE moz_places
+       SET frecency = NOTIFY_FRECENCY(
+         CALCULATE_FRECENCY(id), url, guid, hidden, last_visit_date
+       ) WHERE id in (${sqlBindPlaceholders(chunk)})`,
+      chunk
+    );
+    await db.execute(
+      `UPDATE moz_places
+       SET hidden = 0
+       WHERE id in (${sqlBindPlaceholders(chunk)})
+       AND frecency <> 0`,
+      chunk
+    );
+  }
   // Trigger frecency updates for all affected origins.
   await db.execute(`DELETE FROM moz_updateoriginsupdate_temp`);
 };
 
 // Inner implementation of History.clear().
 var clear = async function(db) {
   await db.executeTransaction(async function() {
     // Remove all non-bookmarked places entries first, this will speed up the
@@ -945,49 +947,52 @@ var cleanupPages = async function(db, pa
     pages.filter(p => p.hasForeign || p.hasVisits).map(p => p.id)
   );
 
   let pagesToRemove = pages.filter(p => !p.hasForeign && !p.hasVisits);
   if (!pagesToRemove.length) {
     return;
   }
 
-  let idsToRemove = pagesToRemove.map(p => p.id);
   // Note, we are already in a transaction, since callers create it.
   // Check relations regardless, to avoid creating orphans in case of
   // async race conditions.
-  await db.execute(
-    `DELETE FROM moz_places
-     WHERE id IN ( ${sqlBindPlaceholders(idsToRemove)} )
-       AND foreign_count = 0 AND last_visit_date ISNULL`,
-    idsToRemove
-  );
+  for (let chunk of PlacesUtils.chunkArray(pagesToRemove, db.variableLimit)) {
+    let idsToRemove = chunk.map(p => p.id);
+    await db.execute(
+      `DELETE FROM moz_places
+       WHERE id IN ( ${sqlBindPlaceholders(idsToRemove)} )
+         AND foreign_count = 0 AND last_visit_date ISNULL`,
+      idsToRemove
+    );
+
+    // Expire orphans.
+    let hashesToRemove = chunk.map(p => p.hash);
+    await db.executeCached(
+      `DELETE FROM moz_pages_w_icons
+       WHERE page_url_hash IN (${sqlBindPlaceholders(hashesToRemove)})`,
+      hashesToRemove
+    );
+
+    await db.execute(
+      `DELETE FROM moz_annos
+       WHERE place_id IN ( ${sqlBindPlaceholders(idsToRemove)} )`,
+      idsToRemove
+    );
+    await db.execute(
+      `DELETE FROM moz_inputhistory
+       WHERE place_id IN ( ${sqlBindPlaceholders(idsToRemove)} )`,
+      idsToRemove
+    );
+  }
   // Hosts accumulated during the places delete are updated through a trigger
   // (see nsPlacesTriggers.h).
   await db.executeCached(`DELETE FROM moz_updateoriginsdelete_temp`);
 
-  // Expire orphans.
-  let hashesToRemove = pagesToRemove.map(p => p.hash);
-  await db.executeCached(
-    `DELETE FROM moz_pages_w_icons
-     WHERE page_url_hash IN (${sqlBindPlaceholders(hashesToRemove)})`,
-    hashesToRemove
-  );
   await removeOrphanIcons(db);
-
-  await db.execute(
-    `DELETE FROM moz_annos
-     WHERE place_id IN ( ${sqlBindPlaceholders(idsToRemove)} )`,
-    idsToRemove
-  );
-  await db.execute(
-    `DELETE FROM moz_inputhistory
-     WHERE place_id IN ( ${sqlBindPlaceholders(idsToRemove)} )`,
-    idsToRemove
-  );
 };
 
 /**
  * Remove icons whose origin is not in moz_origins, unless referenced.
  * @param db: (Sqlite connection)
  *      The database.
  */
 function removeOrphanIcons(db) {
@@ -1276,43 +1281,52 @@ var removeVisitsByFilter = async functio
     if (!visitsToRemove.length) {
       // Nothing to do
       return false;
     }
 
     let pages = [];
     await db.executeTransaction(async function() {
       // 2. Remove all offending visits.
-      await db.execute(
-        `DELETE FROM moz_historyvisits
-         WHERE id IN (${sqlBindPlaceholders(visitsToRemove)})`,
-        visitsToRemove
-      );
+      for (let chunk of PlacesUtils.chunkArray(
+        visitsToRemove,
+        db.variableLimit
+      )) {
+        await db.execute(
+          `DELETE FROM moz_historyvisits
+           WHERE id IN (${sqlBindPlaceholders(chunk)})`,
+          chunk
+        );
+      }
 
       // 3. Find out which pages have been orphaned
-      pagesToInspect = [...pagesToInspect];
-      await db.execute(
-        `SELECT id, url, url_hash, guid,
-          (foreign_count != 0) AS has_foreign,
-          (last_visit_date NOTNULL) as has_visits
-         FROM moz_places
-         WHERE id IN (${sqlBindPlaceholders(pagesToInspect)})`,
-        pagesToInspect,
-        row => {
-          let page = {
-            id: row.getResultByName("id"),
-            guid: row.getResultByName("guid"),
-            hasForeign: row.getResultByName("has_foreign"),
-            hasVisits: row.getResultByName("has_visits"),
-            url: new URL(row.getResultByName("url")),
-            hash: row.getResultByName("url_hash"),
-          };
-          pages.push(page);
-        }
-      );
+      for (let chunk of PlacesUtils.chunkArray(
+        [...pagesToInspect],
+        db.variableLimit
+      )) {
+        await db.execute(
+          `SELECT id, url, url_hash, guid,
+            (foreign_count != 0) AS has_foreign,
+            (last_visit_date NOTNULL) as has_visits
+           FROM moz_places
+           WHERE id IN (${sqlBindPlaceholders(chunk)})`,
+          chunk,
+          row => {
+            let page = {
+              id: row.getResultByName("id"),
+              guid: row.getResultByName("guid"),
+              hasForeign: row.getResultByName("has_foreign"),
+              hasVisits: row.getResultByName("has_visits"),
+              url: new URL(row.getResultByName("url")),
+              hash: row.getResultByName("url_hash"),
+            };
+            pages.push(page);
+          }
+        );
+      }
 
       // 4. Clean up and notify
       await cleanupPages(db, pages);
     });
 
     notifyCleanup(db, pages, transition);
     notifyOnResult(onResultData, onResult); // don't wait
   } finally {
@@ -1408,48 +1422,43 @@ var removeByFilter = async function(db, 
     // Nothing to do
     return false;
   }
 
   try {
     await db.executeTransaction(async function() {
       // 4. Actually remove visits
       let pageIds = pages.map(p => p.id);
-      await db.execute(
-        `DELETE FROM moz_historyvisits
-         WHERE place_id IN(${sqlBindPlaceholders(pageIds)})`,
-        pageIds
-      );
+      for (let chunk of PlacesUtils.chunkArray(pageIds, db.variableLimit)) {
+        await db.execute(
+          `DELETE FROM moz_historyvisits
+           WHERE place_id IN(${sqlBindPlaceholders(chunk)})`,
+          chunk
+        );
+      }
       // 5. Clean up and notify
       await cleanupPages(db, pages);
     });
 
     notifyCleanup(db, pages);
     notifyOnResult(onResultData, onResult);
   } finally {
     PlacesUtils.history.clearEmbedVisits();
   }
 
   return hasPagesToRemove;
 };
 
 // Inner implementation of History.remove.
 var remove = async function(db, { guids, urls }, onResult = null) {
   // 1. Find out what needs to be removed
-  let query = `SELECT id, url, url_hash, guid, foreign_count, title, frecency
-     FROM moz_places
-     WHERE guid IN (${sqlBindPlaceholders(guids)})
-        OR (url_hash IN (${sqlBindPlaceholders(urls, "hash(", ")")})
-            AND url IN (${sqlBindPlaceholders(urls)}))
-    `;
-  let params = guids.concat(urls).concat(urls);
   let onResultData = onResult ? [] : null;
   let pages = [];
   let hasPagesToRemove = false;
-  await db.execute(query, params, function(row) {
+  function onRow(row) {
     let hasForeign = row.getResultByName("foreign_count") != 0;
     if (!hasForeign) {
       hasPagesToRemove = true;
     }
     let id = row.getResultByName("id");
     let guid = row.getResultByName("guid");
     let url = row.getResultByName("url");
     let page = {
@@ -1464,32 +1473,57 @@ var remove = async function(db, { guids,
     if (onResult) {
       onResultData.push({
         guid,
         title: row.getResultByName("title"),
         frecency: row.getResultByName("frecency"),
         url: new URL(url),
       });
     }
-  });
+  }
+  for (let chunk of PlacesUtils.chunkArray(guids, db.variableLimit)) {
+    let query = `SELECT id, url, url_hash, guid, foreign_count, title, frecency
+       FROM moz_places
+       WHERE guid IN (${sqlBindPlaceholders(guids)})
+      `;
+    await db.execute(query, chunk, onRow);
+  }
+  for (let chunk of PlacesUtils.chunkArray(urls, db.variableLimit)) {
+    // Make an array of variables like `["?1", "?2", ...]`, up to the length of
+    // the chunk. This lets us bind each URL once, reusing the binding for the
+    // `url_hash IN (...)` and `url IN (...)` clauses. We add 1 because indexed
+    // parameters start at 1, not 0.
+    let variables = Array.from(
+      { length: chunk.length },
+      (_, i) => "?" + (i + 1)
+    );
+    let query = `SELECT id, url, url_hash, guid, foreign_count, title, frecency
+       FROM moz_places
+       WHERE url_hash IN (${variables.map(v => `hash(${v})`).join(",")}) AND
+             url IN (${variables.join(",")})
+      `;
+    await db.execute(query, chunk, onRow);
+  }
 
   try {
     if (!pages.length) {
       // Nothing to do
       return false;
     }
 
     await db.executeTransaction(async function() {
       // 2. Remove all visits to these pages.
       let pageIds = pages.map(p => p.id);
-      await db.execute(
-        `DELETE FROM moz_historyvisits
-         WHERE place_id IN (${sqlBindPlaceholders(pageIds)})`,
-        pageIds
-      );
+      for (let chunk of PlacesUtils.chunkArray(pageIds, db.variableLimit)) {
+        await db.execute(
+          `DELETE FROM moz_historyvisits
+           WHERE place_id IN (${sqlBindPlaceholders(chunk)})`,
+          chunk
+        );
+      }
 
       // 3. Clean up and notify
       await cleanupPages(db, pages);
     });
 
     notifyCleanup(db, pages);
     notifyOnResult(onResultData, onResult); // don't wait
   } finally {
--- a/toolkit/components/places/tests/history/test_removeByFilter.js
+++ b/toolkit/components/places/tests/history/test_removeByFilter.js
@@ -399,16 +399,50 @@ add_task(async function test_error_cases
     /TypeError: Expected well formed hostname string for/
   );
   Assert.throws(
     () => PlacesUtils.history.removeByFilter({ host: "" }),
     /TypeError: Expected a non-empty filter/
   );
 });
 
+add_task(async function test_chunking() {
+  await PlacesUtils.history.clear();
+  await PlacesUtils.bookmarks.eraseEverything();
+
+  info("Insert many visited pages");
+  let pages = [];
+  for (let i = 1; i <= 1500; i++) {
+    let visits = [
+      {
+        date: new Date(Date.now() - (86400 + i) * 1000),
+        transition: PlacesUtils.history.TRANSITIONS.TYPED,
+      },
+    ];
+    pages.push(
+      {
+        url: `http://example.com/${i}`,
+        title: `Page ${i}`,
+        visits,
+      },
+      {
+        url: `http://subdomain.example.com/${i}`,
+        title: `Subdomain ${i}`,
+        visits,
+      }
+    );
+  }
+  await PlacesUtils.history.insertMany(pages);
+
+  info("Remove all visited pages");
+  await PlacesUtils.history.removeByFilter({
+    host: ".example.com",
+  });
+});
+
 // Helper functions
 
 function getObserverPromise(bookmarkedUri) {
   if (!bookmarkedUri) {
     return { observer: null, promiseObserved: Promise.resolve() };
   }
   let observer;
   let promiseObserved = new Promise((resolve, reject) => {