From 2169684ebb1740050e9356b4b7ad73241618dd6c Mon Sep 17 00:00:00 2001
From: Christopher Berner <me@cberner.com>
Date: Thu, 25 Jul 2024 17:11:53 -0700
Subject: [PATCH] Improve freeing of pages

Previously pages freed in the oldest transaction that still had a read
reference were not freed
---
 src/transactions.rs        |  9 ++++---
 tests/integration_tests.rs | 50 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/src/transactions.rs b/src/transactions.rs
index 9c7de4d7..dd072737 100644
--- a/src/transactions.rs
+++ b/src/transactions.rs
@@ -946,9 +946,10 @@ impl WriteTransaction {
     }
 
     pub(crate) fn durable_commit(&mut self, eventual: bool, two_phase: bool) -> Result {
-        let oldest_live_read = self
+        let free_until_transaction = self
             .transaction_tracker
             .oldest_live_read_transaction()
+            .map(|x| x.next())
             .unwrap_or(self.transaction_id);
 
         let user_root = self
@@ -965,7 +966,7 @@ impl WriteTransaction {
             .table_tree
             .flush_table_root_updates()?;
 
-        self.process_freed_pages(oldest_live_read)?;
+        self.process_freed_pages(free_until_transaction)?;
         // If a savepoint exists it might reference the freed-tree, since it holds a reference to the
         // root of the freed-tree. Therefore, we must use the transactional free mechanism to free
         // those pages. If there are no save points then these can be immediately freed, which is
@@ -1055,11 +1056,11 @@ impl WriteTransaction {
 
     // NOTE: must be called before store_freed_pages() during commit, since this can create
     // more pages freed by the current transaction
-    fn process_freed_pages(&mut self, oldest_live_read: TransactionId) -> Result {
+    fn process_freed_pages(&mut self, free_until: TransactionId) -> Result {
         // We assume below that PageNumber is length 8
         assert_eq!(PageNumber::serialized_size(), 8);
         let lookup_key = FreedTableKey {
-            transaction_id: oldest_live_read.raw_id(),
+            transaction_id: free_until.raw_id(),
             pagination_id: 0,
         };
 
diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs
index 2680d013..aeca518c 100644
--- a/tests/integration_tests.rs
+++ b/tests/integration_tests.rs
@@ -952,6 +952,56 @@ fn regression21() {
     drop(read_tx);
 }
 
+#[test]
+fn regression22() {
+    let tmpfile = create_tempfile();
+
+    let db = Database::create(tmpfile.path()).unwrap();
+    let txn = db.begin_write().unwrap();
+    {
+        let mut table = txn.open_table(U64_TABLE).unwrap();
+        table.insert(0, 0).unwrap();
+    }
+    txn.commit().unwrap();
+
+    let txn = db.begin_write().unwrap();
+    {
+        let mut table = txn.open_table(U64_TABLE).unwrap();
+        table.remove(0).unwrap();
+    }
+    txn.commit().unwrap();
+
+    // Extra commit to finalize the cleanup of the freed pages
+    let txn = db.begin_write().unwrap();
+    txn.commit().unwrap();
+
+    let txn = db.begin_write().unwrap();
+    let allocated_pages = txn.stats().unwrap().allocated_pages();
+    {
+        let mut table = txn.open_table(U64_TABLE).unwrap();
+        table.insert(0, 0).unwrap();
+    }
+    txn.commit().unwrap();
+
+    let txn = db.begin_write().unwrap();
+    {
+        let mut table = txn.open_table(U64_TABLE).unwrap();
+        table.remove(0).unwrap();
+    }
+    txn.commit().unwrap();
+
+    let read_txn = db.begin_read().unwrap();
+
+    // Extra commit to finalize the cleanup of the freed pages. The read transaction should not
+    // block the freeing, but there was a bug where it did.
+    db.begin_write().unwrap().commit().unwrap();
+
+    drop(read_txn);
+
+    let txn = db.begin_write().unwrap();
+    assert_eq!(allocated_pages, txn.stats().unwrap().allocated_pages());
+}
+
 #[test]
 fn check_integrity_clean() {
     let tmpfile = create_tempfile();