Merge pull request 'garage_db: refactor open function' (#1142) from factor-db-open into main-v1

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

src/db/fjall_adapter.rs

@@ -11,12 +11,30 @@ use fjall::{
 };
 
 use crate::{
+	open::{Engine, OpenOpt},
 	Db, Error, IDb, ITx, ITxFn, OnCommit, Result, TxError, TxFnResult, TxOpError, TxOpResult,
 	TxResult, TxValueIter, Value, ValueIter,
 };
 
 pub use fjall;
 
+// --
+
+pub(crate) fn open_db(path: &PathBuf, opt: &OpenOpt) -> Result<Db> {
+	info!("Opening Fjall database at: {}", path.display());
+	if opt.fsync {
+		return Err(Error(
+			"metadata_fsync is not supported with the Fjall database engine".into(),
+		));
+	}
+	let mut config = fjall::Config::new(path);
+	if let Some(block_cache_size) = opt.fjall_block_cache_size {
+		config = config.cache_size(block_cache_size as u64);
+	}
+	let keyspace = config.open_transactional()?;
+	Ok(FjallDb::init(keyspace))
+}
+
 // -- err
 
 impl From<fjall::Error> for Error {
@@ -95,10 +113,9 @@ impl IDb for FjallDb {
 			.collect::<Result<Vec<_>>>()?)
 	}
 
-	fn snapshot(&self, to: &PathBuf) -> Result<()> {
-		std::fs::create_dir_all(to)?;
-		let mut path = to.clone();
-		path.push("data.fjall");
+	fn snapshot(&self, base_path: &PathBuf) -> Result<()> {
+		std::fs::create_dir_all(base_path)?;
+		let path = Engine::Fjall.db_path(base_path);
 
 		let source_state = self.keyspace.read_tx();
 		let copy_keyspace = fjall::Config::new(path).open()?;

src/db/lmdb_adapter.rs

@@ -11,12 +11,55 @@ use heed::types::ByteSlice;
 use heed::{BytesDecode, Env, RoTxn, RwTxn, UntypedDatabase as Database};
 
 use crate::{
+	open::{Engine, OpenOpt},
 	Db, Error, IDb, ITx, ITxFn, OnCommit, Result, TxError, TxFnResult, TxOpError, TxOpResult,
 	TxResult, TxValueIter, Value, ValueIter,
 };
 
 pub use heed;
 
+// ---- top-level open function
+
+pub(crate) fn open_db(path: &PathBuf, opt: &OpenOpt) -> Result<Db> {
+	info!("Opening LMDB database at: {}", path.display());
+	if let Err(e) = std::fs::create_dir_all(&path) {
+		return Err(Error(
+			format!("Unable to create LMDB data directory: {}", e).into(),
+		));
+	}
+
+	let map_size = match opt.lmdb_map_size {
+		None => recommended_map_size(),
+		Some(v) => v - (v % 4096),
+	};
+
+	let mut env_builder = heed::EnvOpenOptions::new();
+	env_builder.max_dbs(100);
+	env_builder.map_size(map_size);
+	env_builder.max_readers(2048);
+	unsafe {
+		env_builder.flag(heed::flags::Flags::MdbNoRdAhead);
+		env_builder.flag(heed::flags::Flags::MdbNoMetaSync);
+		if !opt.fsync {
+			env_builder.flag(heed::flags::Flags::MdbNoSync);
+		}
+	}
+	match env_builder.open(&path) {
+		Err(heed::Error::Io(e)) if e.kind() == std::io::ErrorKind::OutOfMemory => {
+			return Err(Error(
+				"OutOfMemory error while trying to open LMDB database. This can happen \
+                if your operating system is not allowing you to use sufficient virtual \
+                memory address space. Please check that no limit is set (ulimit -v). \
+                You may also try to set a smaller `lmdb_map_size` configuration parameter. \
+                On 32-bit machines, you should probably switch to another database engine."
+					.into(),
+			))
+		}
+		Err(e) => Err(Error(format!("Cannot open LMDB database: {}", e).into())),
+		Ok(db) => Ok(LmdbDb::init(db)),
+	}
+}
+
 // -- err
 
 impl From<heed::Error> for Error {
@@ -104,10 +147,9 @@ impl IDb for LmdbDb {
 		Ok(ret2)
 	}
 
-	fn snapshot(&self, to: &PathBuf) -> Result<()> {
-		std::fs::create_dir_all(to)?;
-		let mut path = to.clone();
-		path.push("data.mdb");
+	fn snapshot(&self, base_path: &PathBuf) -> Result<()> {
+		std::fs::create_dir_all(base_path)?;
+		let path = Engine::Lmdb.db_path(base_path);
 		self.db
 			.copy_to_path(path, heed::CompactionOption::Enabled)?;
 		Ok(())

src/db/open.rs

@@ -1,4 +1,3 @@
-use std::convert::TryInto;
 use std::path::PathBuf;
 
 use crate::{Db, Error, Result};
@@ -24,6 +23,23 @@ impl Engine {
 			Self::Fjall => "fjall",
 		}
 	}
+
+	/// Return engine-specific DB path from base path
+	pub fn db_path(&self, base_path: &PathBuf) -> PathBuf {
+		let mut ret = base_path.clone();
+		match self {
+			Self::Lmdb => {
+				ret.push("db.lmdb");
+			}
+			Self::Sqlite => {
+				ret.push("db.sqlite");
+			}
+			Self::Fjall => {
+				ret.push("db.fjall");
+			}
+		}
+		ret
+	}
 }
 
 impl std::fmt::Display for Engine {
@@ -43,7 +59,7 @@ impl std::str::FromStr for Engine {
 			"sled" => Err(Error("Sled is no longer supported as a database engine. Converting your old metadata db can be done using an older Garage binary (e.g. v0.9.4).".into())),
 			kind => Err(Error(
 				format!(
-					"Invalid DB engine: {} (options are: lmdb, sqlite)",
+					"Invalid DB engine: {} (options are: lmdb, sqlite, fjall)",
 					kind
 				)
 				.into(),
@@ -72,70 +88,15 @@ pub fn open_db(path: &PathBuf, engine: Engine, opt: &OpenOpt) -> Result<Db> {
 	match engine {
 		// ---- Sqlite DB ----
 		#[cfg(feature = "sqlite")]
-		Engine::Sqlite => {
-			info!("Opening Sqlite database at: {}", path.display());
-			let manager = r2d2_sqlite::SqliteConnectionManager::file(path);
-			Ok(crate::sqlite_adapter::SqliteDb::new(manager, opt.fsync)?)
-		}
+		Engine::Sqlite => crate::sqlite_adapter::open_db(path, opt),
 
 		// ---- LMDB DB ----
 		#[cfg(feature = "lmdb")]
-		Engine::Lmdb => {
-			info!("Opening LMDB database at: {}", path.display());
-			if let Err(e) = std::fs::create_dir_all(&path) {
-				return Err(Error(
-					format!("Unable to create LMDB data directory: {}", e).into(),
-				));
-			}
-
-			let map_size = match opt.lmdb_map_size {
-				None => crate::lmdb_adapter::recommended_map_size(),
-				Some(v) => v - (v % 4096),
-			};
-
-			let mut env_builder = heed::EnvOpenOptions::new();
-			env_builder.max_dbs(100);
-			env_builder.map_size(map_size);
-			env_builder.max_readers(2048);
-			unsafe {
-				env_builder.flag(crate::lmdb_adapter::heed::flags::Flags::MdbNoRdAhead);
-				env_builder.flag(crate::lmdb_adapter::heed::flags::Flags::MdbNoMetaSync);
-				if !opt.fsync {
-					env_builder.flag(heed::flags::Flags::MdbNoSync);
-				}
-			}
-			match env_builder.open(&path) {
-				Err(heed::Error::Io(e)) if e.kind() == std::io::ErrorKind::OutOfMemory => {
-					return Err(Error(
-						"OutOfMemory error while trying to open LMDB database. This can happen \
-                        if your operating system is not allowing you to use sufficient virtual \
-                        memory address space. Please check that no limit is set (ulimit -v). \
-                        You may also try to set a smaller `lmdb_map_size` configuration parameter. \
-                        On 32-bit machines, you should probably switch to another database engine."
-							.into(),
-					))
-				}
-				Err(e) => Err(Error(format!("Cannot open LMDB database: {}", e).into())),
-				Ok(db) => Ok(crate::lmdb_adapter::LmdbDb::init(db)),
-			}
-		}
+		Engine::Lmdb => crate::lmdb_adapter::open_db(path, opt),
 
 		// ---- Fjall DB ----
 		#[cfg(feature = "fjall")]
-		Engine::Fjall => {
-			info!("Opening Fjall database at: {}", path.display());
-			if opt.fsync {
-				return Err(Error(
-					"metadata_fsync is not supported with the Fjall database engine".into(),
-				));
-			}
-			let mut config = fjall::Config::new(path);
-			if let Some(block_cache_size) = opt.fjall_block_cache_size {
-				config = config.cache_size(block_cache_size.try_into().unwrap());
-			}
-			let keyspace = config.open_transactional()?;
-			Ok(crate::fjall_adapter::FjallDb::init(keyspace))
-		}
+		Engine::Fjall => crate::fjall_adapter::open_db(path, opt),
 
 		// Pattern is unreachable when all supported DB engines are compiled into binary. The allow
 		// attribute is added so that we won't have to change this match in case stop building

src/db/sqlite_adapter.rs

@@ -11,12 +11,23 @@ use r2d2_sqlite::SqliteConnectionManager;
 use rusqlite::{params, Rows, Statement, Transaction};
 
 use crate::{
+	open::{Engine, OpenOpt},
 	Db, Error, IDb, ITx, ITxFn, OnCommit, Result, TxError, TxFnResult, TxOpError, TxOpResult,
 	TxResult, TxValueIter, Value, ValueIter,
 };
 
 pub use rusqlite;
 
+// ---- top-level open function
+
+pub(crate) fn open_db(path: &PathBuf, opt: &OpenOpt) -> Result<Db> {
+	info!("Opening Sqlite database at: {}", path.display());
+	let manager = r2d2_sqlite::SqliteConnectionManager::file(path);
+	Ok(SqliteDb::new(manager, opt.fsync)?)
+}
+
+// ----
+
 type Connection = r2d2::PooledConnection<SqliteConnectionManager>;
 
 // --- err
@@ -139,17 +150,19 @@ impl IDb for SqliteDb {
 		Ok(trees)
 	}
 
-	fn snapshot(&self, to: &PathBuf) -> Result<()> {
+	fn snapshot(&self, base_path: &PathBuf) -> Result<()> {
 		fn progress(p: rusqlite::backup::Progress) {
 			let percent = (p.pagecount - p.remaining) * 100 / p.pagecount;
 			info!("Sqlite snapshot progress: {}%", percent);
 		}
-		std::fs::create_dir_all(to)?;
-		let mut path = to.clone();
-		path.push("db.sqlite");
+
+		std::fs::create_dir_all(base_path)?;
+		let path = Engine::Sqlite.db_path(&base_path);
+
 		self.db
 			.get()?
 			.backup(rusqlite::DatabaseName::Main, path, Some(progress))?;
+
 		Ok(())
 	}
 

src/db/test.rs

@@ -155,7 +155,7 @@ fn test_fjall_db() {
 	use crate::fjall_adapter::{fjall, FjallDb};
 
 	let path = mktemp::Temp::new_dir().unwrap();
-	let config = fjall::Config::new(path);
+	let config = fjall::Config::new(path).temporary(true);
 	let keyspace = config.open_transactional().unwrap();
 	let db = FjallDb::init(keyspace);
 	test_suite(db);

src/model/garage.rs

@@ -116,18 +116,7 @@ impl Garage {
 		info!("Opening database...");
 		let db_engine = db::Engine::from_str(&config.db_engine)
 			.ok_or_message("Invalid `db_engine` value in configuration file")?;
-		let mut db_path = config.metadata_dir.clone();
-		match db_engine {
-			db::Engine::Sqlite => {
-				db_path.push("db.sqlite");
-			}
-			db::Engine::Lmdb => {
-				db_path.push("db.lmdb");
-			}
-			db::Engine::Fjall => {
-				db_path.push("db.fjall");
-			}
-		}
+		let db_path = db_engine.db_path(&config.metadata_dir);
 		let db_opt = db::OpenOpt {
 			fsync: config.metadata_fsync,
 			lmdb_map_size: match config.lmdb_map_size {