From b806322c5a8bcc55c5a821cf4757c1bab6f3ce86 Mon Sep 17 00:00:00 2001
From: Jason Aten <jason.aten@molecula.com>
Date: Sat, 1 Aug 2020 21:40:54 -0400
Subject: [PATCH] DetectMemAccessPastTx flag added, default false. Allow badger
 to run at full speed rather than with debugging code on by default

---
 badger.go    | 50 ++++++++++++++++++++++++++++----------------------
 txfactory.go | 15 ++++++++++++++-
 2 files changed, 42 insertions(+), 23 deletions(-)

diff --git a/badger.go b/badger.go
index bcd95f8da..ec4710f08 100644
--- a/badger.go
+++ b/badger.go
@@ -1588,29 +1588,35 @@ func (tx *BadgerTx) toContainer(typ byte, v []byte) (r *roaring.Container) {
 		return nil
 	}
 
-	// For safety we copy v, since it lives in BadgerDB's memory-mapped vlog-file,
-	// and Badger will recycle it after tx ends with rollback or commit.
-	// We copy into Go runtime GC managed memory. Technically we don't need
-	// to do this if all of our use stays within the lifetime
-	// of the badger transaction we were started on. Hence:
-	//
-	// TODO: performance tuning might want w := v here, if we can guarantee no access to memory past the Tx lifetime.
-	//
-	// Problem is, at least some tests appear to not respect transaction boundaries...
-	//
-	// Seebs suggested this nice variation: we could use individual mmaps for these
-	// copies, which would be unusable in production, but workable for testing, and then unmap them,
-	// which would get us probable segfaults on future accesses to them.
-	//
-	w := make([]byte, len(v))
-	copy(w, v)
-	// the copy above makes green: // green go test -v -run TestAPI_ImportColumnAttrs
-	//w := v // if instead of append we use v directly, it causes red: go test -v -run TestAPI_ImportColumnAttrs
+	var w []byte
+	if tx.doAllocZero {
+		// Do electric fence-inspired bad-memory read detection.
+		//
+		// The v []byte lives in BadgerDB's memory-mapped vlog-file,
+		// and Badger will recycle it after tx ends with rollback or commit.
+		//
+		// Problem is, at least some operations were not respecting transaction boundaries.
+		// This technique helped us find them. The rowCache was an example.
+		//
+		// See the global const DetectMemAccessPastTx
+		// at the top of txfactory.go to activate/deactivate this.
+		//
+		// Seebs suggested this nice variation: we could use individual mmaps for these
+		// copies, which would be unusable in production, but workable for testing, and then unmap them,
+		// which would get us probable segfaults on future accesses to them.
+		//
+		// The go runtime also has an -efence flag which may be similarly useful if really pressed.
+		//
+		w = make([]byte, len(v))
+		copy(w, v)
 
-	// register w so we can catch out-of-tx memory access
-	tx.acMu.Lock()
-	defer tx.acMu.Unlock()
-	tx.ourAllocs = append(tx.ourAllocs, w)
+		// register w so we can catch out-of-tx memory access
+		tx.acMu.Lock()
+		defer tx.acMu.Unlock()
+		tx.ourAllocs = append(tx.ourAllocs, w)
+	} else {
+		w = v
+	}
 
 	switch typ {
 	case containerArray:
diff --git a/txfactory.go b/txfactory.go
index de2e90c25..816ed59c8 100644
--- a/txfactory.go
+++ b/txfactory.go
@@ -49,6 +49,19 @@ const (
 // Can be overridden with env variable PILOSA_TXSRC for testing.
 const DefaultTxsrc = RoaringTxn
 
+// DetectMemAccessPastTx true helps us catch places in api and executor
+// where mmapped memory is being accessed after the point in time
+// which the transaction has committed or rolled back. Since
+// memory segments will be recycled by the underlying databases,
+// this can lead to corruption. When DetectMemAccessPastTx is true,
+// code in badger.go will copy the transactionally viewed memory before
+// returning it for bitmap reading, and then zero it or overwrite it
+// with -2 when the Tx completes.
+//
+// Should be false for production.
+//
+const DetectMemAccessPastTx = false
+
 var sep = string(os.PathSeparator)
 
 // TxFactory abstracts the creation of Tx interface-level
@@ -175,7 +188,7 @@ func NewTxFactory(txsrc string, dir, name string, openExisting bool) (f *TxFacto
 		}
 		// electric-fence like finding of access to mmapped data beyond
 		// transaction end time.
-		f.badgerDB.doAllocZero = true
+		f.badgerDB.doAllocZero = DetectMemAccessPastTx
 	}
 
 	switch ty {