From bd8ea269fa56a66a0c0879d7a4e8562cb807ab1b Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Sun, 27 Aug 2023 19:53:28 +0200 Subject: [PATCH 1/4] Make current nightly Clippy happy. --- Cargo.toml | 2 +- src/db.rs | 2 +- src/multimap_table.rs | 2 +- src/table.rs | 5 ++--- src/transactions.rs | 2 +- src/tree_store/page_store/page_manager.rs | 8 ++++---- src/tree_store/page_store/region.rs | 2 +- 7 files changed, 11 insertions(+), 12 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index bbbc7f88..2e7f3854 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,7 +32,7 @@ redb1 = { version = "=1.0.0", package = "redb" } # Just benchmarking dependencies [target.'cfg(not(target_os = "wasi"))'.dev-dependencies] ctrlc = "3.2.3" -fastrand = "1.9.0" +fastrand = "2.0.0" lmdb-rkv = "0.14.0" sanakirja = "1.3.3" sled = "0.34.7" diff --git a/src/db.rs b/src/db.rs index 65e08fa1..a78ca9cd 100644 --- a/src/db.rs +++ b/src/db.rs @@ -368,7 +368,7 @@ impl Database { fn mark_persistent_savepoints( system_root: Option<(PageNumber, Checksum)>, - mem: &mut TransactionalMemory, + mem: &TransactionalMemory, oldest_unprocessed_free_transaction: TransactionId, ) -> Result { let freed_list = Arc::new(Mutex::new(vec![])); diff --git a/src/multimap_table.rs b/src/multimap_table.rs index 8a69e02f..4e94c49b 100644 --- a/src/multimap_table.rs +++ b/src/multimap_table.rs @@ -948,7 +948,7 @@ impl<'db, 'txn, K: RedbKey + 'static, V: RedbKey + 'static> Drop { fn drop(&mut self) { self.transaction - .close_table(&self.name, self.system, &mut self.tree); + .close_table(&self.name, self.system, &self.tree); } } diff --git a/src/table.rs b/src/table.rs index 6b2453d0..060f0d1d 100644 --- a/src/table.rs +++ b/src/table.rs @@ -63,8 +63,7 @@ impl<'db, 'txn, K: RedbKey + 'static, V: RedbValue + 'static> Table<'db, 'txn, K // TODO: optimize this let last = self .iter()? - .rev() - .next() + .next_back() .map(|x| x.map(|(key, _)| K::as_bytes(&key.value()).as_ref().to_vec())); if let Some(owned_key) = last { let owned_key = owned_key?; @@ -189,7 +188,7 @@ impl Sealed for Table<'_, '_, K, V> {} impl<'db, 'txn, K: RedbKey + 'static, V: RedbValue + 'static> Drop for Table<'db, 'txn, K, V> { fn drop(&mut self) { self.transaction - .close_table(&self.name, self.system, &mut self.tree); + .close_table(&self.name, self.system, &self.tree); } } diff --git a/src/transactions.rs b/src/transactions.rs index bce92eea..dc9cf4fa 100644 --- a/src/transactions.rs +++ b/src/transactions.rs @@ -644,7 +644,7 @@ impl<'db> WriteTransaction<'db> { &self, name: &str, system: bool, - table: &mut BtreeMut, + table: &BtreeMut, ) { if system { self.open_system_tables diff --git a/src/tree_store/page_store/page_manager.rs b/src/tree_store/page_store/page_manager.rs index e34929a1..9f218712 100644 --- a/src/tree_store/page_store/page_manager.rs +++ b/src/tree_store/page_store/page_manager.rs @@ -110,7 +110,7 @@ impl TransactionalMemory { let region_size = min(region_size, (MAX_PAGE_INDEX as u64 + 1) * page_size as u64); assert!(region_size.is_power_of_two()); - let mut storage = PagedCachedFile::new( + let storage = PagedCachedFile::new( file, page_size as u64, read_cache_size_bytes, @@ -178,7 +178,7 @@ impl TransactionalMemory { .write(0, DB_HEADER_SIZE, true, |_| CachePriority::High)? .mem_mut() .copy_from_slice(&header.to_bytes(false, false)); - allocators.flush_to(tracker_page, layout, &mut storage)?; + allocators.flush_to(tracker_page, layout, &storage)?; storage.flush()?; // Write the magic number only after the data structure is initialized and written to disk @@ -406,7 +406,7 @@ impl TransactionalMemory { state .allocators - .flush_to(tracker_page, state.header.layout(), &mut self.storage)?; + .flush_to(tracker_page, state.header.layout(), &self.storage)?; state.header.recovery_required = false; self.write_header(&state.header, false)?; @@ -1083,7 +1083,7 @@ impl Drop for TransactionalMemory { .flush_to( state.header.region_tracker(), state.header.layout(), - &mut self.storage, + &self.storage, ) .is_err() { diff --git a/src/tree_store/page_store/region.rs b/src/tree_store/page_store/region.rs index 780d2f25..32c3f99c 100644 --- a/src/tree_store/page_store/region.rs +++ b/src/tree_store/page_store/region.rs @@ -177,7 +177,7 @@ impl Allocators { &self, region_tracker_page: PageNumber, layout: DatabaseLayout, - storage: &mut PagedCachedFile, + storage: &PagedCachedFile, ) -> Result { let page_size = layout.full_region_layout().page_size(); let region_header_size = From ebd04a0995a846be02d8a12b8de49f93826129ca Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Sun, 27 Aug 2023 19:53:34 +0200 Subject: [PATCH 2/4] Allow opening a database using a file handle instead of a path This enables using this library with e.g. `cap-std` which avoids path-based access in favour of a capability-based approach. --- src/db.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/db.rs b/src/db.rs index a78ca9cd..62ba159d 100644 --- a/src/db.rs +++ b/src/db.rs @@ -784,6 +784,19 @@ impl Builder { Err(StorageError::Io(io::Error::from(ErrorKind::InvalidData)).into()) } } + + /// Open an existing or create a new database in the given `file`. + /// + /// The file must be empty or contain a valid database. + pub fn create_file(&self, file: File) -> Result { + Database::new( + file, + self.page_size, + self.region_size, + self.read_cache_size_bytes, + self.write_cache_size_bytes, + ) + } } // This just makes it easier to throw `dbg` etc statements on `Result` @@ -1082,4 +1095,13 @@ mod test { let final_file_size = tmpfile.as_file().metadata().unwrap().len(); assert!(final_file_size < file_size); } + + #[test] + fn create_new_db_in_empty_file() { + let tmpfile = crate::create_tempfile(); + + let _db = Database::builder() + .create_file(tmpfile.into_file()) + .unwrap(); + } } From 2e766d32c9483ebe485d9ad93afe9e19e0eb9ae3 Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Sun, 3 Sep 2023 19:54:08 +0200 Subject: [PATCH 3/4] Add tests for opening missing/empty file by path before refactoring that method. --- src/db.rs | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/src/db.rs b/src/db.rs index 62ba159d..ea36c75a 100644 --- a/src/db.rs +++ b/src/db.rs @@ -808,7 +808,10 @@ impl std::fmt::Debug for Database { #[cfg(test)] mod test { - use crate::{Database, Durability, ReadableTable, TableDefinition}; + use crate::{ + Database, DatabaseError, Durability, ReadableTable, StorageError, TableDefinition, + }; + use std::io::ErrorKind; #[test] fn small_pages() { @@ -1104,4 +1107,31 @@ mod test { .create_file(tmpfile.into_file()) .unwrap(); } + + #[test] + fn open_missing_file() { + let tmpfile = crate::create_tempfile(); + + let err = Database::builder() + .open(tmpfile.path().with_extension("missing")) + .unwrap_err(); + + match err { + DatabaseError::Storage(StorageError::Io(err)) if err.kind() == ErrorKind::NotFound => {} + err => panic!("Unexpected error for empty file: {}", err), + } + } + + #[test] + fn open_empty_file() { + let tmpfile = crate::create_tempfile(); + + let err = Database::builder().open(tmpfile.path()).unwrap_err(); + + match err { + DatabaseError::Storage(StorageError::Io(err)) + if err.kind() == ErrorKind::InvalidData => {} + err => panic!("Unexpected error for empty file: {}", err), + } + } } From 1f8ff8d6e236e9909eab92935ef48914e8206264 Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Sun, 3 Sep 2023 19:55:11 +0200 Subject: [PATCH 4/4] Refactor DatabaseBuilder::open to perform fewer system calls. --- src/db.rs | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/src/db.rs b/src/db.rs index ea36c75a..e0a20cd7 100644 --- a/src/db.rs +++ b/src/db.rs @@ -12,7 +12,6 @@ use crate::{ use crate::{ReadTransaction, Result, WriteTransaction}; use std::fmt::{Display, Formatter}; use std::fs::{File, OpenOptions}; -use std::io; use std::io::ErrorKind; use std::marker::PhantomData; use std::ops::RangeFull; @@ -769,20 +768,19 @@ impl Builder { /// Opens an existing redb database. pub fn open(&self, path: impl AsRef) -> Result { - if !path.as_ref().exists() { - Err(StorageError::Io(ErrorKind::NotFound.into()).into()) - } else if File::open(path.as_ref())?.metadata()?.len() > 0 { - let file = OpenOptions::new().read(true).write(true).open(path)?; - Database::new( - file, - self.page_size, - None, - self.read_cache_size_bytes, - self.write_cache_size_bytes, - ) - } else { - Err(StorageError::Io(io::Error::from(ErrorKind::InvalidData)).into()) + let file = OpenOptions::new().read(true).write(true).open(path)?; + + if file.metadata()?.len() == 0 { + return Err(StorageError::Io(ErrorKind::InvalidData.into()).into()); } + + Database::new( + file, + self.page_size, + None, + self.read_cache_size_bytes, + self.write_cache_size_bytes, + ) } /// Open an existing or create a new database in the given `file`.