From 207a9e92f780b165b524d76a613ec3d406001f30 Mon Sep 17 00:00:00 2001 From: Christopher Berner Date: Sat, 3 Feb 2024 09:57:21 -0800 Subject: [PATCH] Remove lifetime from WriteTransaction --- benches/common.rs | 16 ++++---- examples/special_values.rs | 14 +++---- fuzz/fuzz_targets/fuzz_redb.rs | 6 +-- src/db.rs | 10 ++--- src/multimap_table.rs | 24 +++++------ src/table.rs | 24 +++++------ src/transactions.rs | 73 ++++++++++++++++++++-------------- tests/basic_tests.rs | 1 - 8 files changed, 86 insertions(+), 82 deletions(-) diff --git a/benches/common.rs b/benches/common.rs index 35153c33..c6763e69 100644 --- a/benches/common.rs +++ b/benches/common.rs @@ -85,7 +85,7 @@ impl<'a> RedbBenchDatabase<'a> { } impl<'a> BenchDatabase for RedbBenchDatabase<'a> { - type W<'db> = RedbBenchWriteTransaction<'db> where Self: 'db; + type W<'db> = RedbBenchWriteTransaction where Self: 'db; type R<'db> = RedbBenchReadTransaction<'db> where Self: 'db; fn db_type_name() -> &'static str { @@ -165,12 +165,12 @@ impl<'a> AsRef<[u8]> for RedbAccessGuard<'a> { } } -pub struct RedbBenchWriteTransaction<'db> { - txn: redb::WriteTransaction<'db>, +pub struct RedbBenchWriteTransaction { + txn: redb::WriteTransaction, } -impl<'db> BenchWriteTransaction for RedbBenchWriteTransaction<'db> { - type W<'txn> = RedbBenchInserter<'db, 'txn> where Self: 'txn; +impl BenchWriteTransaction for RedbBenchWriteTransaction { + type W<'txn> = RedbBenchInserter<'txn> where Self: 'txn; fn get_inserter(&mut self) -> Self::W<'_> { let table = self.txn.open_table(X).unwrap(); @@ -182,11 +182,11 @@ impl<'db> BenchWriteTransaction for RedbBenchWriteTransaction<'db> { } } -pub struct RedbBenchInserter<'db, 'txn> { - table: redb::Table<'db, 'txn, &'static [u8], &'static [u8]>, +pub struct RedbBenchInserter<'txn> { + table: redb::Table<'txn, &'static [u8], &'static [u8]>, } -impl BenchInserter for RedbBenchInserter<'_, '_> { +impl BenchInserter for RedbBenchInserter<'_> { fn insert(&mut self, key: &[u8], value: &[u8]) -> Result<(), ()> { self.table.insert(key, value).map(|_| ()).map_err(|_| ()) } diff --git a/examples/special_values.rs b/examples/special_values.rs index b19d5655..fecbe7e5 100644 --- a/examples/special_values.rs +++ b/examples/special_values.rs @@ -35,15 +35,15 @@ impl SpecialValuesDb { } struct SpecialValuesTransaction<'db> { - inner: WriteTransaction<'db>, + inner: WriteTransaction, file: &'db mut File, } impl<'db> SpecialValuesTransaction<'db> { - fn open_table<'txn, K: RedbKey + 'static, V: RedbValue + 'static>( - &'txn mut self, + fn open_table( + &mut self, table: TableDefinition, - ) -> SpecialValuesTable<'db, 'txn, K, V> { + ) -> SpecialValuesTable { let def: TableDefinition = TableDefinition::new(table.name()); SpecialValuesTable { inner: self.inner.open_table(def).unwrap(), @@ -58,13 +58,13 @@ impl<'db> SpecialValuesTransaction<'db> { } } -struct SpecialValuesTable<'db, 'txn, K: RedbKey + 'static, V: RedbValue + 'static> { - inner: Table<'db, 'txn, K, (u64, u64)>, +struct SpecialValuesTable<'txn, K: RedbKey + 'static, V: RedbValue + 'static> { + inner: Table<'txn, K, (u64, u64)>, file: &'txn mut File, _value_type: PhantomData, } -impl<'db, 'txn, K: RedbKey + 'static, V: RedbValue + 'static> SpecialValuesTable<'db, 'txn, K, V> { +impl<'txn, K: RedbKey + 'static, V: RedbValue + 'static> SpecialValuesTable<'txn, K, V> { fn insert(&mut self, key: K::SelfType<'_>, value: V::SelfType<'_>) { // Append to end of file let offset = self.file.seek(SeekFrom::End(0)).unwrap(); diff --git a/fuzz/fuzz_targets/fuzz_redb.rs b/fuzz/fuzz_targets/fuzz_redb.rs index aaf285ce..e9988007 100644 --- a/fuzz/fuzz_targets/fuzz_redb.rs +++ b/fuzz/fuzz_targets/fuzz_redb.rs @@ -470,7 +470,7 @@ fn is_simulated_io_error(err: &redb::Error) -> bool { } } -fn exec_table_crash_support(config: &FuzzConfig, apply: fn(WriteTransaction<'_>, &mut BTreeMap, &FuzzTransaction, &mut SavepointManager) -> Result<(), redb::Error>) -> Result<(), redb::Error> { +fn exec_table_crash_support(config: &FuzzConfig, apply: fn(WriteTransaction, &mut BTreeMap, &FuzzTransaction, &mut SavepointManager) -> Result<(), redb::Error>) -> Result<(), redb::Error> { let mut redb_file: NamedTempFile = NamedTempFile::new().unwrap(); let backend = FuzzerBackend::new(FileBackend::new(redb_file.as_file().try_clone().unwrap())?); let countdown = backend.countdown.clone(); @@ -642,7 +642,7 @@ fn handle_savepoints(mut txn: WriteTransaction, reference: &mut BTreeM } -fn apply_crashable_transaction_multimap(txn: WriteTransaction<'_>, uncommitted_reference: &mut BTreeMap>, transaction: &FuzzTransaction, savepoints: &mut SavepointManager>) -> Result<(), redb::Error> { +fn apply_crashable_transaction_multimap(txn: WriteTransaction, uncommitted_reference: &mut BTreeMap>, transaction: &FuzzTransaction, savepoints: &mut SavepointManager>) -> Result<(), redb::Error> { { let mut table = txn.open_multimap_table(MULTIMAP_TABLE_DEF)?; for op in transaction.ops.iter() { @@ -662,7 +662,7 @@ fn apply_crashable_transaction_multimap(txn: WriteTransaction<'_>, uncommitted_r Ok(()) } -fn apply_crashable_transaction(txn: WriteTransaction<'_>, uncommitted_reference: &mut BTreeMap, transaction: &FuzzTransaction, savepoints: &mut SavepointManager) -> Result<(), redb::Error> { +fn apply_crashable_transaction(txn: WriteTransaction, uncommitted_reference: &mut BTreeMap, transaction: &FuzzTransaction, savepoints: &mut SavepointManager) -> Result<(), redb::Error> { { let mut table = txn.open_table(TABLE_DEF)?; for op in transaction.ops.iter() { diff --git a/src/db.rs b/src/db.rs index ab70a1da..8761f7d3 100644 --- a/src/db.rs +++ b/src/db.rs @@ -255,7 +255,7 @@ impl TransactionGuard { self.transaction_id.unwrap() } - fn leak(mut self) -> TransactionId { + pub(crate) fn leak(mut self) -> TransactionId { self.transaction_id.take().unwrap() } } @@ -768,11 +768,6 @@ impl Database { )) } - pub(crate) fn allocate_savepoint(&self) -> Result<(SavepointId, TransactionId)> { - let id = self.transaction_tracker.allocate_savepoint(); - Ok((id, self.allocate_read_transaction()?.leak())) - } - /// Convenience method for [`Builder::new`] pub fn builder() -> Builder { Builder::new() @@ -788,7 +783,8 @@ impl Database { self.transaction_tracker.start_write_transaction(), self.transaction_tracker.clone(), ); - WriteTransaction::new(self, guard, self.transaction_tracker.clone()).map_err(|e| e.into()) + WriteTransaction::new(guard, self.transaction_tracker.clone(), self.mem.clone()) + .map_err(|e| e.into()) } /// Begins a read transaction diff --git a/src/multimap_table.rs b/src/multimap_table.rs index cf62121e..fca55ed0 100644 --- a/src/multimap_table.rs +++ b/src/multimap_table.rs @@ -740,31 +740,29 @@ impl<'a, K: RedbKey + 'static, V: RedbKey + 'static> DoubleEndedIterator /// A multimap table /// /// [Multimap tables](https://en.wikipedia.org/wiki/Multimap) may have multiple values associated with each key -pub struct MultimapTable<'db, 'txn, K: RedbKey + 'static, V: RedbKey + 'static> { +pub struct MultimapTable<'txn, K: RedbKey + 'static, V: RedbKey + 'static> { name: String, - transaction: &'txn WriteTransaction<'db>, + transaction: &'txn WriteTransaction, freed_pages: Arc>>, tree: BtreeMut<'txn, K, &'static DynamicCollection>, mem: Arc, _value_type: PhantomData, } -impl MultimapTableHandle - for MultimapTable<'_, '_, K, V> -{ +impl MultimapTableHandle for MultimapTable<'_, K, V> { fn name(&self) -> &str { &self.name } } -impl<'db, 'txn, K: RedbKey + 'static, V: RedbKey + 'static> MultimapTable<'db, 'txn, K, V> { +impl<'txn, K: RedbKey + 'static, V: RedbKey + 'static> MultimapTable<'txn, K, V> { pub(crate) fn new( name: &str, table_root: Option<(PageNumber, Checksum)>, freed_pages: Arc>>, mem: Arc, - transaction: &'txn WriteTransaction<'db>, - ) -> MultimapTable<'db, 'txn, K, V> { + transaction: &'txn WriteTransaction, + ) -> MultimapTable<'txn, K, V> { MultimapTable { name: name.to_string(), transaction, @@ -1115,8 +1113,8 @@ impl<'db, 'txn, K: RedbKey + 'static, V: RedbKey + 'static> MultimapTable<'db, ' } } -impl<'db, 'txn, K: RedbKey + 'static, V: RedbKey + 'static> ReadableMultimapTable - for MultimapTable<'db, 'txn, K, V> +impl<'txn, K: RedbKey + 'static, V: RedbKey + 'static> ReadableMultimapTable + for MultimapTable<'txn, K, V> { /// Returns an iterator over all values for the given key. Values are in ascending order. fn get<'a>(&self, key: impl Borrow>) -> Result> @@ -1187,11 +1185,9 @@ impl<'db, 'txn, K: RedbKey + 'static, V: RedbKey + 'static> ReadableMultimapTabl } } -impl Sealed for MultimapTable<'_, '_, K, V> {} +impl Sealed for MultimapTable<'_, K, V> {} -impl<'db, 'txn, K: RedbKey + 'static, V: RedbKey + 'static> Drop - for MultimapTable<'db, 'txn, K, V> -{ +impl<'txn, K: RedbKey + 'static, V: RedbKey + 'static> Drop for MultimapTable<'txn, K, V> { fn drop(&mut self) { self.transaction.close_table(&self.name, &self.tree); } diff --git a/src/table.rs b/src/table.rs index 0046f1ec..bf7e2134 100644 --- a/src/table.rs +++ b/src/table.rs @@ -58,26 +58,26 @@ impl TableStats { } /// A table containing key-value mappings -pub struct Table<'db, 'txn, K: RedbKey + 'static, V: RedbValue + 'static> { +pub struct Table<'txn, K: RedbKey + 'static, V: RedbValue + 'static> { name: String, - transaction: &'txn WriteTransaction<'db>, + transaction: &'txn WriteTransaction, tree: BtreeMut<'txn, K, V>, } -impl TableHandle for Table<'_, '_, K, V> { +impl TableHandle for Table<'_, K, V> { fn name(&self) -> &str { &self.name } } -impl<'db, 'txn, K: RedbKey + 'static, V: RedbValue + 'static> Table<'db, 'txn, K, V> { +impl<'txn, K: RedbKey + 'static, V: RedbValue + 'static> Table<'txn, K, V> { pub(crate) fn new( name: &str, table_root: Option<(PageNumber, Checksum)>, freed_pages: Arc>>, mem: Arc, - transaction: &'txn WriteTransaction<'db>, - ) -> Table<'db, 'txn, K, V> { + transaction: &'txn WriteTransaction, + ) -> Table<'txn, K, V> { Table { name: name.to_string(), transaction, @@ -193,7 +193,7 @@ impl<'db, 'txn, K: RedbKey + 'static, V: RedbValue + 'static> Table<'db, 'txn, K } } -impl<'db, 'txn, K: RedbKey + 'static, V: MutInPlaceValue + 'static> Table<'db, 'txn, K, V> { +impl<'txn, K: RedbKey + 'static, V: MutInPlaceValue + 'static> Table<'txn, K, V> { /// Reserve space to insert a key-value pair /// The returned reference will have length equal to value_length pub fn insert_reserve<'a>( @@ -215,9 +215,7 @@ impl<'db, 'txn, K: RedbKey + 'static, V: MutInPlaceValue + 'static> Table<'db, ' } } -impl<'db, 'txn, K: RedbKey + 'static, V: RedbValue + 'static> ReadableTable - for Table<'db, 'txn, K, V> -{ +impl<'txn, K: RedbKey + 'static, V: RedbValue + 'static> ReadableTable for Table<'txn, K, V> { fn get<'a>(&self, key: impl Borrow>) -> Result>> where K: 'a, @@ -257,9 +255,9 @@ impl<'db, 'txn, K: RedbKey + 'static, V: RedbValue + 'static> ReadableTable Sealed for Table<'_, '_, K, V> {} +impl Sealed for Table<'_, K, V> {} -impl<'db, 'txn, K: RedbKey + 'static, V: RedbValue + 'static> Drop for Table<'db, 'txn, K, V> { +impl<'txn, K: RedbKey + 'static, V: RedbValue + 'static> Drop for Table<'txn, K, V> { fn drop(&mut self) { self.transaction.close_table(&self.name, &self.tree); } @@ -308,7 +306,7 @@ fn debug_helper( Ok(()) } -impl Debug for Table<'_, '_, K, V> { +impl Debug for Table<'_, K, V> { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { debug_helper(f, &self.name, self.len(), self.first(), self.last()) } diff --git a/src/transactions.rs b/src/transactions.rs index 29c2aef7..072d701b 100644 --- a/src/transactions.rs +++ b/src/transactions.rs @@ -11,7 +11,7 @@ use crate::tree_store::{ }; use crate::types::{RedbKey, RedbValue}; use crate::{ - AccessGuard, Database, MultimapTable, MultimapTableDefinition, MultimapTableHandle, Range, + AccessGuard, MultimapTable, MultimapTableDefinition, MultimapTableHandle, Range, ReadOnlyMultimapTable, ReadOnlyTable, Result, Savepoint, SavepointError, StorageError, Table, TableDefinition, TableError, TableHandle, TransactionError, UntypedMultimapTableHandle, UntypedTableHandle, @@ -274,7 +274,7 @@ struct SystemNamespace<'db> { impl<'db> SystemNamespace<'db> { fn open_system_table<'txn, 's, K: RedbKey + 'static, V: RedbValue + 'static>( &'s mut self, - transaction: &'txn WriteTransaction<'db>, + transaction: &'txn WriteTransaction, definition: SystemTableDefinition, ) -> Result> { #[cfg(feature = "logging")] @@ -335,9 +335,9 @@ impl<'db> TableNamespace<'db> { #[track_caller] pub fn open_multimap_table<'txn, K: RedbKey + 'static, V: RedbKey + 'static>( &mut self, - transaction: &'txn WriteTransaction<'db>, + transaction: &'txn WriteTransaction, definition: MultimapTableDefinition, - ) -> Result, TableError> { + ) -> Result, TableError> { #[cfg(feature = "logging")] info!("Opening multimap table: {}", definition); let root = self.inner_open::(definition.name(), TableType::Multimap)?; @@ -355,9 +355,9 @@ impl<'db> TableNamespace<'db> { #[track_caller] pub fn open_table<'txn, K: RedbKey + 'static, V: RedbValue + 'static>( &mut self, - transaction: &'txn WriteTransaction<'db>, + transaction: &'txn WriteTransaction, definition: TableDefinition, - ) -> Result, TableError> { + ) -> Result, TableError> { #[cfg(feature = "logging")] info!("Opening table: {}", definition); let root = self.inner_open::(definition.name(), TableType::Normal)?; @@ -384,7 +384,7 @@ impl<'db> TableNamespace<'db> { #[track_caller] fn delete_table<'txn>( &mut self, - transaction: &'txn WriteTransaction<'db>, + transaction: &'txn WriteTransaction, name: &str, ) -> Result { #[cfg(feature = "logging")] @@ -396,7 +396,7 @@ impl<'db> TableNamespace<'db> { #[track_caller] fn delete_multimap_table<'txn>( &mut self, - transaction: &'txn WriteTransaction<'db>, + transaction: &'txn WriteTransaction, name: &str, ) -> Result { #[cfg(feature = "logging")] @@ -419,21 +419,20 @@ impl<'db> TableNamespace<'db> { /// A read/write transaction /// /// Only a single [`WriteTransaction`] may exist at a time -pub struct WriteTransaction<'db> { - db: &'db Database, +pub struct WriteTransaction { transaction_tracker: Arc, mem: Arc, transaction_guard: Arc, transaction_id: TransactionId, // The table of freed pages by transaction. FreedTableKey -> binary. // The binary blob is a length-prefixed array of PageNumber - freed_tree: Mutex>>, + freed_tree: Mutex>>, freed_pages: Arc>>, // Pages that were freed from the freed-tree. These can be freed immediately after commit(), // since read transactions do not access the freed-tree post_commit_frees: Arc>>, - tables: Mutex>, - system_tables: Mutex>, + tables: Mutex>, + system_tables: Mutex>, completed: bool, dirty: AtomicBool, durability: Durability, @@ -442,18 +441,18 @@ pub struct WriteTransaction<'db> { deleted_persistent_savepoints: Mutex>, } -impl<'db> WriteTransaction<'db> { +impl WriteTransaction { pub(crate) fn new( - db: &'db Database, guard: TransactionGuard, transaction_tracker: Arc, + mem: Arc, ) -> Result { let transaction_id = guard.id(); let guard = Arc::new(guard); - let root_page = db.get_memory().get_data_root(); - let system_page = db.get_memory().get_system_root(); - let freed_root = db.get_memory().get_freed_root(); + let root_page = mem.get_data_root(); + let system_page = mem.get_system_root(); + let freed_root = mem.get_freed_root(); let freed_pages = Arc::new(Mutex::new(vec![])); let post_commit_frees = Arc::new(Mutex::new(vec![])); @@ -462,7 +461,7 @@ impl<'db> WriteTransaction<'db> { table_tree: TableTreeMut::new( root_page, guard.clone(), - db.get_memory(), + mem.clone(), freed_pages.clone(), ), }; @@ -470,16 +469,15 @@ impl<'db> WriteTransaction<'db> { table_tree: TableTreeMut::new( system_page, guard.clone(), - db.get_memory(), + mem.clone(), freed_pages.clone(), ), transaction_guard: guard.clone(), }; Ok(Self { - db, transaction_tracker, - mem: db.get_memory(), + mem: mem.clone(), transaction_guard: guard.clone(), transaction_id, tables: Mutex::new(tables), @@ -487,7 +485,7 @@ impl<'db> WriteTransaction<'db> { freed_tree: Mutex::new(BtreeMut::new( freed_root, guard, - db.get_memory(), + mem, post_commit_frees.clone(), )), freed_pages, @@ -607,6 +605,23 @@ impl<'db> WriteTransaction<'db> { Ok(savepoints.into_iter()) } + // TODO: deduplicate this with the one in Database + fn allocate_read_transaction(&self) -> Result { + let id = self + .transaction_tracker + .register_read_transaction(&self.mem)?; + + Ok(TransactionGuard::new_read( + id, + self.transaction_tracker.clone(), + )) + } + + fn allocate_savepoint(&self) -> Result<(SavepointId, TransactionId)> { + let id = self.transaction_tracker.allocate_savepoint(); + Ok((id, self.allocate_read_transaction()?.leak())) + } + /// Creates a snapshot of the current database state, which can be used to rollback the database /// /// This savepoint will be freed as soon as the returned `[Savepoint]` is dropped. @@ -617,7 +632,7 @@ impl<'db> WriteTransaction<'db> { return Err(SavepointError::InvalidSavepoint); } - let (id, transaction_id) = self.db.allocate_savepoint()?; + let (id, transaction_id) = self.allocate_savepoint()?; #[cfg(feature = "logging")] info!( "Creating savepoint id={:?}, txn_id={:?}", @@ -629,7 +644,7 @@ impl<'db> WriteTransaction<'db> { let system_root = self.mem.get_system_root(); let freed_root = self.mem.get_freed_root(); let savepoint = Savepoint::new_ephemeral( - &self.db.get_memory(), + &self.mem, self.transaction_tracker.clone(), id, transaction_id, @@ -666,7 +681,7 @@ impl<'db> WriteTransaction<'db> { ); // Restoring a savepoint that reverted a file format or checksum type change could corrupt // the database - assert_eq!(self.db.get_memory().get_version(), savepoint.get_version()); + assert_eq!(self.mem.get_version(), savepoint.get_version()); self.dirty.store(true, Ordering::Release); let allocated_since_savepoint = self @@ -774,7 +789,7 @@ impl<'db> WriteTransaction<'db> { pub fn open_table<'txn, K: RedbKey + 'static, V: RedbValue + 'static>( &'txn self, definition: TableDefinition, - ) -> Result, TableError> { + ) -> Result, TableError> { self.tables.lock().unwrap().open_table(self, definition) } @@ -785,7 +800,7 @@ impl<'db> WriteTransaction<'db> { pub fn open_multimap_table<'txn, K: RedbKey + 'static, V: RedbKey + 'static>( &'txn self, definition: MultimapTableDefinition, - ) -> Result, TableError> { + ) -> Result, TableError> { self.tables .lock() .unwrap() @@ -1150,7 +1165,7 @@ impl<'db> WriteTransaction<'db> { } } -impl<'a> Drop for WriteTransaction<'a> { +impl Drop for WriteTransaction { fn drop(&mut self) { if !self.completed && !thread::panicking() && !self.mem.storage_failure() { #[allow(unused_variables)] diff --git a/tests/basic_tests.rs b/tests/basic_tests.rs index b440c15e..2908b514 100644 --- a/tests/basic_tests.rs +++ b/tests/basic_tests.rs @@ -1507,7 +1507,6 @@ fn concurrent_write_transactions_block() { let (sender, receiver) = sync::mpsc::channel(); let t = { - let db = db.clone(); std::thread::spawn(move || { sender.send(()).unwrap(); db.begin_write().unwrap().commit().unwrap();