From faa1662bf633737322b1153cdb206832a877cd18 Mon Sep 17 00:00:00 2001
From: Ben Johnson <benbjohnson@yahoo.com>
Date: Fri, 7 Aug 2020 10:07:31 -0600
Subject: [PATCH] Fix RBF checkpoint high water mark

Previously, the `checkpoint()` function determined the segments to drop
based on the current active transactions' WAL ID references. However, if
no transactions are active then the checkpoint would drop segments too
aggressively.

This changes the determination by using the highest WAL ID that is
actually checkpointed to disk to determine the high water mark. If no
page are checkpointed then no segments can be dropped.
---
 rbf/db.go | 30 ++++++++++++++++++++----------
 rbf/tx.go | 12 +-----------
 2 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/rbf/db.go b/rbf/db.go
index bec221c99..b6eeda6b5 100644
--- a/rbf/db.go
+++ b/rbf/db.go
@@ -187,6 +187,7 @@ func (db *DB) checkpoint() error {
 	// Loop over each transaction
 	walID++
 	pageMap := immutable.NewMap(&uint32Hasher{})
+	var maxCheckpointedWALID int64
 	for {
 		// Determine last page of transaction.
 		metaWALID, metaFlags, err := db.findNextWALMetaPage(walID)
@@ -240,20 +241,27 @@ func (db *DB) checkpoint() error {
 			if err := db.writePage(pgno, page); err != nil {
 				return err
 			}
+
+			// Track highest WALID that has been checkpointed back to disk.
+			if IsMetaPage(page) {
+				maxCheckpointedWALID = walID
+			}
 		}
 	}
 
 	// Remove WAL segments that have been checkpointed.
-	for len(db.segments) > 1 {
-		segment := db.segments[0]
-		if minActiveWALID != 0 && segment.MaxWALID() >= minActiveWALID {
-			break
-		}
+	if maxCheckpointedWALID != 0 {
+		for len(db.segments) > 1 {
+			segment := db.segments[0]
+			if segment.MaxWALID() >= maxCheckpointedWALID {
+				break
+			}
 
-		if err := segment.Close(); err != nil {
-			return err
+			if err := segment.Close(); err != nil {
+				return err
+			}
+			db.segments, db.segments[0] = db.segments[1:], nil
 		}
-		db.segments, db.segments[0] = db.segments[1:], nil
 	}
 
 	db.pageMap = pageMap
@@ -590,8 +598,10 @@ func (db *DB) removeTx(tx *Tx) error {
 
 	// Write pages from WAL to DB.
 	// TODO(bbj): Move this to an async goroutine.
-	if err := db.checkpoint(); err != nil {
-		return err
+	if tx.writable {
+		if err := db.checkpoint(); err != nil {
+			return err
+		}
 	}
 
 	delete(tx.db.txs, tx)
diff --git a/rbf/tx.go b/rbf/tx.go
index b4821bd98..6083493d7 100644
--- a/rbf/tx.go
+++ b/rbf/tx.go
@@ -106,18 +106,8 @@ func (tx *Tx) Rollback() {
 		}
 	}
 
-	// turn on these error checks! we see
-	// panic: cannot find segment containing WAL page: 1
-	// when running go test -v
-	// TestCursor_FirstNext_Quick/6
-	//
-	//panicOn(tx.db.checkpoint())
-	//panicOn(tx.db.removeTx(tx))
-
-	_ = tx.db.checkpoint()
-
 	// Disconnect transaction from DB.
-	_ = tx.db.removeTx(tx)
+	panicOn(tx.db.removeTx(tx))
 }
 
 // Root returns the root page number for a bitmap. Returns 0 if the bitmap does not exist.