From fabc2ba6a252c356b7e082cbc47856e607fa42c2 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Wed, 30 Mar 2016 01:16:41 -0400 Subject: [PATCH] Always start at MAX() + 1 for Zotero.ID.get(), and deasyncify Instead of getting batches of unused primary key ids, even if they're lower than other ids, which for some reason seemed like a good idea in 2008, just do a `MAX()` on the table at startup and return the next available id on each call to `Zotero.ID.get()`. This is much simpler, and not reusing ids allows them to be used as a chronological sort field. While SQLite's `SELECT last_insert_rowid()` could return auto-increment values, it's unsafe with async DB access, since a second `INSERT` can come in before the first `last_insert_rowid()` is called. This is true even in a transaction unless a function that calls it is never called in parallel (e.g., with `Zotero.Promise.all()`, which can be faster than sequential `yield`s). Note that the next id is always initialized as MAX() + 1, so if an object is added and then deleted, after a restart the same id will be given. (This is equivalent to (though unrelated to) SQLite's `INTEGER PRIMARY KEY` behavior, as opposed to its `INTEGER PRIMARY KEY AUTOINCREMENT` behavior.) Closes #993, Feed items out of order --- .../content/zotero/xpcom/data/collection.js | 7 +- chrome/content/zotero/xpcom/data/creators.js | 2 +- .../content/zotero/xpcom/data/dataObject.js | 2 +- chrome/content/zotero/xpcom/data/feedItem.js | 12 +- chrome/content/zotero/xpcom/data/item.js | 9 +- chrome/content/zotero/xpcom/data/library.js | 2 +- chrome/content/zotero/xpcom/data/tags.js | 7 +- chrome/content/zotero/xpcom/id.js | 358 ++---------------- chrome/content/zotero/xpcom/proxy.js | 2 +- chrome/content/zotero/xpcom/search.js | 7 +- chrome/content/zotero/xpcom/zotero.js | 1 + 11 files changed, 59 insertions(+), 350 deletions(-) diff --git a/chrome/content/zotero/xpcom/data/collection.js b/chrome/content/zotero/xpcom/data/collection.js index 962d7a7cbd..3aae995f7f 100644 --- a/chrome/content/zotero/xpcom/data/collection.js +++ b/chrome/content/zotero/xpcom/data/collection.js @@ -259,7 +259,7 @@ Zotero.Collection.prototype._saveData = Zotero.Promise.coroutine(function* (env) var isNew = env.isNew; var options = env.options; - var collectionID = env.id = this._id = this.id ? this.id : yield Zotero.ID.get('collections'); + var collectionID = this._id = this.id ? this.id : Zotero.ID.get('collections'); Zotero.debug("Saving collection " + this.id); @@ -279,10 +279,7 @@ Zotero.Collection.prototype._saveData = Zotero.Promise.coroutine(function* (env) let placeholders = env.sqlColumns.map(function () '?').join(); let sql = "INSERT INTO collections (" + env.sqlColumns.join(', ') + ") " + "VALUES (" + placeholders + ")"; - var insertID = yield Zotero.DB.queryAsync(sql, env.sqlValues); - if (!collectionID) { - collectionID = env.id = insertID; - } + yield Zotero.DB.queryAsync(sql, env.sqlValues); } else { let sql = 'UPDATE collections SET ' diff --git a/chrome/content/zotero/xpcom/data/creators.js b/chrome/content/zotero/xpcom/data/creators.js index 177596a89a..94fbeaae86 100644 --- a/chrome/content/zotero/xpcom/data/creators.js +++ b/chrome/content/zotero/xpcom/data/creators.js @@ -90,7 +90,7 @@ Zotero.Creators = new function() { sql, [data.firstName, data.lastName, data.fieldMode] ); if (!id && create) { - id = yield Zotero.ID.get('creators'); + id = Zotero.ID.get('creators'); let sql = "INSERT INTO creators (creatorID, firstName, lastName, fieldMode) " + "VALUES (?, ?, ?, ?)"; yield Zotero.DB.queryAsync( diff --git a/chrome/content/zotero/xpcom/data/dataObject.js b/chrome/content/zotero/xpcom/data/dataObject.js index 9c6628811a..19ce3f9c8b 100644 --- a/chrome/content/zotero/xpcom/data/dataObject.js +++ b/chrome/content/zotero/xpcom/data/dataObject.js @@ -902,7 +902,7 @@ Zotero.DataObject.prototype._initSave = Zotero.Promise.coroutine(function* (env) // Undo registerObject() on failure var func = function () { - this.ObjectsClass.unload(env.id); + this.ObjectsClass.unload(this._id); }.bind(this); if (env.options.tx) { env.transactionOptions.onRollback = func; diff --git a/chrome/content/zotero/xpcom/data/feedItem.js b/chrome/content/zotero/xpcom/data/feedItem.js index 068f87fb71..771c125606 100644 --- a/chrome/content/zotero/xpcom/data/feedItem.js +++ b/chrome/content/zotero/xpcom/data/feedItem.js @@ -153,7 +153,7 @@ Zotero.FeedItem.prototype._initSave = Zotero.Promise.coroutine(function* (env) { var superOnCommit = env.transactionOptions.onCommit; env.transactionOptions.onCommit = () => { if (superOnCommit) superOnCommit(); - this.ObjectsClass._setGUIDMapping(this.guid, env.id); + this.ObjectsClass._setGUIDMapping(this.guid, this._id); }; } @@ -165,7 +165,15 @@ Zotero.FeedItem.prototype._saveData = Zotero.Promise.coroutine(function* (env) { if (this._changed.feedItemData || env.isNew) { var sql = "REPLACE INTO feedItems VALUES (?,?,?,?)"; - yield Zotero.DB.queryAsync(sql, [env.id, this.guid, this._feedItemReadTime, this._feedItemTranslatedTime]); + yield Zotero.DB.queryAsync( + sql, + [ + this.id, + this.guid, + this._feedItemReadTime, + this._feedItemTranslatedTime + ] + ); this._clearChanged('feedItemData'); } diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js index 9216d90e83..a15503655f 100644 --- a/chrome/content/zotero/xpcom/data/item.js +++ b/chrome/content/zotero/xpcom/data/item.js @@ -1225,7 +1225,7 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) { // Primary fields // // If available id value, use it -- otherwise we'll use autoincrement - var itemID = env.id = this._id = this.id ? this.id : yield Zotero.ID.get('items'); + var itemID = this._id = this.id ? this.id : Zotero.ID.get('items'); env.sqlColumns.push( 'itemTypeID', @@ -1259,10 +1259,7 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) { let sql = "INSERT INTO items (" + env.sqlColumns.join(", ") + ") " + "VALUES (" + env.sqlValues.map(function () "?").join() + ")"; - var insertID = yield Zotero.DB.queryAsync(sql, env.sqlValues); - if (!itemID) { - itemID = env.id = insertID; - } + yield Zotero.DB.queryAsync(sql, env.sqlValues); if (!env.options.skipNotifier) { Zotero.Notifier.queue('add', 'item', itemID, env.notifierData); @@ -1305,7 +1302,7 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) { let valueID = yield Zotero.DB.valueQueryAsync(valueSQL, [value], { debug: true }) if (!valueID) { - valueID = yield Zotero.ID.get('itemDataValues'); + valueID = Zotero.ID.get('itemDataValues'); yield Zotero.DB.queryAsync(insertValueSQL, [valueID, value], { debug: false }); } diff --git a/chrome/content/zotero/xpcom/data/library.js b/chrome/content/zotero/xpcom/data/library.js index 10f618cc69..09d00d4858 100644 --- a/chrome/content/zotero/xpcom/data/library.js +++ b/chrome/content/zotero/xpcom/data/library.js @@ -444,7 +444,7 @@ Zotero.Library.prototype._saveData = Zotero.Promise.coroutine(function* (env) { } if (env.isNew) { - let id = yield Zotero.ID.get('libraries'); // Cannot retrieve auto-incremented ID with async queries + let id = Zotero.ID.get('libraries'); changedCols.unshift('libraryID'); params.unshift(id); diff --git a/chrome/content/zotero/xpcom/data/tags.js b/chrome/content/zotero/xpcom/data/tags.js index 18a1418163..450e751eeb 100644 --- a/chrome/content/zotero/xpcom/data/tags.js +++ b/chrome/content/zotero/xpcom/data/tags.js @@ -65,12 +65,9 @@ Zotero.Tags = new function() { var sql = "SELECT tagID FROM tags WHERE name=?"; var id = yield Zotero.DB.valueQueryAsync(sql, data.tag); if (!id && create) { - id = yield Zotero.ID.get('tags'); + id = Zotero.ID.get('tags'); let sql = "INSERT INTO tags (tagID, name) VALUES (?, ?)"; - let insertID = yield Zotero.DB.queryAsync(sql, [id, data.tag]); - if (!id) { - id = insertID; - } + yield Zotero.DB.queryAsync(sql, [id, data.tag]); } return id; }); diff --git a/chrome/content/zotero/xpcom/id.js b/chrome/content/zotero/xpcom/id.js index 0c9d5d50bd..b8c369768e 100644 --- a/chrome/content/zotero/xpcom/id.js +++ b/chrome/content/zotero/xpcom/id.js @@ -1,9 +1,9 @@ /* ***** BEGIN LICENSE BLOCK ***** - Copyright © 2009 Center for History and New Media + Copyright © 2016 Center for History and New Media George Mason University, Fairfax, Virginia, USA - http://zotero.org + https://www.zotero.org This file is part of Zotero. @@ -24,310 +24,38 @@ */ Zotero.ID_Tracker = function () { - this.getBigInt = getBigInt; - this.skip = skip; - this.getTableName = getTableName; - - // Number of ids to compare against at a time - this.__defineGetter__('numIDs', function () 10000); - - // Number of times to try increasing the maxID if first range fails - this.__defineGetter__('maxTries', function () 3); - - // Total number of ids to find - this.__defineGetter__('maxToFind', function () 1000); - - var _available = {}; - var _min = {}; - var _skip = {}; + var _tables = [ + 'collections', + 'creators', + 'customFields', + 'customItemTypes', + 'itemDataValues', + 'items', + 'libraries', + 'proxies', + 'savedSearches', + 'tags' + ]; + var _nextIDs = {}; - /* - * Gets an unused primary key id for a DB table - */ - this.get = Zotero.Promise.coroutine(function* (table, notNull) { - table = this.getTableName(table); - - switch (table) { - // Autoincrement tables - // - // Callers need to handle a potential NULL for these unless they - // pass |notNull| - case 'libraries': - case 'items': - case 'creators': - case 'creatorData': - case 'collections': - case 'savedSearches': - case 'tags': - case 'customItemTypes': - case 'customFields': - var id = yield _getNextAvailable(table); - if (!id && notNull) { - return _getNext(table); - } - return id; - - // Non-autoincrement tables - // - // TODO: Use for everything - case 'itemDataValues': - case 'proxies': - var id = yield _getNextAvailable(table); - if (!id) { - // If we can't find an empty id quickly, just use MAX() + 1 - return _getNext(table); - } - return id; - - default: - throw ("Unsupported table '" + table + "' in Zotero.ID.get()"); + this.init = Zotero.Promise.coroutine(function* () { + for (let table of _tables) { + _nextIDs[table] = yield _getNext(table); } }); - function getBigInt(max) { - if (!max) { - max = 9007199254740991; - } - return Math.floor(Math.random() * (max)) + 1; - } - /** - * Mark ids as used - * - * @param string table - * @param int|array ids Item ids to skip + * Gets an unused primary key id for a DB table */ - function skip(table, ids) { - table = this.getTableName(table); - - switch (ids.constructor.name) { - case 'Array': - break; - - case 'Number': - ids = [ids]; - break; - - default: - throw ("ids must be an int or array of ints in Zotero.ID.skip()"); + this.get = function (table) { + if (!_nextIDs[table]) { + throw new Error("IDs not loaded for table '" + table + "'"); } - if (!ids.length) { - return; - } - - if (!_skip[table]) { - _skip[table] = {}; - } - - for (var i=0, len=ids.length; i max) { - max = parseInt(id); - } - } - if (!max) { - throw new Error("_skip['" + table + "'] must contain positive values"); - } - sql += 'MAX(' + column + ', ' + max + ')'; - } - else { - sql += column; - } - sql += ')+1 FROM ' + table; - return Zotero.DB.valueQueryAsync(sql); - }); - - - /* - * Loads available ids for table into memory - */ - var _loadAvailable = Zotero.Promise.coroutine(function* (table) { - Zotero.debug("Loading available ids for table '" + table + "'"); - - var minID = _min[table] ? _min[table] + 1 : 1; - var numIDs = Zotero.ID.numIDs; - var maxTries = Zotero.ID.maxTries; - var maxToFind = Zotero.ID.maxToFind; - - var column = _getTableColumn(table); - - switch (table) { - case 'creators': - case 'creatorData': - case 'items': - case 'itemDataValues': - case 'tags': - break; - - case 'libraries': - case 'collections': - case 'savedSearches': - case 'customItemTypes': - case 'customFields': - case 'proxies': - var maxToFind = 100; - break; - - default: - throw ("Unsupported table '" + table + "' in Zotero.ID._loadAvailable()"); - } - - var maxID = minID + numIDs - 1; - var sql = "SELECT " + column + " FROM " + table - + " WHERE " + column + " BETWEEN ? AND ? ORDER BY " + column; - var ids = yield Zotero.DB.columnQueryAsync(sql, [minID, maxID]); - // If no ids found, we have numIDs unused ids - if (!ids) { - maxID = Math.min(maxID, minID + (maxToFind - 1)); - Zotero.debug("Found " + (maxID - minID + 1) + " available ids in table '" + table + "'"); - _available[table] = [[minID, maxID]]; - return; - } - - // If we didn't find any unused ids, try increasing maxID a few times - while (ids.length == numIDs && maxTries>0) { - Zotero.debug('No available ids found between ' + minID + ' and ' + maxID + '; trying next ' + numIDs); - minID = maxID + 1; - maxID = minID + numIDs - 1; - ids = yield Zotero.DB.columnQueryAsync(sql, [minID, maxID]); - maxTries--; - } - - // Didn't find any unused ids -- _getNextAvailable() will return NULL for - // this table for rest of session - if (ids.length == numIDs) { - Zotero.debug("Found no available ids in table '" + table + "'"); - _available[table] = []; - return; - } - - var available = [], found = 0, j = 0, availableStart = null; - - for (var i=minID; i<=maxID && found i && i<=maxID) { - if (!availableStart) { - availableStart = i; - } - i++; - - if ((found + (i - availableStart) + 1) > maxToFind) { - break; - } - } - if (availableStart) { - available.push([availableStart, i-1]); - // Keep track of how many empties we've found - found += ((i-1) - availableStart) + 1; - availableStart = null; - } - j++; - } - - Zotero.debug("Found " + found + " available ids in table '" + table + "'"); - - _available[table] = available; - }); + return ++_nextIDs[table]; + }; function _getTableColumn(table) { @@ -351,33 +79,17 @@ Zotero.ID_Tracker = function () { return table.substr(0, table.length - 1) + 'ID'; } } + + + /** + * Get MAX(id) + 1 from table + * + * @return {Promise} + */ + function _getNext(table) { + var sql = 'SELECT COALESCE(MAX(' + _getTableColumn(table) + ') + 1, 1) FROM ' + table; + return Zotero.DB.valueQueryAsync(sql); + }; } Zotero.ID = new Zotero.ID_Tracker; - -/** - * Notifier observer to mark saved object ids as used - */ -Zotero.ID.EventListener = new function () { - this.init = init; - this.notify = notify; - - function init() { - Zotero.Notifier.registerObserver(this); - } - - - function notify(event, type, ids) { - if (event == 'add') { - try { - var table = Zotero.ID.getTableName(type); - } - // Skip if not a table we handle - catch (e) { - return; - } - Zotero.ID.skip(table, ids); - } - } -} - diff --git a/chrome/content/zotero/xpcom/proxy.js b/chrome/content/zotero/xpcom/proxy.js index a80fd413bd..90d0c86f4b 100644 --- a/chrome/content/zotero/xpcom/proxy.js +++ b/chrome/content/zotero/xpcom/proxy.js @@ -645,7 +645,7 @@ Zotero.Proxy.prototype.save = Zotero.Promise.coroutine(function* (transparent) { ); yield Zotero.DB.queryAsync("DELETE FROM proxyHosts WHERE proxyID = ?", [this.proxyID]); } else { - let id = yield Zotero.ID.get('proxies'); + let id = Zotero.ID.get('proxies'); yield Zotero.DB.queryAsync( "INSERT INTO proxies (proxyID, multiHost, autoAssociate, scheme) VALUES (?, ?, ?, ?)", [id, this.multiHost ? 1 : 0, this.autoAssociate ? 1 : 0, this.scheme] diff --git a/chrome/content/zotero/xpcom/search.js b/chrome/content/zotero/xpcom/search.js index 805d812d8a..9fc9695d30 100644 --- a/chrome/content/zotero/xpcom/search.js +++ b/chrome/content/zotero/xpcom/search.js @@ -148,7 +148,7 @@ Zotero.Search.prototype._saveData = Zotero.Promise.coroutine(function* (env) { var isNew = env.isNew; var options = env.options; - var searchID = env.id = this._id = this.id ? this.id : yield Zotero.ID.get('savedSearches'); + var searchID = this._id = this.id ? this.id : Zotero.ID.get('savedSearches'); env.sqlColumns.push( 'savedSearchName' @@ -164,10 +164,7 @@ Zotero.Search.prototype._saveData = Zotero.Promise.coroutine(function* (env) { let placeholders = env.sqlColumns.map(function () '?').join(); let sql = "INSERT INTO savedSearches (" + env.sqlColumns.join(', ') + ") " + "VALUES (" + placeholders + ")"; - var insertID = yield Zotero.DB.queryAsync(sql, env.sqlValues); - if (!searchID) { - searchID = env.id = insertID; - } + yield Zotero.DB.queryAsync(sql, env.sqlValues); } else { let sql = 'UPDATE savedSearches SET ' diff --git a/chrome/content/zotero/xpcom/zotero.js b/chrome/content/zotero/xpcom/zotero.js index 06adf1e4ca..0964c1d220 100644 --- a/chrome/content/zotero/xpcom/zotero.js +++ b/chrome/content/zotero/xpcom/zotero.js @@ -621,6 +621,7 @@ Components.utils.import("resource://gre/modules/osfile.jsm"); // Initialize Locate Manager Zotero.LocateManager.init(); + yield Zotero.ID.init(); yield Zotero.Collections.init(); yield Zotero.Items.init(); yield Zotero.Searches.init();