From 367e4411cc5c15717b10e83812a54687622c02de Mon Sep 17 00:00:00 2001 From: Lina Cambridge Date: Tue, 15 Oct 2019 21:23:26 +0000 Subject: [PATCH] Bug 1588329 - Use the new `variableLimit` getter to chunk binding params in Places. r=mak Differential Revision: https://phabricator.services.mozilla.com/D49073 --HG-- extra : moz-landing-system : lando --- toolkit/components/places/Bookmarks.jsm | 23 +++--------- toolkit/components/places/PlacesSyncUtils.jsm | 6 +-- .../places/SyncedBookmarksMirror.jsm | 4 +- .../places/bookmark_sync/src/store.rs | 37 +++++++------------ toolkit/components/places/nsNavBookmarks.cpp | 12 ++++-- 5 files changed, 29 insertions(+), 53 deletions(-) diff --git a/toolkit/components/places/Bookmarks.jsm b/toolkit/components/places/Bookmarks.jsm index 7204809830a2..54ab97e58001 100644 --- a/toolkit/components/places/Bookmarks.jsm +++ b/toolkit/components/places/Bookmarks.jsm @@ -103,7 +103,6 @@ async function promiseTagsFolderId() { 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({ /** @@ -2217,16 +2216,10 @@ function insertBookmarkTree(items, source, parent, urls, lastAddedForParent) { ); // 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) ); } @@ -2628,10 +2621,7 @@ function removeBookmarks(items, 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. @@ -2639,9 +2629,8 @@ function removeBookmarks(items, options) { // 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) ); } diff --git a/toolkit/components/places/PlacesSyncUtils.jsm b/toolkit/components/places/PlacesSyncUtils.jsm index da043b6a5cf5..0968de0e9629 100644 --- a/toolkit/components/places/PlacesSyncUtils.jsm +++ b/toolkit/components/places/PlacesSyncUtils.jsm @@ -31,7 +31,6 @@ 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"; @@ -238,10 +237,7 @@ const HistorySyncUtils = (PlacesSyncUtils.history = Object.freeze({ // 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 diff --git a/toolkit/components/places/SyncedBookmarksMirror.jsm b/toolkit/components/places/SyncedBookmarksMirror.jsm index 833524c5ce61..7c7297504d8f 100644 --- a/toolkit/components/places/SyncedBookmarksMirror.jsm +++ b/toolkit/components/places/SyncedBookmarksMirror.jsm @@ -80,8 +80,6 @@ const SyncedBookmarksMerger = Components.Constructor( 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; @@ -1008,7 +1006,7 @@ class SyncedBookmarksMirror { 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( diff --git a/toolkit/components/places/bookmark_sync/src/store.rs b/toolkit/components/places/bookmark_sync/src/store.rs index a408c94dac8d..7dd20d9aa441 100644 --- a/toolkit/components/places/bookmark_sync/src/store.rs +++ b/toolkit/components/places/bookmark_sync/src/store.rs @@ -17,8 +17,6 @@ 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; } @@ -487,7 +485,7 @@ fn update_local_items_in_places<'t>( // 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) @@ -517,10 +515,7 @@ fn update_local_items_in_places<'t>( // 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_ @@ -594,7 +589,7 @@ fn update_local_items_in_places<'t>( } 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) @@ -636,7 +631,7 @@ fn update_local_items_in_places<'t>( 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, @@ -663,10 +658,7 @@ fn update_local_items_in_places<'t>( } 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 ({})", @@ -683,10 +675,7 @@ fn update_local_items_in_places<'t>( 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 {}", @@ -703,7 +692,7 @@ fn update_local_items_in_places<'t>( } 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)?; } @@ -729,7 +718,7 @@ fn update_local_items_in_places<'t>( 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 @@ -747,7 +736,7 @@ fn update_local_items_in_places<'t>( 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 @@ -762,7 +751,7 @@ fn update_local_items_in_places<'t>( } 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 @@ -1032,7 +1021,7 @@ fn stage_items_to_upload( 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, @@ -1067,7 +1056,7 @@ fn stage_items_to_upload( ))?; 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, @@ -1112,7 +1101,7 @@ fn stage_items_to_upload( // 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 {}", diff --git a/toolkit/components/places/nsNavBookmarks.cpp b/toolkit/components/places/nsNavBookmarks.cpp index 6f1efe388054..2e5c4a9661e2 100644 --- a/toolkit/components/places/nsNavBookmarks.cpp +++ b/toolkit/components/places/nsNavBookmarks.cpp @@ -49,8 +49,6 @@ int64_t NS_NavBookmarksTotalSyncChanges() { PLACES_FACTORY_SINGLETON_IMPLEMENTATION(nsNavBookmarks, gBookmarksService) -#define SQLITE_MAX_VARIABLE_NUMBER 999 - namespace { #define SKIP_TAGS(condition) ((condition) ? SkipTags : DontSkip) @@ -1274,7 +1272,14 @@ nsresult nsNavBookmarks::InsertTombstones( return NS_OK; } - size_t maxRowsPerChunk = SQLITE_MAX_VARIABLE_NUMBER / 2; + nsCOMPtr 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 = @@ -1300,7 +1305,6 @@ nsresult nsNavBookmarks::InsertTombstones( 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);