Fix very slow purging of itemDataValues at startup in large databases

When an item is erased (removed from the trash or cleaned up from a
feed), we set a flag to purge values in `itemDataValues` on the next
startup, with this query:

DELETE FROM itemDataValues WHERE valueID NOT IN (SELECT valueID FROM itemData);

For some people, that query was incredibly slow and would result in
Zotero intermittently hanging on "Loading items…" for a long time at
startup. It's possible this is mostly limited to people who subscribe to
high-volume feeds and have a lot of item churn. One affected person had
>900K values in `itemDataValues` despite having only 20K items.

It turns out the slow query is due to the foreign-key constraint on
`itemData(valueID)` that references`itemDataValues(valueID)`. SQLite is
checking every row being deleted from `itemDataValues` against
`itemData`, even though the query is specifically removing rows that
don't exist in `itemData`! For the 900K-value DB, disabling foreign-key
checks causes the `DELETE` query to take 25 seconds instead of...some
much longer time that I didn't wait for.

We already had an `executeTransaction()` flag, `disableForeignKeys`, to
temporarily disable foreign-key checks, but it didn't do so in a way
that was safe for post-initialization usage -- a write query outside of
a transaction could've run between the transaction commit and
foreign-key checks being re-enabled. This commit changes it to properly
block all other queries unless they include an `ignoreDBLock` option,
meaning that queries within the function passed to the transaction need
to include that option. (And since that's not realistic for the couple
other uses of `disableForeignKeys` -- one for a test and one in code
that almost certainly hasn't been run by anyone in 15 years -- those now
just run `PRAGMA foreign_keys=OFF|ON` explicitly, leaving this as the
only current use.)

chrome/content/zotero/xpcom/data/items.js

@@ -1653,19 +1653,19 @@ Zotero.Items = function() {
 	/**
 	 * Purge unused data values
 	 */
-	this.purge = Zotero.Promise.coroutine(function* () {
+	this.purge = async function () {
-		Zotero.DB.requireTransaction();
-		
 		if (!Zotero.Prefs.get('purge.items')) {
 			return;
 		}
 		
-		var sql = "DELETE FROM itemDataValues WHERE valueID NOT IN "
+		await Zotero.DB.executeTransaction(async function () {
-					+ "(SELECT valueID FROM itemData)";
+			let sql = "DELETE FROM itemDataValues WHERE valueID NOT IN "
-		yield Zotero.DB.queryAsync(sql);
+				+ "(SELECT valueID FROM itemData)";
+			await Zotero.DB.queryAsync(sql, [], { ignoreDBLock: true });
+		}, { disableForeignKeys: true });
 		
 		Zotero.Prefs.set('purge.items', false)
-	});
+	};
 	
 	
 	

chrome/content/zotero/xpcom/db.js

@@ -419,12 +419,14 @@ Zotero.DBConnection.prototype.getNextName = async function (libraryID, table, fi
 /**
  * @param {Function} func - Async function containing `await Zotero.DB.queryAsync()` and similar
  * @param {Object} [options]
- * @param {Boolean} [options.disableForeignKeys] - Disable foreign key constraints before
+ * @param {Boolean} [options.disableForeignKeys] - Disable foreign key checks before the
- *    transaction and re-enable after. (`PRAGMA foreign_keys=0|1` is a no-op during a transaction.)
+ *    transaction and re-enable after, while preventing any other queries from running.
+ *    `queryAsync()` and similar within `func` must pass `ignoreDBLock: true` or they'll hang.
+ *    (`PRAGMA foreign_keys=OFF|ON` is a no-op during a transaction, so it can't just be set within
+ *    the function.)
  * @return {Promise} - Promise for result of generator function
  */
-Zotero.DBConnection.prototype.executeTransaction = async function (func, options) {
+Zotero.DBConnection.prototype.executeTransaction = async function (func, options = {}) {
-	options = options || {};
 	var resolve;
 	
 	// Set temporary options for this transaction that will be reset at the end
@@ -462,34 +464,49 @@ Zotero.DBConnection.prototype.executeTransaction = async function (func, options
 			}
 		}
 		
-		if (options.disableForeignKeys) {
+		let result;
-			await this.queryAsync("PRAGMA foreign_keys = 0");
+		let resolveDBLockPromise;
+		try {
+			let conn = this._getConnection(options) || (await this._getConnectionAsync(options));
+			
+			if (func.constructor.name == 'GeneratorFunction') {
+				throw new Error("Zotero.DB.executeTransaction() no longer takes a generator function "
+					+ "-- pass an async function instead");
+			}
+			
+			if (options.disableForeignKeys) {
+				this._dbLockPromise = new Promise(function () {
+					resolveDBLockPromise = arguments[0];
+				});
+				await this.queryAsync("PRAGMA foreign_keys=OFF", [], { ignoreDBLock: true });
+			}
+			
+			result = await conn.executeTransaction(func);
+			Zotero.debug(`Committed DB transaction ${id}`, 4);
 		}
-		
+		finally {
-		var conn = this._getConnection(options) || (await this._getConnectionAsync(options));
+			if (options.disableForeignKeys) {
-		
+				await this.queryAsync("PRAGMA foreign_keys=ON", [], { ignoreDBLock: true });
-		if (func.constructor.name == 'GeneratorFunction') {
+				if (resolveDBLockPromise) {
-			throw new Error("Zotero.DB.executeTransaction() no longer takes a generator function "
+					resolveDBLockPromise();
-				+ "-- pass an async function instead");
+					this._dbLockPromise = undefined;
+				}
+			}
 		}
 		
-		var result = await conn.executeTransaction(func);
-		Zotero.debug(`Committed DB transaction ${id}`, 4);
-		
 		// Clear transaction time
 		if (this._transactionDate) {
 			this._transactionDate = null;
 		}
 		
+		this._transactionID = null;
+		
 		if (options.vacuumOnCommit) {
 			Zotero.debug('Vacuuming database');
 			await this.queryAsync('VACUUM');
 			Zotero.debug('Done vacuuming');
-			
 		}
 		
-		this._transactionID = null;
-		
 		// Function to run once transaction has been committed but before any
 		// permanent callbacks
 		if (options.onCommit) {
@@ -546,10 +563,6 @@ Zotero.DBConnection.prototype.executeTransaction = async function (func, options
 		throw e;
 	}
 	finally {
-		if (options.disableForeignKeys) {
-			await this.queryAsync("PRAGMA foreign_keys = 1");
-		}
-		
 		// Reset options back to their previous values
 		if (options) {
 			for (let option in options) {
@@ -597,13 +610,19 @@ Zotero.DBConnection.prototype.requireTransaction = function () {
  *                         rows are Proxy objects that return values from the
  *                         underlying mozIStorageRows based on column names.
  */
-Zotero.DBConnection.prototype.queryAsync = async function (sql, params, options) {
+Zotero.DBConnection.prototype.queryAsync = async function (sql, params, options = {}) {
 	try {
 		let onRow = null;
 		let conn = this._getConnection(options) || (await this._getConnectionAsync(options));
 		if (!options || !options.noParseParams) {
 			[sql, params] = this.parseQueryAndParams(sql, params);
 		}
+		
+		if (this._dbLockPromise && !options.ignoreDBLock) {
+			Zotero.debug(`Waiting for DB lock to be released: ${sql}`, 2);
+			await this._dbLockPromise;
+		}
+		
 		if (Zotero.Debug.enabled) {
 			this.logQuery(sql, params, options);
 		}
@@ -719,6 +738,12 @@ Zotero.DBConnection.prototype.valueQueryAsync = async function (sql, params, opt
 	try {
 		let conn = this._getConnection(options) || (await this._getConnectionAsync(options));
 		[sql, params] = this.parseQueryAndParams(sql, params);
+		
+		if (this._dbLockPromise && !options.ignoreDBLock) {
+			Zotero.debug(`Waiting for DB lock to be released: ${sql}`, 2);
+			await this._dbLockPromise;
+		}
+		
 		if (Zotero.Debug.enabled) {
 			this.logQuery(sql, params, options);
 		}
@@ -766,6 +791,12 @@ Zotero.DBConnection.prototype.columnQueryAsync = async function (sql, params, op
 	try {
 		let conn = this._getConnection(options) || (await this._getConnectionAsync(options));
 		[sql, params] = this.parseQueryAndParams(sql, params);
+		
+		if (this._dbLockPromise && !options.ignoreDBLock) {
+			Zotero.debug(`Waiting for DB lock to be released: ${sql}`, 2);
+			await this._dbLockPromise;
+		}
+		
 		if (Zotero.Debug.enabled) {
 			this.logQuery(sql, params, options);
 		}

chrome/content/zotero/xpcom/schema.js

@@ -600,9 +600,15 @@ Zotero.Schema = new function(){
 	
 	
 	this._updateGlobalSchemaForTest = async function (schema) {
-		await Zotero.DB.executeTransaction(async function () {
+		await Zotero.DB.queryAsync("PRAGMA foreign_keys=OFF");
-			await _updateGlobalSchema(schema);
+		try {
-		}.bind(this), { disableForeignKeys: true });
+			await Zotero.DB.executeTransaction(async function () {
+				await _updateGlobalSchema(schema);
+			});
+		}
+		finally {
+			await Zotero.DB.queryAsync("PRAGMA foreign_keys=ON");
+		}
 	};
 	
 	
@@ -733,58 +739,64 @@ Zotero.Schema = new function(){
 			
 			var itemTypeID = Zotero.ID.get('customItemTypes');
 			
-			yield Zotero.DB.executeTransaction(async function () {
+			yield Zotero.DB.queryAsync("PRAGMA foreign_keys=OFF");
-				await Zotero.DB.queryAsync("INSERT INTO customItemTypes VALUES (?, 'nsfReviewer', 'NSF Reviewer', 1, 'chrome://zotero/skin/report_user.png')", itemTypeID);
+			try {
-				
+				yield Zotero.DB.executeTransaction(async function () {
-				var fields = [
+					await Zotero.DB.queryAsync("INSERT INTO customItemTypes VALUES (?, 'nsfReviewer', 'NSF Reviewer', 1, 'chrome://zotero/skin/report_user.png')", itemTypeID);
-					['name', 'Name'],
+					
-					['institution', 'Institution'],
+					var fields = [
-					['address', 'Address'],
+						['name', 'Name'],
-					['telephone', 'Telephone'],
+						['institution', 'Institution'],
-					['email', 'Email'],
+						['address', 'Address'],
-					['homepage', 'Webpage'],
+						['telephone', 'Telephone'],
-					['discipline', 'Discipline'],
+						['email', 'Email'],
-					['nsfID', 'NSF ID'],
+						['homepage', 'Webpage'],
-					['dateSent', 'Date Sent'],
+						['discipline', 'Discipline'],
-					['dateDue', 'Date Due'],
+						['nsfID', 'NSF ID'],
-					['accepted', 'Accepted'],
+						['dateSent', 'Date Sent'],
-					['programDirector', 'Program Director']
+						['dateDue', 'Date Due'],
-				];
+						['accepted', 'Accepted'],
-				for (var i=0; i<fields.length; i++) {
+						['programDirector', 'Program Director']
-					var fieldID = Zotero.ItemFields.getID(fields[i][0]);
+					];
-					if (!fieldID) {
+					for (var i=0; i<fields.length; i++) {
-						var fieldID = Zotero.ID.get('customFields');
+						var fieldID = Zotero.ItemFields.getID(fields[i][0]);
-						await Zotero.DB.queryAsync("INSERT INTO customFields VALUES (?, ?, ?)", [fieldID, fields[i][0], fields[i][1]]);
+						if (!fieldID) {
-						await Zotero.DB.queryAsync("INSERT INTO customItemTypeFields VALUES (?, NULL, ?, 1, ?)", [itemTypeID, fieldID, i+1]);
+							var fieldID = Zotero.ID.get('customFields');
-					}
+							await Zotero.DB.queryAsync("INSERT INTO customFields VALUES (?, ?, ?)", [fieldID, fields[i][0], fields[i][1]]);
-					else {
+							await Zotero.DB.queryAsync("INSERT INTO customItemTypeFields VALUES (?, NULL, ?, 1, ?)", [itemTypeID, fieldID, i+1]);
-						await Zotero.DB.queryAsync("INSERT INTO customItemTypeFields VALUES (?, ?, NULL, 1, ?)", [itemTypeID, fieldID, i+1]);
+						}
+						else {
+							await Zotero.DB.queryAsync("INSERT INTO customItemTypeFields VALUES (?, ?, NULL, 1, ?)", [itemTypeID, fieldID, i+1]);
+						}
+						
+						switch (fields[i][0]) {
+							case 'name':
+								var baseFieldID = Zotero.ItemFields.getID('title');
+								break;
+							
+							case 'dateSent':
+								var baseFieldID = Zotero.ItemFields.getID('date');
+								break;
+							
+							case 'homepage':
+								var baseFieldID = Zotero.ItemFields.getID('url');
+								break;
+							
+							default:
+								var baseFieldID = null;
+						}
+						
+						if (baseFieldID) {
+							await Zotero.DB.queryAsync("INSERT INTO customBaseFieldMappings VALUES (?, ?, ?)", [itemTypeID, baseFieldID, fieldID]);
+						}
 					}
 					
-					switch (fields[i][0]) {
+					await _reloadSchema();
-						case 'name':
+				});
-							var baseFieldID = Zotero.ItemFields.getID('title');
+			}
-							break;
+			finally {
-						
+				yield Zotero.DB.queryAsync("PRAGMA foreign_keys=ON");
-						case 'dateSent':
+			}
-							var baseFieldID = Zotero.ItemFields.getID('date');
-							break;
-						
-						case 'homepage':
-							var baseFieldID = Zotero.ItemFields.getID('url');
-							break;
-						
-						default:
-							var baseFieldID = null;
-					}
-					
-					if (baseFieldID) {
-						await Zotero.DB.queryAsync("INSERT INTO customBaseFieldMappings VALUES (?, ?, ?)", [itemTypeID, baseFieldID, fieldID]);
-					}
-				}
-				
-				await _reloadSchema();
-			}, { disableForeignKeys: true });
 			
 			var s = new Zotero.Search;
 			s.name = "Overdue NSF Reviewers";
@@ -814,26 +826,32 @@ Zotero.Schema = new function(){
 			}
 			
 			Zotero.debug("Uninstalling nsfReviewer item type");
-			yield Zotero.DB.executeTransaction(async function () {
+			yield Zotero.DB.queryAsync("PRAGMA foreign_keys=OFF");
-				await Zotero.DB.queryAsync("DELETE FROM customItemTypeFields WHERE customItemTypeID=?", itemTypeID - Zotero.ItemTypes.customIDOffset);
+			try {
-				await Zotero.DB.queryAsync("DELETE FROM customBaseFieldMappings WHERE customItemTypeID=?", itemTypeID - Zotero.ItemTypes.customIDOffset);
+				yield Zotero.DB.executeTransaction(async function () {
-				var fields = Zotero.ItemFields.getItemTypeFields(itemTypeID);
+					await Zotero.DB.queryAsync("DELETE FROM customItemTypeFields WHERE customItemTypeID=?", itemTypeID - Zotero.ItemTypes.customIDOffset);
-				for (let fieldID of fields) {
+					await Zotero.DB.queryAsync("DELETE FROM customBaseFieldMappings WHERE customItemTypeID=?", itemTypeID - Zotero.ItemTypes.customIDOffset);
-					if (Zotero.ItemFields.isCustom(fieldID)) {
+					var fields = Zotero.ItemFields.getItemTypeFields(itemTypeID);
-						await Zotero.DB.queryAsync("DELETE FROM customFields WHERE customFieldID=?", fieldID - Zotero.ItemTypes.customIDOffset);
+					for (let fieldID of fields) {
+						if (Zotero.ItemFields.isCustom(fieldID)) {
+							await Zotero.DB.queryAsync("DELETE FROM customFields WHERE customFieldID=?", fieldID - Zotero.ItemTypes.customIDOffset);
+						}
 					}
-				}
+					await Zotero.DB.queryAsync("DELETE FROM customItemTypes WHERE customItemTypeID=?", itemTypeID - Zotero.ItemTypes.customIDOffset);
-				await Zotero.DB.queryAsync("DELETE FROM customItemTypes WHERE customItemTypeID=?", itemTypeID - Zotero.ItemTypes.customIDOffset);
+					
-				
+					var searches = Zotero.Searches.getByLibrary(Zotero.Libraries.userLibraryID);
-				var searches = Zotero.Searches.getByLibrary(Zotero.Libraries.userLibraryID);
+					for (let search of searches) {
-				for (let search of searches) {
+						if (search.name == 'Overdue NSF Reviewers') {
-					if (search.name == 'Overdue NSF Reviewers') {
+							await search.erase();
-						await search.erase();
+						}
 					}
-				}
+					
-				
+					await _reloadSchema();
-				await _reloadSchema();
+				});
-			}.bind(this), { disableForeignKeys: true });
+			}
+			finally {
+				yield Zotero.DB.queryAsync("PRAGMA foreign_keys=ON");
+			}
 			
 			ps.alert(null, "Zotero Item Type Removed", "The 'NSF Reviewer' item type has been uninstalled.");
 		}

chrome/content/zotero/xpcom/zotero.js

@@ -1754,9 +1754,7 @@ Services.scriptloader.loadSubScript("resource://zotero/polyfill.js");
 			return Zotero.Tags.purge();
 		});
 		yield Zotero.Fulltext.purgeUnusedWords();
-		yield Zotero.DB.executeTransaction(async function () {
+		yield Zotero.Items.purge();
-			return Zotero.Items.purge();
-		});
 		// DEBUG: this might not need to be permanent
 		//yield Zotero.DB.executeTransaction(async function () {
 		//	return Zotero.Relations.purge();