Merge pull request 'Don't fetch old values in cross-partition transactional inserts' (#877) from withings/garage:perf/kv/insert-no-return-cross-partition into main

Reviewed-on: https://git.deuxfleurs.fr/Deuxfleurs/garage/pulls/877

4 files changed, 19 insertions, 35 deletions

diff --git a/src/db/lib.rs b/src/db/lib.rs
index d6057505..3485745a 100644
--- a/src/db/lib.rs
+++ b/src/db/lib.rs
@@ -274,12 +274,12 @@ impl<'a> Transaction<'a> {
 		tree: &Tree,
 		key: T,
 		value: U,
-	) -> TxOpResult<Option<Value>> {
+	) -> TxOpResult<()> {
 		self.tx.insert(tree.1, key.as_ref(), value.as_ref())
 	}
 	/// Returns the old value if there was one
 	#[inline]
-	pub fn remove<T: AsRef<[u8]>>(&mut self, tree: &Tree, key: T) -> TxOpResult<Option<Value>> {
+	pub fn remove<T: AsRef<[u8]>>(&mut self, tree: &Tree, key: T) -> TxOpResult<()> {
 		self.tx.remove(tree.1, key.as_ref())
 	}
 	/// Clears all values in a tree
@@ -362,8 +362,8 @@ pub(crate) trait ITx {
 	fn get(&self, tree: usize, key: &[u8]) -> TxOpResult<Option<Value>>;
 	fn len(&self, tree: usize) -> TxOpResult<usize>;
 
-	fn insert(&mut self, tree: usize, key: &[u8], value: &[u8]) -> TxOpResult<Option<Value>>;
-	fn remove(&mut self, tree: usize, key: &[u8]) -> TxOpResult<Option<Value>>;
+	fn insert(&mut self, tree: usize, key: &[u8], value: &[u8]) -> TxOpResult<()>;
+	fn remove(&mut self, tree: usize, key: &[u8]) -> TxOpResult<()>;
 	fn clear(&mut self, tree: usize) -> TxOpResult<()>;
 
 	fn iter(&self, tree: usize) -> TxOpResult<TxValueIter<'_>>;
diff --git a/src/db/lmdb_adapter.rs b/src/db/lmdb_adapter.rs
index 436a67fa..de4c3910 100644
--- a/src/db/lmdb_adapter.rs
+++ b/src/db/lmdb_adapter.rs
@@ -252,17 +252,15 @@ impl<'a> ITx for LmdbTx<'a> {
 		Ok(tree.len(&self.tx)? as usize)
 	}
 
-	fn insert(&mut self, tree: usize, key: &[u8], value: &[u8]) -> TxOpResult<Option<Value>> {
+	fn insert(&mut self, tree: usize, key: &[u8], value: &[u8]) -> TxOpResult<()> {
 		let tree = *self.get_tree(tree)?;
-		let old_val = tree.get(&self.tx, key)?.map(Vec::from);
 		tree.put(&mut self.tx, key, value)?;
-		Ok(old_val)
+		Ok(())
 	}
-	fn remove(&mut self, tree: usize, key: &[u8]) -> TxOpResult<Option<Value>> {
+	fn remove(&mut self, tree: usize, key: &[u8]) -> TxOpResult<()> {
 		let tree = *self.get_tree(tree)?;
-		let old_val = tree.get(&self.tx, key)?.map(Vec::from);
 		tree.delete(&mut self.tx, key)?;
-		Ok(old_val)
+		Ok(())
 	}
 	fn clear(&mut self, tree: usize) -> TxOpResult<()> {
 		let tree = *self.get_tree(tree)?;
diff --git a/src/db/sqlite_adapter.rs b/src/db/sqlite_adapter.rs
index 5a142117..9c9a668d 100644
--- a/src/db/sqlite_adapter.rs
+++ b/src/db/sqlite_adapter.rs
@@ -192,7 +192,7 @@ impl IDb for SqliteDb {
 		let db = self.db.get()?;
 		let lock = self.write_lock.lock();
 
-		let n = db.execute(&format!("DELETE FROM {} WHERE k = ?1", tree), params![key])?;
+		db.execute(&format!("DELETE FROM {} WHERE k = ?1", tree), params![key])?;
 
 		drop(lock);
 		Ok(())
@@ -336,31 +336,17 @@ impl<'a> ITx for SqliteTx<'a> {
 		}
 	}
 
-	fn insert(&mut self, tree: usize, key: &[u8], value: &[u8]) -> TxOpResult<Option<Value>> {
+	fn insert(&mut self, tree: usize, key: &[u8], value: &[u8]) -> TxOpResult<()> {
 		let tree = self.get_tree(tree)?;
-		let old_val = self.internal_get(tree, key)?;
-
-		let sql = match &old_val {
-			Some(_) => format!("UPDATE {} SET v = ?2 WHERE k = ?1", tree),
-			None => format!("INSERT INTO {} (k, v) VALUES (?1, ?2)", tree),
-		};
-		let n = self.tx.execute(&sql, params![key, value])?;
-		assert_eq!(n, 1);
-
-		Ok(old_val)
+		let sql = format!("INSERT OR REPLACE INTO {} (k, v) VALUES (?1, ?2)", tree);
+		self.tx.execute(&sql, params![key, value])?;
+		Ok(())
 	}
-	fn remove(&mut self, tree: usize, key: &[u8]) -> TxOpResult<Option<Value>> {
+	fn remove(&mut self, tree: usize, key: &[u8]) -> TxOpResult<()> {
 		let tree = self.get_tree(tree)?;
-		let old_val = self.internal_get(tree, key)?;
-
-		if old_val.is_some() {
-			let n = self
-				.tx
-				.execute(&format!("DELETE FROM {} WHERE k = ?1", tree), params![key])?;
-			assert_eq!(n, 1);
-		}
-
-		Ok(old_val)
+		self.tx
+			.execute(&format!("DELETE FROM {} WHERE k = ?1", tree), params![key])?;
+		Ok(())
 	}
 	fn clear(&mut self, tree: usize) -> TxOpResult<()> {
 		let tree = self.get_tree(tree)?;
diff --git a/src/db/test.rs b/src/db/test.rs
index e3b7badf..26b816b8 100644
--- a/src/db/test.rs
+++ b/src/db/test.rs
@@ -21,7 +21,7 @@ fn test_suite(db: Db) {
 	let res = db.transaction::<_, (), _>(|tx| {
 		assert_eq!(tx.get(&tree, ka).unwrap().unwrap(), va);
 
-		assert_eq!(tx.insert(&tree, ka, vb).unwrap().unwrap(), va);
+		assert_eq!(tx.insert(&tree, ka, vb).unwrap(), ());
 
 		assert_eq!(tx.get(&tree, ka).unwrap().unwrap(), vb);
 
@@ -33,7 +33,7 @@ fn test_suite(db: Db) {
 	let res = db.transaction::<(), _, _>(|tx| {
 		assert_eq!(tx.get(&tree, ka).unwrap().unwrap(), vb);
 
-		assert_eq!(tx.insert(&tree, ka, vc).unwrap().unwrap(), vb);
+		assert_eq!(tx.insert(&tree, ka, vc).unwrap(), ());
 
 		assert_eq!(tx.get(&tree, ka).unwrap().unwrap(), vc);