Bug 1588329 - Use the new `variableLimit` getter to chunk binding params in Places. r?mak! draft
authorLina Cambridge <lina@yakshaving.ninja>
Sat, 12 Oct 2019 23:21:47 +0000
changeset 2378199 c7b03d292e49fffbeedf3ffdbd356a5de3837f8c
parent 2378198 e31679dc527279c29f6049ba4c0f24c098913b39
child 2378200 88129ea10ff613651ea38d79d988985c24e0d784
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 - Use the new `variableLimit` getter to chunk binding params in Places. r?mak! Differential Diff: PHID-DIFF-s2tyrqqez5iykjvkfjw2
toolkit/components/places/Bookmarks.jsm
toolkit/components/places/PlacesSyncUtils.jsm
toolkit/components/places/SyncedBookmarksMirror.jsm
toolkit/components/places/bookmark_sync/src/store.rs
toolkit/components/places/nsNavBookmarks.cpp
--- a/toolkit/components/places/Bookmarks.jsm
+++ b/toolkit/components/places/Bookmarks.jsm
@@ -98,17 +98,16 @@ async function promiseTagsFolderId() {
     { guid: Bookmarks.tagsGuid }
   );
   return (gTagsFolderId = rows[0].getResultByName("id"));
 }
 
 const MATCH_ANYWHERE_UNMODIFIED =
   Ci.mozIPlacesAutoComplete.MATCH_ANYWHERE_UNMODIFIED;
 const BEHAVIOR_BOOKMARK = Ci.mozIPlacesAutoComplete.BEHAVIOR_BOOKMARK;
-const SQLITE_MAX_VARIABLE_NUMBER = 999;
 
 var Bookmarks = Object.freeze({
   /**
    * Item's type constants.
    * These should stay consistent with nsINavBookmarksService.idl
    */
   TYPE_BOOKMARK: 1,
   TYPE_FOLDER: 2,
@@ -2212,26 +2211,20 @@ function insertBookmarkTree(items, sourc
          (SELECT id FROM moz_bookmarks WHERE guid = :parentGuid),
          IFNULL(:index, (SELECT count(*) FROM moz_bookmarks WHERE parent = :rootId)),
          NULLIF(:title, ''), :date_added, :last_modified, :guid,
          :syncChangeCounter, :syncStatus)`,
           items
         );
 
         // Remove stale tombstones for new items.
-        for (let chunk of PlacesUtils.chunkArray(
-          items,
-          SQLITE_MAX_VARIABLE_NUMBER
-        )) {
+        for (let chunk of PlacesUtils.chunkArray(items, db.variableLimit)) {
           await db.executeCached(
-            `DELETE FROM moz_bookmarks_deleted WHERE guid IN (${new Array(
-              chunk.length
-            )
-              .fill("?")
-              .join(",")})`,
+            `DELETE FROM moz_bookmarks_deleted
+             WHERE guid IN (${sqlBindPlaceholders(chunk)})`,
             chunk.map(item => item.guid)
           );
         }
 
         await setAncestorsLastModified(
           db,
           parent.guid,
           lastAddedForParent,
@@ -2623,30 +2616,26 @@ function removeBookmarks(items, options)
               throw new Error("Cannot remove a non-empty folder.");
             }
             urls = urls.concat(
               await removeFoldersContents(db, [item.guid], options)
             );
           }
         }
 
-        for (let chunk of PlacesUtils.chunkArray(
-          items,
-          SQLITE_MAX_VARIABLE_NUMBER
-        )) {
+        for (let chunk of PlacesUtils.chunkArray(items, db.variableLimit)) {
           // We don't go through the annotations service for this cause otherwise
           // we'd get a pointless onItemChanged notification and it would also
           // set lastModified to an unexpected value.
           await removeAnnotationsForItems(db, chunk);
 
           // Remove the bookmarks.
           await db.executeCached(
-            `DELETE FROM moz_bookmarks WHERE guid IN (${new Array(chunk.length)
-              .fill("?")
-              .join(",")})`,
+            `DELETE FROM moz_bookmarks
+             WHERE guid IN (${sqlBindPlaceholders(chunk)})`,
             chunk.map(item => item.guid)
           );
         }
 
         for (let [parentGuid, parentId] of parents.entries()) {
           // Now recalculate the positions.
           await db.executeCached(
             `WITH positions(id, pos, seq) AS (
--- a/toolkit/components/places/PlacesSyncUtils.jsm
+++ b/toolkit/components/places/PlacesSyncUtils.jsm
@@ -26,17 +26,16 @@ ChromeUtils.defineModuleGetter(
  * `nsINavBookmarksService`, with special handling for
  * tags, keywords, synced annotations, and missing parents.
  */
 var PlacesSyncUtils = {};
 
 const { SOURCE_SYNC } = Ci.nsINavBookmarksService;
 
 const MICROSECONDS_PER_SECOND = 1000000;
-const SQLITE_MAX_VARIABLE_NUMBER = 999;
 
 const MOBILE_BOOKMARKS_PREF = "browser.bookmarks.showMobileBookmarks";
 
 // These are defined as lazy getters to defer initializing the bookmarks
 // service until it's needed.
 XPCOMUtils.defineLazyGetter(this, "ROOT_RECORD_ID_TO_GUID", () => ({
   menu: PlacesUtils.bookmarks.menuGuid,
   places: PlacesUtils.bookmarks.rootGuid,
@@ -233,20 +232,17 @@ const HistorySyncUtils = (PlacesSyncUtil
    */
   async determineNonSyncableGuids(guids) {
     // Filter out hidden pages and `TRANSITION_FRAMED_LINK` visits. These are
     // excluded when rendering the history menu, so we use the same constraints
     // for Sync. We also don't want to sync `TRANSITION_EMBED` visits, but those
     // aren't stored in the database.
     let db = await PlacesUtils.promiseDBConnection();
     let nonSyncableGuids = [];
-    for (let chunk of PlacesUtils.chunkArray(
-      guids,
-      SQLITE_MAX_VARIABLE_NUMBER
-    )) {
+    for (let chunk of PlacesUtils.chunkArray(guids, db.variableLimit)) {
       let rows = await db.execute(
         `
         SELECT DISTINCT p.guid FROM moz_places p
         JOIN moz_historyvisits v ON p.id = v.place_id
         WHERE p.guid IN (${new Array(chunk.length).fill("?").join(",")}) AND
             (p.hidden = 1 OR v.visit_type IN (0,
               ${PlacesUtils.history.TRANSITION_FRAMED_LINK}))
       `,
--- a/toolkit/components/places/SyncedBookmarksMirror.jsm
+++ b/toolkit/components/places/SyncedBookmarksMirror.jsm
@@ -75,18 +75,16 @@ const SyncedBookmarksMerger = Components
   "mozISyncedBookmarksMerger"
 );
 
 // These can be removed once they're exposed in a central location (bug
 // 1375896).
 const DB_URL_LENGTH_MAX = 65536;
 const DB_TITLE_LENGTH_MAX = 4096;
 
-const SQLITE_MAX_VARIABLE_NUMBER = 999;
-
 // The current mirror database schema version. Bump for migrations, then add
 // migration code to `migrateMirrorSchema`.
 const MIRROR_SCHEMA_VERSION = 7;
 
 const DEFAULT_MAX_FRECENCIES_TO_RECALCULATE = 400;
 
 // Use a shared jankYielder in these functions
 XPCOMUtils.defineLazyGetter(this, "yieldState", () => Async.yieldState());
@@ -1023,17 +1021,17 @@ class SyncedBookmarksMirror {
       }
     );
 
     let children = record.children;
     if (children && Array.isArray(children)) {
       let offset = 0;
       for (let chunk of PlacesUtils.chunkArray(
         children,
-        SQLITE_MAX_VARIABLE_NUMBER - 1
+        this.db.variableLimit - 1
       )) {
         if (signal.aborted) {
           throw new SyncedBookmarksMirror.InterruptedError(
             "Interrupted while storing children for incoming folder"
           );
         }
         // Builds a fragment like `(?2, ?1, 0), (?3, ?1, 1), ...`, where ?1 is
         // the folder's GUID, [?2, ?3] are the first and second child GUIDs
--- a/toolkit/components/places/bookmark_sync/src/store.rs
+++ b/toolkit/components/places/bookmark_sync/src/store.rs
@@ -12,18 +12,16 @@ use nsstring::nsString;
 use storage::{Conn, Step};
 use xpcom::interfaces::{mozISyncedBookmarksMerger, nsINavBookmarksService};
 
 use crate::driver::{AbortController, Driver};
 use crate::error::{Error, Result};
 
 pub const LMANNO_FEEDURI: &'static str = "livemark/feedURI";
 
-const SQLITE_MAX_VARIABLE_NUMBER: usize = 999;
-
 extern "C" {
     fn NS_NavBookmarksTotalSyncChanges() -> i64;
 }
 
 fn total_sync_changes() -> i64 {
     unsafe { NS_NavBookmarksTotalSyncChanges() }
 }
 
@@ -482,17 +480,17 @@ fn update_local_items_in_places<'t>(
     )?;
 
     let now = rounded_now();
 
     // Insert URLs for new remote items into the `moz_places` table. We need to
     // do this before inserting new remote items, since we need Place IDs for
     // both old and new URLs.
     debug!(driver, "Inserting Places for new items");
-    for chunk in ops.apply_remote_items.chunks(SQLITE_MAX_VARIABLE_NUMBER) {
+    for chunk in ops.apply_remote_items.chunks(db.variable_limit()?) {
         let mut statement = db.prepare(format!(
             "INSERT OR IGNORE INTO moz_places(url, url_hash, rev_host, hidden,
                                               frecency, guid)
              SELECT u.url, u.hash, u.revHost, 0,
                     (CASE v.kind WHEN {} THEN 0 ELSE -1 END),
                     IFNULL((SELECT h.guid FROM moz_places h
                             WHERE h.url_hash = u.hash AND
                                   h.url = u.url), u.guid)
@@ -512,20 +510,17 @@ fn update_local_items_in_places<'t>(
 
     // Trigger frecency updates for all new origins.
     debug!(driver, "Updating origins for new URLs");
     controller.err_if_aborted()?;
     db.exec("DELETE FROM moz_updateoriginsinsert_temp")?;
 
     // Build a table of new and updated items.
     debug!(driver, "Staging apply remote item ops");
-    for chunk in ops
-        .apply_remote_items
-        .chunks(SQLITE_MAX_VARIABLE_NUMBER / 3)
-    {
+    for chunk in ops.apply_remote_items.chunks(db.variable_limit()? / 3) {
         // CTEs in `WITH` clauses aren't indexed, so this query needs a full
         // table scan on `ops`. But that's okay; a separate temp table for ops
         // would also need a full scan. Note that we need both the local _and_
         // remote GUIDs here, because we haven't changed the local GUIDs yet.
         let mut statement = db.prepare(format!(
             "WITH
              ops(mergedGuid, localGuid, remoteGuid, level) AS (VALUES {})
              INSERT INTO itemsToApply(mergedGuid, localId, remoteId,
@@ -589,17 +584,17 @@ fn update_local_items_in_places<'t>(
 
             let remote_guid = nsString::from(&*op.remote_node().guid);
             statement.bind_by_index(offset + 2, remote_guid)?;
         }
         statement.execute()?;
     }
 
     debug!(driver, "Staging change GUID ops");
-    for chunk in ops.change_guids.chunks(SQLITE_MAX_VARIABLE_NUMBER / 2) {
+    for chunk in ops.change_guids.chunks(db.variable_limit()? / 2) {
         let mut statement = db.prepare(format!(
             "INSERT INTO changeGuidOps(localGuid, mergedGuid, syncStatus, level,
                                        lastModifiedMicroseconds)
              VALUES {}",
             repeat_display(chunk.len(), ",", |index, f| {
                 let op = &chunk[index];
                 // If only the local GUID changed, the item was deduped, so we
                 // can mark it as syncing. Otherwise, we changed an invalid
@@ -631,17 +626,17 @@ fn update_local_items_in_places<'t>(
             statement.bind_by_index(offset + 1, merged_guid)?;
         }
         statement.execute()?;
     }
 
     debug!(driver, "Staging apply new local structure ops");
     for chunk in ops
         .apply_new_local_structure
-        .chunks(SQLITE_MAX_VARIABLE_NUMBER / 2)
+        .chunks(db.variable_limit()? / 2)
     {
         let mut statement = db.prepare(format!(
             "INSERT INTO applyNewLocalStructureOps(mergedGuid, mergedParentGuid,
                                                    position, level,
                                                    lastModifiedMicroseconds)
              VALUES {}",
             repeat_display(chunk.len(), ",", |index, f| {
                 let op = &chunk[index];
@@ -658,57 +653,51 @@ fn update_local_items_in_places<'t>(
 
             let merged_parent_guid = nsString::from(&*op.merged_parent_node.guid);
             statement.bind_by_index(offset + 1, merged_parent_guid)?;
         }
         statement.execute()?;
     }
 
     debug!(driver, "Removing tombstones for revived items");
-    for chunk in ops
-        .delete_local_tombstones
-        .chunks(SQLITE_MAX_VARIABLE_NUMBER)
-    {
+    for chunk in ops.delete_local_tombstones.chunks(db.variable_limit()?) {
         let mut statement = db.prepare(format!(
             "DELETE FROM moz_bookmarks_deleted
              WHERE guid IN ({})",
             repeat_sql_vars(chunk.len()),
         ))?;
         for (index, op) in chunk.iter().enumerate() {
             controller.err_if_aborted()?;
             statement.bind_by_index(index as u32, nsString::from(&*op.guid().as_str()))?;
         }
         statement.execute()?;
     }
 
     debug!(
         driver,
         "Inserting new tombstones for non-syncable and invalid items"
     );
-    for chunk in ops
-        .insert_local_tombstones
-        .chunks(SQLITE_MAX_VARIABLE_NUMBER)
-    {
+    for chunk in ops.insert_local_tombstones.chunks(db.variable_limit()?) {
         let mut statement = db.prepare(format!(
             "INSERT INTO moz_bookmarks_deleted(guid, dateRemoved)
              VALUES {}",
             repeat_display(chunk.len(), ",", |_, f| write!(f, "(?, {})", now)),
         ))?;
         for (index, op) in chunk.iter().enumerate() {
             controller.err_if_aborted()?;
             statement.bind_by_index(
                 index as u32,
                 nsString::from(&*op.remote_node().guid.as_str()),
             )?;
         }
         statement.execute()?;
     }
 
     debug!(driver, "Removing local items");
-    for chunk in ops.delete_local_items.chunks(SQLITE_MAX_VARIABLE_NUMBER) {
+    for chunk in ops.delete_local_items.chunks(db.variable_limit()?) {
         remove_local_items(&db, driver, controller, chunk)?;
     }
 
     // Fires the `changeGuids` trigger.
     debug!(driver, "Changing GUIDs");
     controller.err_if_aborted()?;
     db.exec("DELETE FROM changeGuidOps")?;
 
@@ -724,17 +713,17 @@ fn update_local_items_in_places<'t>(
     debug!(driver, "Applying new local structure");
     controller.err_if_aborted()?;
     db.exec("DELETE FROM applyNewLocalStructureOps")?;
 
     debug!(
         driver,
         "Resetting change counters for items that shouldn't be uploaded"
     );
-    for chunk in ops.set_local_merged.chunks(SQLITE_MAX_VARIABLE_NUMBER) {
+    for chunk in ops.set_local_merged.chunks(db.variable_limit()?) {
         let mut statement = db.prepare(format!(
             "UPDATE moz_bookmarks SET
                syncChangeCounter = 0
              WHERE guid IN ({})",
             repeat_sql_vars(chunk.len()),
         ))?;
         for (index, op) in chunk.iter().enumerate() {
             controller.err_if_aborted()?;
@@ -742,32 +731,32 @@ fn update_local_items_in_places<'t>(
         }
         statement.execute()?;
     }
 
     debug!(
         driver,
         "Bumping change counters for items that should be uploaded"
     );
-    for chunk in ops.set_local_unmerged.chunks(SQLITE_MAX_VARIABLE_NUMBER) {
+    for chunk in ops.set_local_unmerged.chunks(db.variable_limit()?) {
         let mut statement = db.prepare(format!(
             "UPDATE moz_bookmarks SET
                syncChangeCounter = 1
              WHERE guid IN ({})",
             repeat_sql_vars(chunk.len()),
         ))?;
         for (index, op) in chunk.iter().enumerate() {
             controller.err_if_aborted()?;
             statement.bind_by_index(index as u32, nsString::from(&*op.merged_node.guid))?;
         }
         statement.execute()?;
     }
 
     debug!(driver, "Flagging applied remote items as merged");
-    for chunk in ops.set_remote_merged.chunks(SQLITE_MAX_VARIABLE_NUMBER) {
+    for chunk in ops.set_remote_merged.chunks(db.variable_limit()?) {
         let mut statement = db.prepare(format!(
             "UPDATE items SET
                needsMerge = 0
              WHERE guid IN ({})",
             repeat_sql_vars(chunk.len()),
         ))?;
         for (index, op) in chunk.iter().enumerate() {
             controller.err_if_aborted()?;
@@ -1027,17 +1016,17 @@ fn stage_items_to_upload(
     upload_tombstones: &[UploadTombstone],
     weak_upload: &[nsString],
 ) -> Result<()> {
     debug!(driver, "Cleaning up staged items left from last sync");
     controller.err_if_aborted()?;
     db.exec("DELETE FROM itemsToUpload")?;
 
     debug!(driver, "Staging weak uploads");
-    for chunk in weak_upload.chunks(SQLITE_MAX_VARIABLE_NUMBER) {
+    for chunk in weak_upload.chunks(db.variable_limit()?) {
         let mut statement = db.prepare(format!(
             "INSERT INTO itemsToUpload(id, guid, syncChangeCounter, parentGuid,
                                        parentTitle, dateAdded, type, title,
                                        placeId, isQuery, url, keyword, position,
                                        tagFolderName)
              {}
              WHERE b.guid IN ({})",
             UploadItemsFragment("b"),
@@ -1062,17 +1051,17 @@ fn stage_items_to_upload(
                                              keyword, position, tagFolderName)
          {}
          JOIN itemsToApply n ON n.mergedGuid = b.guid
          WHERE n.localDateAddedMicroseconds < n.remoteDateAddedMicroseconds",
         UploadItemsFragment("b"),
     ))?;
 
     debug!(driver, "Staging remaining locally changed items for upload");
-    for chunk in upload_items.chunks(SQLITE_MAX_VARIABLE_NUMBER) {
+    for chunk in upload_items.chunks(db.variable_limit()?) {
         let mut statement = db.prepare(format!(
             "INSERT OR IGNORE INTO itemsToUpload(id, guid, syncChangeCounter,
                                                  parentGuid, parentTitle,
                                                  dateAdded, type, title,
                                                  placeId, isQuery, url, keyword,
                                                  position, tagFolderName)
              {}
              WHERE b.guid IN ({})",
@@ -1107,17 +1096,17 @@ fn stage_items_to_upload(
         SELECT o.id, t.tag
         FROM localTags t
         JOIN itemsToUpload o ON o.placeId = t.placeId",
     )?;
 
     // Finally, stage tombstones for deleted items. Ignore conflicts if we have
     // tombstones for undeleted items; Places Maintenance should clean these up.
     debug!(driver, "Staging tombstones to upload");
-    for chunk in upload_tombstones.chunks(SQLITE_MAX_VARIABLE_NUMBER) {
+    for chunk in upload_tombstones.chunks(db.variable_limit()?) {
         let mut statement = db.prepare(format!(
             "INSERT OR IGNORE INTO itemsToUpload(guid, syncChangeCounter, isDeleted)
              VALUES {}",
             repeat_display(chunk.len(), ",", |_, f| write!(f, "(?, 1, 1)"))
         ))?;
         for (index, op) in chunk.iter().enumerate() {
             controller.err_if_aborted()?;
             statement.bind_by_index(index as u32, nsString::from(op.guid().as_str()))?;
--- a/toolkit/components/places/nsNavBookmarks.cpp
+++ b/toolkit/components/places/nsNavBookmarks.cpp
@@ -44,18 +44,16 @@ extern "C" {
 int64_t NS_NavBookmarksTotalSyncChanges() {
   return nsNavBookmarks::sTotalSyncChanges;
 }
 
 }  // extern "C"
 
 PLACES_FACTORY_SINGLETON_IMPLEMENTATION(nsNavBookmarks, gBookmarksService)
 
-#define SQLITE_MAX_VARIABLE_NUMBER 999
-
 namespace {
 
 #define SKIP_TAGS(condition) ((condition) ? SkipTags : DontSkip)
 
 bool DontSkip(nsCOMPtr<nsINavBookmarkObserver> obs) { return false; }
 bool SkipTags(nsCOMPtr<nsINavBookmarkObserver> obs) {
   bool skipTags = false;
   (void)obs->GetSkipTags(&skipTags);
@@ -1269,17 +1267,24 @@ nsresult nsNavBookmarks::InsertTombstone
 }
 
 nsresult nsNavBookmarks::InsertTombstones(
     const nsTArray<TombstoneData>& aTombstones) {
   if (aTombstones.IsEmpty()) {
     return NS_OK;
   }
 
-  size_t maxRowsPerChunk = SQLITE_MAX_VARIABLE_NUMBER / 2;
+  nsCOMPtr<mozIStorageConnection> conn = mDB->MainConn();
+  NS_ENSURE_STATE(conn);
+
+  int32_t variableLimit = 0;
+  nsresult rv = conn->GetVariableLimit(&variableLimit);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  size_t maxRowsPerChunk = variableLimit / 2;
   for (uint32_t startIndex = 0; startIndex < aTombstones.Length();
        startIndex += maxRowsPerChunk) {
     size_t rowsPerChunk =
         std::min(maxRowsPerChunk, aTombstones.Length() - startIndex);
 
     // Build a query to insert all tombstones in a single statement, chunking to
     // avoid the SQLite bound parameter limit.
     nsAutoCString tombstonesToInsert;
@@ -1295,17 +1300,16 @@ nsresult nsNavBookmarks::InsertTombstone
     nsCOMPtr<mozIStorageStatement> stmt = mDB->GetStatement(
         NS_LITERAL_CSTRING("INSERT INTO moz_bookmarks_deleted "
                            "(guid, dateRemoved) ") +
         tombstonesToInsert);
     NS_ENSURE_STATE(stmt);
     mozStorageStatementScoper scoper(stmt);
 
     uint32_t paramIndex = 0;
-    nsresult rv;
     for (uint32_t i = 0; i < rowsPerChunk; ++i) {
       const TombstoneData& tombstone = aTombstones[startIndex + i];
       rv = stmt->BindUTF8StringByIndex(paramIndex++, tombstone.guid);
       NS_ENSURE_SUCCESS(rv, rv);
       rv = stmt->BindInt64ByIndex(paramIndex++, tombstone.dateRemoved);
       NS_ENSURE_SUCCESS(rv, rv);
     }