Repository: cberner/redb
Pull request #390 (2022-10-03 03:14:57)

Fix minor TODOs


Patch:
diff --git a/src/db.rs b/src/db.rs
index 4e2d03f..805a04b 100644
--- a/src/db.rs
+++ b/src/db.rs
@@ -421,9 +421,14 @@ impl Database {
 
     /// Changes the write strategy of the database for future [`WriteTransaction`]s.
     ///
+    /// Calling this method will invalidate all existing [`Savepoint`]s.
+    ///
     /// Note: Changing to the [`WriteStrategy::Checksum`] strategy can take a long time, as checksums
     /// will need to be calculated for every entry in the database
     pub fn set_write_strategy(&self, strategy: WriteStrategy) -> Result {
+        let mut valid_savepoints = self.valid_savepoints.lock().unwrap();
+        valid_savepoints.clear();
+
         let guard = self.live_write_transaction.lock().unwrap();
         assert!(guard.is_none());
         // TODO: implement switching to checksum strategy
@@ -436,6 +441,8 @@ impl Database {
             .commit(root_page, freed_root, id, false, Some(strategy.into()))?;
         drop(guard);
 
+        drop(valid_savepoints);
+
         Ok(())
     }
 
@@ -462,9 +469,6 @@ impl Database {
     }
 
     pub fn restore_savepoint(&self, savepoint: &Savepoint) -> Result {
-        // TODO: ensure that restoring a savepoint can't undo a database upgrade or change in WriteStrategy
-        // right now it could restore pages without checksums after the database had been changed to use
-        // WriteStrategy::Checksum, which could lead to data corruption
         let mut guard = self.valid_savepoints.lock().unwrap();
         if !guard.contains(&savepoint.get_id()) {
             return Err(Error::InvalidSavepoint);
diff --git a/src/transactions.rs b/src/transactions.rs
index b33d46f..28d0766 100644
--- a/src/transactions.rs
+++ b/src/transactions.rs
@@ -152,6 +152,14 @@ impl<'db> WriteTransaction<'db> {
         );
         *live_write_transaction = Some(transaction_id);
 
+        // Restoring a savepoint that reverted a file format or checksum type change could corrupt
+        // the database
+        assert_eq!(db.get_memory().get_version(), savepoint.get_version());
+        assert_eq!(
+            db.get_memory().checksum_type(),
+            savepoint.get_checksum_type()
+        );
+
         let freed_pages = db
             .get_memory()
             .pages_allocated_since_raw_state(savepoint.get_regional_allocator_states());
@@ -323,7 +331,6 @@ impl<'db> WriteTransaction<'db> {
     }
 
     /// List all the tables
-    // TODO: should return an iterator of &str, once GATs are available
     pub fn list_tables(&self) -> Result<impl Iterator<Item = String> + '_> {
         self.table_tree
             .borrow()
@@ -332,7 +339,6 @@ impl<'db> WriteTransaction<'db> {
     }
 
     /// List all the multimap tables
-    // TODO: should return an iterator of &str, once GATs are available
     pub fn list_multimap_tables(&self) -> Result<impl Iterator<Item = String> + '_> {
         self.table_tree
             .borrow()
diff --git a/src/tree_store/page_store/page_manager.rs b/src/tree_store/page_store/page_manager.rs
index b0d65c8..04f433e 100644
--- a/src/tree_store/page_store/page_manager.rs
+++ b/src/tree_store/page_store/page_manager.rs
@@ -1286,6 +1286,15 @@ impl TransactionalMemory {
         }
     }
 
+    pub(crate) fn get_version(&self) -> u8 {
+        let metadata = self.lock_metadata();
+        if self.read_from_secondary.load(Ordering::Acquire) {
+            metadata.secondary_slot().get_version()
+        } else {
+            metadata.primary_slot().get_version()
+        }
+    }
+
     pub(crate) fn get_data_root(&self) -> Option<(PageNumber, Checksum)> {
         let metadata = self.lock_metadata();
         if self.read_from_secondary.load(Ordering::Acquire) {
diff --git a/src/tree_store/page_store/savepoint.rs b/src/tree_store/page_store/savepoint.rs
index 91e7ec8..ee91bab 100644
--- a/src/tree_store/page_store/savepoint.rs
+++ b/src/tree_store/page_store/savepoint.rs
@@ -1,4 +1,5 @@
 use crate::db::TransactionId;
+use crate::tree_store::page_store::ChecksumType;
 use crate::tree_store::{Checksum, PageNumber};
 use crate::Database;
 
@@ -7,6 +8,8 @@ pub struct Savepoint<'a> {
     // Each savepoint has an associated read transaction id to ensure that any pages it references
     // are not freed
     id: TransactionId,
+    version: u8,
+    checksum_type: ChecksumType,
     root: Option<(PageNumber, Checksum)>,
     freed_root: Option<(PageNumber, Checksum)>,
     regional_allocators: Vec<Vec<u8>>,
@@ -23,12 +26,22 @@ impl<'a> Savepoint<'a> {
         Self {
             db,
             id,
+            version: db.get_memory().get_version(),
+            checksum_type: db.get_memory().checksum_type(),
             root,
             freed_root,
             regional_allocators,
         }
     }
 
+    pub(crate) fn get_version(&self) -> u8 {
+        self.version
+    }
+
+    pub(crate) fn get_checksum_type(&self) -> ChecksumType {
+        self.checksum_type
+    }
+
     pub(crate) fn get_id(&self) -> TransactionId {
         self.id
     }


Test patch:
diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs
index 87887da..c58cbe0 100644
--- a/tests/integration_tests.rs
+++ b/tests/integration_tests.rs
@@ -931,6 +931,25 @@ fn regression18() {
     drop(savepoint4);
 }
 
+#[test]
+fn change_invalidate_savepoint() {
+    let tmpfile: NamedTempFile = NamedTempFile::new().unwrap();
+
+    let db_size = 1024 * 1024;
+    let db = unsafe {
+        Database::builder()
+            .set_write_strategy(WriteStrategy::Checksum)
+            .create(tmpfile.path(), db_size)
+            .unwrap()
+    };
+    let savepoint = db.savepoint().unwrap();
+    db.set_write_strategy(WriteStrategy::TwoPhase).unwrap();
+    assert!(matches!(
+        db.restore_savepoint(&savepoint),
+        Err(Error::InvalidSavepoint)
+    ));
+}
+
 #[test]
 fn twophase_open() {
     let tmpfile: NamedTempFile = NamedTempFile::new().unwrap();