Remove timestamp/version columns from libraries table on master [DB u…

…pdate] Avoid writing to master on every write and just update the timestamp and version in the shard's shardLibraries table
VivienDelmon · Oct 19, 2021 · 9b96401 · 9b96401
1 parent 5496d81
commit 9b96401
Show file tree

Hide file tree

Showing 7 changed files with 79 additions and 106 deletions.
diff --git a/misc/db-updates/2021-10-16/0_addMasterGroupHasData b/misc/db-updates/2021-10-16/0_addMasterGroupHasData
@@ -0,0 +1,8 @@
+#!/usr/bin/php -d mysqlnd.net_read_timeout=3600
+<?php
+set_include_path("../../../include");
+require("header.inc.php");
+
+// Run this before rolling out code changes
+Zotero_Admin_DB::query("ALTER TABLE `libraries` ADD `hasData` TINYINT( 1 ) NOT NULL DEFAULT '0' AFTER `version` , ADD INDEX ( `hasData` )");
+Zotero_DB::query("UPDATE `libraries` SET hasData=1 WHERE version > 0 OR lastUpdated != '0000-00-00 00:00:00'");
diff --git a/misc/db-updates/2021-10-16/1_removeMasterGroupTimestampAndVersion b/misc/db-updates/2021-10-16/1_removeMasterGroupTimestampAndVersion
@@ -0,0 +1,8 @@
+#!/usr/bin/php -d mysqlnd.net_read_timeout=3600
+<?php
+set_include_path("../../../include");
+require("header.inc.php");
+
+// Run this after rolling out code changes
+Zotero_DB::query("UPDATE `libraries` SET hasData=1 WHERE version > 0 OR lastUpdated != '0000-00-00 00:00:00'");
+Zotero_Admin_DB::query("ALTER TABLE `libraries` DROP COLUMN `lastUpdated`, DROP COLUMN `version`");
diff --git a/misc/test_reset b/misc/test_reset
@@ -55,9 +55,9 @@ echo "INSERT INTO shards VALUES (1, 1, 'zoterotest1', 'pass1', 'zoterotest1', 'u
 echo "INSERT INTO shards VALUES (2, 1, 'zoterotest2', 'pass2', 'zoterotest2', 'up');" | $MASTER zoterotest_master
 
 # Initial users and groups for tests
-echo "INSERT INTO libraries VALUES (1, 'user', '0000-00-00 00:00:00', 0, 1)" | $MASTER zoterotest_master
-echo "INSERT INTO libraries VALUES (2, 'user', '0000-00-00 00:00:00', 0, 1)" | $MASTER zoterotest_master
-echo "INSERT INTO libraries VALUES (3, 'group', '0000-00-00 00:00:00', 0, 2)" | $MASTER zoterotest_master
+echo "INSERT INTO libraries VALUES (1, 'user', 0, 1)" | $MASTER zoterotest_master
+echo "INSERT INTO libraries VALUES (2, 'user', 0, 1)" | $MASTER zoterotest_master
+echo "INSERT INTO libraries VALUES (3, 'group', 0, 2)" | $MASTER zoterotest_master
 echo "INSERT INTO users VALUES (1, 1, 'testuser', '0000-00-00 00:00:00', '0000-00-00 00:00:00')" | $MASTER zoterotest_master
 echo "INSERT INTO users VALUES (2, 2, 'testuser2', '0000-00-00 00:00:00', '0000-00-00 00:00:00')" | $MASTER zoterotest_master
 echo "INSERT INTO `groups` VALUES (1, 3, 'Test Group', 'test_group', 'Private', 1, 'admins', 'all', 'members', '', '', 0, '0000-00-00 00:00:00', '0000-00-00 00:00:00')" | $MASTER zoterotest_master

diff --git a/model/Groups.inc.php b/model/Groups.inc.php
@@ -148,8 +148,7 @@ public static function getAllAdvanced($userID=false, $params=array(), $permissio
 				}
 				else {
 					// Don't include groups that have never had items
-					$whereSQL = "JOIN libraries L ON (G.libraryID=L.libraryID)
-							WHERE L.lastUpdated != '0000-00-00 00:00:00' ";
+					$whereSQL = "JOIN libraries L ON (G.libraryID=L.libraryID) WHERE L.hasData = 1 ";
 				}
 				$sql .= $whereSQL;
 				$countSQL .= $whereSQL;

diff --git a/model/Libraries.inc.php b/model/Libraries.inc.php
@@ -139,13 +139,6 @@ public static function getUserLibraries($userID) {
 	}
 
 
-	public static function updateVersionAndTimestamp($libraryID) {
-		$timestamp = self::updateTimestamps($libraryID);
-		Zotero_DB::registerTransactionTimestamp($timestamp);
-		self::updateVersion($libraryID);
-	}
-
-
 	public static function getTimestamp($libraryID) {
 		$sql = "SELECT lastUpdated FROM shardLibraries WHERE libraryID=?";
 		return Zotero_DB::valueQuery(
@@ -154,57 +147,6 @@ public static function getTimestamp($libraryID) {
 	}
 
 
-	public static function getUserLibraryUpdateTimes($userID) {
-		$libraryIDs = Zotero_Libraries::getUserLibraries($userID);
-		$sql = "SELECT libraryID, UNIX_TIMESTAMP(lastUpdated) AS lastUpdated FROM libraries
-				WHERE libraryID IN ("
-				. implode(',', array_fill(0, sizeOf($libraryIDs), '?'))
-				. ") LOCK IN SHARE MODE";
-		$rows = Zotero_DB::query($sql, $libraryIDs);
-		$updateTimes = array();
-		foreach ($rows as $row) {
-			$updateTimes[$row['libraryID']] = $row['lastUpdated'];
-		}
-		return $updateTimes;
-	}
-
-
-	public static function updateTimestamps($libraryIDs) {
-		if (is_scalar($libraryIDs)) {
-			if (!is_numeric($libraryIDs)) {
-				throw new Exception("Invalid library ID");
-			}
-			$libraryIDs = array($libraryIDs);
-		}
-
-		Zotero_DB::beginTransaction();
-
-		// TODO: replace with just shardLibraries after sync code removal
-		$sql = "UPDATE libraries SET lastUpdated=NOW() WHERE libraryID IN "
-				. "(" . implode(',', array_fill(0, sizeOf($libraryIDs), '?')) . ")";
-		Zotero_DB::query($sql, $libraryIDs);
-
-		$sql = "SELECT UNIX_TIMESTAMP(lastUpdated) FROM libraries WHERE libraryID=?";
-		$timestamp = Zotero_DB::valueQuery($sql, $libraryIDs[0]);
-
-		$sql = "UPDATE shardLibraries SET lastUpdated=FROM_UNIXTIME(?) WHERE libraryID=?";
-		foreach ($libraryIDs as $libraryID) {
-			Zotero_DB::query(
-				$sql,
-				array(
-					$timestamp,
-					$libraryID
-				),
-				Zotero_Shards::getByLibraryID($libraryID)
-			);
-		}
-
-		Zotero_DB::commit();
-
-		return $timestamp;
-	}
-
-
 	public static function setTimestampLock($libraryIDs, $timestamp) {
 		$fail = false;
 
@@ -242,40 +184,6 @@ public static function getVersion($libraryID) {
 			$sql, $libraryID, Zotero_Shards::getByLibraryID($libraryID)
 		);
 
-		// TEMP: Remove after classic sync, and use shardLibraries only for version info?
-		if (!$version || $version == 1) {
-			$shardID = Zotero_Shards::getByLibraryID($libraryID);
-			$readOnly = Zotero_DB::isReadOnly($shardID);
-
-			$sql = "SELECT lastUpdated, version FROM libraries WHERE libraryID=?";
-			$row = Zotero_DB::rowQuery($sql, $libraryID);
-
-			$sql = "UPDATE shardLibraries SET version=?, lastUpdated=? WHERE libraryID=?";
-			Zotero_DB::query(
-				$sql,
-				array($row['version'], $row['lastUpdated'], $libraryID),
-				$shardID,
-				[
-					'writeInReadMode' => true
-				]
-			);
-			$sql = "SELECT IFNULL(IF(MAX(version)=0, 1, MAX(version)), 1) FROM items WHERE libraryID=?";
-			$version = Zotero_DB::valueQuery($sql, $libraryID, $shardID);
-
-			$sql = "UPDATE shardLibraries SET version=? WHERE libraryID=?";
-			Zotero_DB::query(
-				$sql,
-				[
-					$version,
-					$libraryID
-				],
-				$shardID,
-				[
-					'writeInReadMode' => true
-				]
-			);
-		}
-
 		// Store original version for use by getOriginalVersion()
 		if (!isset(self::$originalVersions[$libraryID])) {
 			self::$originalVersions[$libraryID] = $version;
@@ -318,17 +226,31 @@ public static function getUpdatedVersion($libraryID) {
 	}
 
 
-	public static function updateVersion($libraryID) {
-		self::getOriginalVersion($libraryID);
+	public static function updateVersionAndTimestamp($libraryID) {
+		if (!is_numeric($libraryID)) {
+			throw new Exception("Invalid library ID");
+		}
 
 		$shardID = Zotero_Shards::getByLibraryID($libraryID);
-		$sql = "UPDATE shardLibraries SET version=LAST_INSERT_ID(version+1)
-				WHERE libraryID=?";
+
+		$originalVersion = self::getOriginalVersion($libraryID);
+		$sql = "UPDATE shardLibraries SET version=LAST_INSERT_ID(version+1), lastUpdated=NOW() "
+			. "WHERE libraryID=?";
 		Zotero_DB::query($sql, $libraryID, $shardID);
 		$version = Zotero_DB::valueQuery("SELECT LAST_INSERT_ID()", false, $shardID);
 		// Store new version for use by getUpdatedVersion()
 		self::$updatedVersions[$libraryID] = $version;
-		return $version;
+
+		$sql = "SELECT UNIX_TIMESTAMP(lastUpdated) FROM shardLibraries WHERE libraryID=?";
+		$timestamp = Zotero_DB::valueQuery($sql, $libraryID, $shardID);
+
+		// If library has never been written to before, mark it as having data
+		if (!$originalVersion || $originalVersion == 1) {
+			$sql = "UPDATE libraries SET hasData=1 WHERE libraryID=?";
+			Zotero_DB::query($sql, $libraryID);
+		}
+
+		Zotero_DB::registerTransactionTimestamp($timestamp);
 	}
 
 
@@ -502,8 +424,7 @@ public static function clearAllData($libraryID) {
 
 		Zotero_FullText::deleteByLibrary($libraryID);
 
-		self::updateVersion($libraryID);
-		self::updateTimestamps($libraryID);
+		self::updateVersionAndTimestamp($libraryID);
 
 		Zotero_Notifier::trigger("clear", "library", $libraryID);
 

diff --git a/tests/remote/include/api3.inc.php b/tests/remote/include/api3.inc.php
@@ -66,7 +66,7 @@ public static function useAPIKey($key = "") {
 	public static function createGroup($fields) {
 		$xml = new \SimpleXMLElement('<group/>');
 		$xml['owner'] = $fields['owner'];
-		$xml['name'] = "Test Group " . uniqid();
+		$xml['name'] = $fields['name'] ?? "Test Group " . uniqid();
 		$xml['type'] = $fields['type'];
 		$xml['libraryEditing'] = isset($fields['libraryEditing'])
 			? $fields['libraryEditing']

diff --git a/tests/remote/tests/API/3/GroupTest.php b/tests/remote/tests/API/3/GroupTest.php
@@ -273,5 +273,42 @@ public function testUpdateMemberJSON() {
 
 		API::deleteGroup($groupID);
 	}
+
+
+	public function test_group_should_not_appear_in_search_until_first_populated() {
+		$name = \Zotero_Utilities::randomString(14);
+		$groupID = API::createGroup([
+			'owner' => self::$config['userID'],
+			'type' => 'PublicClosed',
+			'name' => $name,
+			'libraryReading' => 'all'
+		]);
+
+		// Group shouldn't show up if it's never had items
+		$response = API::superGet("groups?q=$name");
+		$this->assertNumResults(0, $response);
+
+		API::groupCreateItem($groupID, "book", false, $this);
+
+		$response = API::superGet("groups?q=$name");
+		$this->assertNumResults(1, $response);
+
+		API::deleteGroup($groupID);
+	}
+
+
+	public function testDeleteGroup() {
+		$groupID = API::createGroup([
+			'owner' => self::$config['userID'],
+			'type' => 'Private',
+			'libraryReading' => 'all'
+		]);
+		API::groupCreateItem($groupID, "book", false, $this, 'key');
+		API::groupCreateItem($groupID, "book", false, $this, 'key');
+		API::groupCreateItem($groupID, "book", false, $this, 'key');
+		API::deleteGroup($groupID);
+
+		$response = API::groupGet($groupID, "");
+		$this->assert404($response);
+	}
 }
-?>