From ca216a14c539abfacbd7ec3e0c239f025336c69e Mon Sep 17 00:00:00 2001
From: Seebs <seebs@seebs.net>
Date: Mon, 29 Mar 2021 12:22:22 -0500
Subject: [PATCH 1/5] fix bitmap.BitwiseEqual bugs

bitmap.BitwiseEqual had a couple of subtle bugs, and the net result
is that if the bitmap you were comparing to had an empty container after
the original bitmap ran out of containers, we'd spuriously report
the container as existing and being... the last container in the original,
actually.

Issues are both that we were grabbing the value from the wrong iterator,
and also that we were iterating twice per loop, and thus could also
have missed a non-empty container immediately following an empty one.
---
 roaring/roaring.go | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/roaring/roaring.go b/roaring/roaring.go
index 1ee979bde..73192ced6 100644
--- a/roaring/roaring.go
+++ b/roaring/roaring.go
@@ -6515,16 +6515,14 @@ func (b *Bitmap) BitwiseEqual(c *Bitmap) (bool, error) {
 			bct++
 			break
 		}
-		bn = biter.Next()
 	}
 	for cn {
 		cn = citer.Next()
-		ck, cc = biter.Value()
+		ck, cc = citer.Value()
 		if cc.N() != 0 {
 			cct++
 			break
 		}
-		cn = biter.Next()
 	}
 	if bn {
 		return false, fmt.Errorf("container mismatch: %d vs %d containers, first bitmap has extra container %d [%v bits]", bct, cct, bk, bc)

From e93d2fe06c3c7bd0d4e8c969a0c6e93ad37325cf Mon Sep 17 00:00:00 2001
From: Seebs <seebs@seebs.net>
Date: Mon, 29 Mar 2021 12:29:32 -0500
Subject: [PATCH 2/5] bitmap unmarshalling and testing bug fixes

When unmarshalling ops, we weren't adding a meaningful OpN to them,
resulting in misleading reports from `pilosa inspect`. Also, we were
mistakenly reporting things as "mapped" when they were actually
using their internal storage (as with small array containers).

Add the "sanity check" to `pilosa inspect` so that errors like the
above get noticed more easily and corrected. Also, to make that work,
have roaring.InspectBinary actually put containers in the bitmap
it creates rather than just creating info entries for them.
---
 ctl/inspect.go              | 11 ++++++++++-
 roaring/roaring.go          |  7 ++++---
 roaring/unmarshal_binary.go | 12 ++++++++++--
 3 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/ctl/inspect.go b/ctl/inspect.go
index f20c974c6..1ed1832fd 100644
--- a/ctl/inspect.go
+++ b/ctl/inspect.go
@@ -29,6 +29,7 @@ import (
 	"syscall"
 	"text/tabwriter"
 	"time"
+	"unsafe"
 
 	"github.com/gogo/protobuf/proto"
 	"github.com/pilosa/pilosa/v2"
@@ -360,16 +361,24 @@ func (cmd *InspectCommand) InspectFile(f *os.File, fi os.FileInfo) error {
 			fmt.Fprintf(cmd.Stderr, "inspect command: munmap failed: %v", err)
 		}
 	}()
+	mappedFrom := uintptr(unsafe.Pointer(&data[0]))
+	mappedTo := mappedFrom + uintptr(len(data))
 	// Attach the mmap file to the bitmap.
 	t := time.Now()
 	fmt.Fprintf(cmd.Stderr, "inspecting bitmap...")
 	var info roaring.BitmapInfo
-	_, _, err = roaring.InspectBinary(data, true, &info)
+	bitmap, _, err := roaring.InspectBinary(data, true, &info)
 	fmt.Fprintf(cmd.Stderr, " (%s)\n", time.Since(t))
 	cmd.DisplayInfo(info)
 	if err != nil {
 		return errors.Wrap(err, "inspecting")
 	}
+	mappedIn, mappedOut, unmappedIn, errs, err := bitmap.SanityCheckMapping(mappedFrom, mappedTo)
+	if err != nil {
+		fmt.Fprintf(cmd.Stderr, "sanity check: %d mapped in, %d mapped out, %d unmapped in, %d errors\n",
+			mappedIn, mappedOut, unmappedIn, errs)
+		fmt.Fprintf(cmd.Stderr, "last error: %v\n", err)
+	}
 	return nil
 }
 
diff --git a/roaring/roaring.go b/roaring/roaring.go
index 73192ced6..bd2ee9d46 100644
--- a/roaring/roaring.go
+++ b/roaring/roaring.go
@@ -6002,7 +6002,7 @@ func (op *op) UnmarshalBinary(data []byte) error {
 
 	switch op.typ {
 	case opTypeAdd, opTypeRemove:
-		// nothing to do, just being not-default
+		op.opN = 1
 	case opTypeAddBatch, opTypeRemoveBatch:
 		// This ensures that in doing 13+op.value*8, the max int won't be exceeded and a wrap around case
 		// (resulting in a negative value) won't occur in the slice indexing while writing
@@ -6013,8 +6013,9 @@ func (op *op) UnmarshalBinary(data []byte) error {
 			return fmt.Errorf("op data truncated - expected %d, got %d", 13+op.value*8, len(data))
 		}
 		_, _ = h.Write(data[13 : 13+op.value*8])
-		op.values = make([]uint64, op.value)
-		for i := uint64(0); i < op.value; i++ {
+		op.opN = int(op.value)
+		op.values = make([]uint64, op.opN)
+		for i := range op.values {
 			start := 13 + i*8
 			op.values[i] = binary.LittleEndian.Uint64(data[start : start+8])
 		}
diff --git a/roaring/unmarshal_binary.go b/roaring/unmarshal_binary.go
index 72d73e961..a29bba516 100644
--- a/roaring/unmarshal_binary.go
+++ b/roaring/unmarshal_binary.go
@@ -58,7 +58,10 @@ func (b *Bitmap) UnmarshalBinary(data []byte) (err error) {
 		default:
 			panic("invalid container type")
 		}
-		newC.setMapped(true)
+		// If we're using the iterator's pointer, we're "mapped". But
+		// for instance, small arrays may use their own data structures,
+		// which is fine.
+		newC.setMapped(newC.pointer == itrPointer)
 		if !b.preferMapping {
 			newC = newC.unmapOrClone()
 		}
@@ -150,10 +153,14 @@ func InspectBinary(data []byte, mapped bool, info *BitmapInfo) (b *Bitmap, mappe
 		default:
 			panic("invalid container type")
 		}
-		newC.setMapped(true)
+		// If our pointer isn't itrPointer, we aren't actually mapped.
+		newC.setMapped(newC.pointer == itrPointer)
 		if !mapped {
 			newC = newC.unmapOrClone()
 		}
+		// Pristine means this is the original object read in from
+		// roaring data, even if it's not mapped, which this is for
+		// now.
 		newC.flags |= flagPristine
 		if newC.flags&flagMapped != 0 {
 			mappedAny = true
@@ -169,6 +176,7 @@ func InspectBinary(data []byte, mapped bool, info *BitmapInfo) (b *Bitmap, mappe
 		})
 		info.ContainerCount++
 		info.BitCount += uint64(newC.n)
+		b.Containers.Put(itrKey, newC)
 		itrKey, itrCType, itrN, itrLen, itrPointer, itrErr = itr.Next()
 	}
 	// note: if we get a non-EOF err, it's possible that we made SOME

From 5ffa4ba8033cb24d841a1148e8329eddf4481fdc Mon Sep 17 00:00:00 2001
From: Seebs <seebs@seebs.net>
Date: Mon, 29 Mar 2021 10:01:21 -0500
Subject: [PATCH 3/5] drop "batched" flag from Add operation

The "batched" flag creates a complexity which is that the return value of Add
might or might not be meaningful, but it doesn't really buy us very much.

If we are concerned about the ops log size of writing single ops as 21-byte
arrays of 1 op rather than as 13-byte ops, we can make the AddN code smarter
about how it writes ops. And probably should.

Along with this, change Remove to use the batched operation form, which
writes a more meaningful ops log, and return a meaningful value for changes
made. Otherwise, it ends up writing potentially thousands of ops to the
ops log without reporting any OpN, because the number of ops written isn't
the same as the number of changes those ops made. This could result in
files growing by megabytes without OpN changing.

There was a comment here about a test failing with RemoveN. I can't prove
it, but I strongly suspect that this was actually a result of that test
case hitting a particular bug that we eventually fixed, and which we might
have fixed sooner if we'd realized why using RemoveN made that test
fail.
---
 bluegreentx.go            |  6 +++---
 bolt.go                   | 20 ++++----------------
 bolt_test.go              | 28 ++++++++++++++--------------
 catcher.go                |  4 ++--
 dbshard_internal_test.go  |  3 +--
 fragment.go               |  5 ++---
 fragment_internal_test.go |  4 ++--
 rbf.go                    | 19 ++++---------------
 rrtx.go                   | 28 +++++-----------------------
 stattx.go                 |  4 ++--
 tx.go                     | 23 +----------------------
 11 files changed, 40 insertions(+), 104 deletions(-)

diff --git a/bluegreentx.go b/bluegreentx.go
index 666389315..235b16b84 100644
--- a/bluegreentx.go
+++ b/bluegreentx.go
@@ -539,7 +539,7 @@ func (c *blueGreenTx) isIn(index, field, view string, shard uint64, ckey uint64)
 	return
 }
 
-func (c *blueGreenTx) Add(index, field, view string, shard uint64, batched bool, a ...uint64) (changeCount int, err error) {
+func (c *blueGreenTx) Add(index, field, view string, shard uint64, a ...uint64) (changeCount int, err error) {
 	c.checker.see(index, field, view, shard)
 	//vv("blueGreenTx) Add(index=%v, field=%v, view=%v, shard=%v", index, field, view, shard)
 	defer func() {
@@ -554,10 +554,10 @@ func (c *blueGreenTx) Add(index, field, view string, shard uint64, batched bool,
 	a2 := make([]uint64, len(a))
 	copy(a2, a)
 
-	ach, errA := c.a.Add(index, field, view, shard, batched, a...)
+	ach, errA := c.a.Add(index, field, view, shard, a...)
 	_, _ = ach, errA
 
-	bch, errB := c.b.Add(index, field, view, shard, batched, a2...)
+	bch, errB := c.b.Add(index, field, view, shard, a2...)
 
 	if !c.o.blueGreenOff {
 
diff --git a/bolt.go b/bolt.go
index 5a942dcb0..0ce42b0f5 100644
--- a/bolt.go
+++ b/bolt.go
@@ -614,28 +614,16 @@ func (tx *BoltTx) RemoveContainer(index, field, view string, shard uint64, ckey
 }
 
 // Add sets all the a bits hot in the specified fragment.
-func (tx *BoltTx) Add(index, field, view string, shard uint64, batched bool, a ...uint64) (changeCount int, err error) {
-	return tx.addOrRemove(index, field, view, shard, batched, false, a...)
+func (tx *BoltTx) Add(index, field, view string, shard uint64, a ...uint64) (changeCount int, err error) {
+	return tx.addOrRemove(index, field, view, shard, false, a...)
 }
 
 // Remove clears all the specified a bits in the chosen fragment.
 func (tx *BoltTx) Remove(index, field, view string, shard uint64, a ...uint64) (changeCount int, err error) {
-	const batched = false
-	const remove = true
-	return tx.addOrRemove(index, field, view, shard, batched, remove, a...)
+	return tx.addOrRemove(index, field, view, shard, true, a...)
 }
 
-func (tx *BoltTx) addOrRemove(index, field, view string, shard uint64, batched, remove bool, a ...uint64) (changeCount int, err error) {
-
-	// pure hack to match RoaringTx
-	defer func() {
-		if !remove && !batched {
-			if changeCount > 0 {
-				changeCount = 1
-			}
-		}
-	}()
-
+func (tx *BoltTx) addOrRemove(index, field, view string, shard uint64, remove bool, a ...uint64) (changeCount int, err error) {
 	if len(a) == 0 {
 		return 0, nil
 	}
diff --git a/bolt_test.go b/bolt_test.go
index 13485161c..81ce74bf5 100644
--- a/bolt_test.go
+++ b/bolt_test.go
@@ -56,7 +56,7 @@ func BoltMustSetBitvalue(dbwrap *BoltWrapper, index, field, view string, shard u
 	tx, _ := dbwrap.NewTx(writable, index, Txo{})
 
 	// add a bit
-	changed, err := tx.Add(index, field, view, shard, doBatched, putme)
+	changed, err := tx.Add(index, field, view, shard, putme)
 	if changed != 1 {
 		panic("should have 1 bit changed")
 	}
@@ -123,7 +123,7 @@ func TestBolt_DeleteFragment(t *testing.T) {
 	views := []string{"v1", "v2"}
 	for _, view := range views {
 		for _, v := range bits {
-			changed, err := tx.Add(index, field, view, shard, doBatched, v)
+			changed, err := tx.Add(index, field, view, shard, v)
 			if changed <= 0 {
 				panic("should have changed")
 			}
@@ -239,7 +239,7 @@ func TestBolt_SetBitmap(t *testing.T) {
 	index, field, view, shard := "i", "f", "v", uint64(0)
 	tx, _ := dbwrap.NewTx(writable, index, Txo{})
 	bitvalue := uint64(0)
-	changed, err := tx.Add(index, field, view, shard, doBatched, bitvalue)
+	changed, err := tx.Add(index, field, view, shard, bitvalue)
 	if changed <= 0 {
 		panic("should have changed")
 	}
@@ -281,14 +281,14 @@ func TestBolt_OffsetRange(t *testing.T) {
 	tx, _ := dbwrap.NewTx(writable, index, Txo{})
 
 	bitvalue := uint64(1 << 20)
-	changed, err := tx.Add(index, field, view, shard, doBatched, bitvalue)
+	changed, err := tx.Add(index, field, view, shard, bitvalue)
 	if changed <= 0 {
 		panic("should have changed")
 	}
 	PanicOn(err)
 
 	bitvalue2 := uint64(1<<20 + 1)
-	changed, err = tx.Add(index, field, view, shard, doBatched, bitvalue2)
+	changed, err = tx.Add(index, field, view, shard, bitvalue2)
 	if changed <= 0 {
 		panic("should have changed")
 	}
@@ -374,7 +374,7 @@ func TestBolt_Count_dense_containers(t *testing.T) {
 
 	expected := 0
 	for i := uint64(0); i < (1<<16)+2; i += 2 {
-		changed, err := tx.Add(index, field, view, shard, doBatched, i)
+		changed, err := tx.Add(index, field, view, shard, i)
 		PanicOn(err)
 		if changed <= 0 {
 			panic("wat? should have changed")
@@ -420,7 +420,7 @@ func TestBolt_ContainerIterator_on_one_bit(t *testing.T) {
 	bitvalue := uint64(42)
 
 	// add a bit
-	changed, err := tx.Add(index, field, view, shard, doBatched, bitvalue)
+	changed, err := tx.Add(index, field, view, shard, bitvalue)
 	if changed <= 0 {
 		panic("should have changed")
 	}
@@ -478,7 +478,7 @@ func TestBolt_ContainerIterator_on_one_bit_fail_to_find(t *testing.T) {
 	searchme := putme + 1
 
 	// add a bit
-	changed, err := tx.Add(index, field, view, shard, doBatched, putme)
+	changed, err := tx.Add(index, field, view, shard, putme)
 	if changed <= 0 {
 		panic("should have changed")
 	}
@@ -533,7 +533,7 @@ func TestBolt_ContainerIterator_empty_iteration_loop(t *testing.T) {
 	searchme := uint64(1 << 17) // in the next container, key:2
 
 	// add a bit
-	changed, err := tx.Add(index, field, view, shard, doBatched, putme)
+	changed, err := tx.Add(index, field, view, shard, putme)
 	if changed <= 0 {
 		panic("should have changed")
 	}
@@ -582,7 +582,7 @@ func TestBolt_ForEach_on_one_bit(t *testing.T) {
 	bitvalue := uint64(42)
 
 	// add a bit
-	changed, err := tx.Add(index, field, view, shard, doBatched, bitvalue)
+	changed, err := tx.Add(index, field, view, shard, bitvalue)
 	if changed <= 0 {
 		panic("should have changed")
 	}
@@ -1132,7 +1132,7 @@ func TestBolt_DeleteIndex(t *testing.T) {
 	bitvalue := uint64(777)
 	bits := []uint64{0, 3, 1 << 16, 1<<16 + 3, 8 << 16}
 	for _, v := range bits {
-		changed, err := tx.Add(index, field, view, shard, doBatched, v)
+		changed, err := tx.Add(index, field, view, shard, v)
 		if changed <= 0 {
 			panic("should have changed")
 		}
@@ -1140,7 +1140,7 @@ func TestBolt_DeleteIndex(t *testing.T) {
 	}
 
 	index2 := "i2" // should not be deleted, even though it shares a prefix with 'i'
-	changed, err := tx.Add(index2, field, view, shard, doBatched, bitvalue)
+	changed, err := tx.Add(index2, field, view, shard, bitvalue)
 	if changed <= 0 {
 		panic("should have changed")
 	}
@@ -1196,7 +1196,7 @@ func TestBolt_DeleteIndex_over100k(t *testing.T) {
 	//limit := uint64(101)
 	for v := uint64(1); v < limit; v++ {
 		// shift by << 16 to get into a different shard
-		changed, err := tx.Add(index, field, view, shard, doBatched, v<<16)
+		changed, err := tx.Add(index, field, view, shard, v<<16)
 		if changed <= 0 {
 			panic("should have changed")
 		}
@@ -1208,7 +1208,7 @@ func TestBolt_DeleteIndex_over100k(t *testing.T) {
 	}
 
 	index2 := "i2" // should not be deleted, even though it shares a prefix with 'i'
-	changed, err := tx.Add(index2, field, view, shard, doBatched, bitvalue)
+	changed, err := tx.Add(index2, field, view, shard, bitvalue)
 	if changed <= 0 {
 		panic("should have changed")
 	}
diff --git a/catcher.go b/catcher.go
index 983499fef..6b9e421c1 100644
--- a/catcher.go
+++ b/catcher.go
@@ -151,7 +151,7 @@ func (c *catcherTx) IsDone() bool {
 	return c.b.IsDone()
 }
 
-func (c *catcherTx) Add(index, field, view string, shard uint64, batched bool, a ...uint64) (changeCount int, err error) {
+func (c *catcherTx) Add(index, field, view string, shard uint64, a ...uint64) (changeCount int, err error) {
 
 	defer func() {
 		if r := recover(); r != nil {
@@ -159,7 +159,7 @@ func (c *catcherTx) Add(index, field, view string, shard uint64, batched bool, a
 			PanicOn(r)
 		}
 	}()
-	return c.b.Add(index, field, view, shard, batched, a...)
+	return c.b.Add(index, field, view, shard, a...)
 }
 
 func (c *catcherTx) Remove(index, field, view string, shard uint64, a ...uint64) (changeCount int, err error) {
diff --git a/dbshard_internal_test.go b/dbshard_internal_test.go
index 6c2926bc0..55ddb077c 100644
--- a/dbshard_internal_test.go
+++ b/dbshard_internal_test.go
@@ -300,7 +300,6 @@ func makeRBFtestDB(path string, h *Holder, shard uint64) {
 func makeTxTestDBWithViewsShards(holder *Holder, idx *Index, exp *FieldView2Shards) {
 
 	// TODO(jea): need date time quantum views!!
-	batched := false
 	for field, viewmap := range exp.m {
 		for view, shset := range viewmap {
 
@@ -310,7 +309,7 @@ func makeTxTestDBWithViewsShards(holder *Holder, idx *Index, exp *FieldView2Shar
 				// simply write 1 bit to each shard to force its creation.
 				bits := []uint64{(shard << shardwidth.Exponent) + 1}
 				tx := idx.holder.txf.NewTx(Txo{Write: writable, Index: idx, Shard: shard})
-				changeCount, err := tx.Add(idx.name, field, view, shard, batched, bits...)
+				changeCount, err := tx.Add(idx.name, field, view, shard, bits...)
 				PanicOn(err)
 				if changeCount != len(bits) {
 					panic(fmt.Sprintf("writing field '%v', view '%v' shard '%v', expected changeCount to equal len bits = %v but was %v", field, view, shard, len(bits), changeCount))
diff --git a/fragment.go b/fragment.go
index 09c11d7ce..8d305bde1 100644
--- a/fragment.go
+++ b/fragment.go
@@ -729,7 +729,7 @@ func (f *fragment) unprotectedSetBit(tx Tx, rowID, columnID uint64) (changed boo
 
 	// Write to storage.
 	changeCount := 0
-	changeCount, err = tx.Add(f.index(), f.field(), f.view(), f.shard, doBatched, pos)
+	changeCount, err = tx.Add(f.index(), f.field(), f.view(), f.shard, pos)
 	changed = changeCount > 0
 	if err != nil {
 		return false, errors.Wrap(err, "writing")
@@ -2488,8 +2488,7 @@ func (f *fragment) importPositions(tx Tx, set, clear []uint64, rowSet map[uint64
 			f.stats.Count(MetricImportingN, int64(len(set)), 1)
 
 			// TODO benchmark Add/RemoveN behavior with sorted/unsorted positions
-			// Note: AddN() avoids writing to the op-log. While Add() does.
-			changedN, err := tx.Add(f.index(), f.field(), f.view(), f.shard, !doBatched, set...)
+			changedN, err := tx.Add(f.index(), f.field(), f.view(), f.shard, set...)
 			if err != nil {
 				return errors.Wrap(err, "adding positions")
 			}
diff --git a/fragment_internal_test.go b/fragment_internal_test.go
index 68d5e2c0a..846fd4e9c 100644
--- a/fragment_internal_test.go
+++ b/fragment_internal_test.go
@@ -5441,7 +5441,7 @@ func TestRemapCache(t *testing.T) {
 	}()
 
 	// create a container
-	_, err := tx.Add(index, field, view, shard, !doBatched, 65537)
+	_, err := tx.Add(index, field, view, shard, 65537)
 	if err != nil {
 		t.Fatalf("storage add: %v", err)
 	}
@@ -5454,7 +5454,7 @@ func TestRemapCache(t *testing.T) {
 	_ = f.mustRow(tx, 0)
 	// add a bit that isn't in that container, so that container doesn't
 	// change
-	_, err = tx.Add(index, field, view, shard, !doBatched, 2)
+	_, err = tx.Add(index, field, view, shard, 2)
 	if err != nil {
 		t.Fatalf("storage add: %v", err)
 	}
diff --git a/rbf.go b/rbf.go
index e1c70aa12..fbbfb36e8 100644
--- a/rbf.go
+++ b/rbf.go
@@ -241,27 +241,16 @@ func (tx *RBFTx) RemoveContainer(index, field, view string, shard uint64, key ui
 }
 
 // Add sets all the a bits hot in the specified fragment.
-func (tx *RBFTx) Add(index, field, view string, shard uint64, batched bool, a ...uint64) (changeCount int, err error) {
-	return tx.addOrRemove(index, field, view, shard, batched, false, a...)
+func (tx *RBFTx) Add(index, field, view string, shard uint64, a ...uint64) (changeCount int, err error) {
+	return tx.addOrRemove(index, field, view, shard, false, a...)
 }
 
 // Remove clears all the specified a bits in the chosen fragment.
 func (tx *RBFTx) Remove(index, field, view string, shard uint64, a ...uint64) (changeCount int, err error) {
-	const batched = false
-	const remove = true
-	return tx.addOrRemove(index, field, view, shard, batched, remove, a...)
+	return tx.addOrRemove(index, field, view, shard, true, a...)
 }
 
-func (tx *RBFTx) addOrRemove(index, field, view string, shard uint64, batched, remove bool, a ...uint64) (changeCount int, err error) {
-	// pure hack to match RoaringTx
-	defer func() {
-		if !remove && !batched {
-			if changeCount > 0 {
-				changeCount = 1
-			}
-		}
-	}()
-
+func (tx *RBFTx) addOrRemove(index, field, view string, shard uint64, remove bool, a ...uint64) (changeCount int, err error) {
 	if len(a) == 0 {
 		return 0, nil
 	}
diff --git a/rrtx.go b/rrtx.go
index fe9818dcb..288b4f361 100644
--- a/rrtx.go
+++ b/rrtx.go
@@ -199,25 +199,16 @@ func (tx *RoaringTx) RemoveContainer(index, field, view string, shard uint64, ke
 	return nil
 }
 
-func (tx *RoaringTx) Add(index, field, view string, shard uint64, batched bool, a ...uint64) (changeCount int, err error) {
+func (tx *RoaringTx) Add(index, field, view string, shard uint64, a ...uint64) (changeCount int, err error) {
 	//vv("RoaringTx.Add(index='%v', shard='%v') stack=\n%v", index, shard, stack())
 	b, err := tx.bitmap(index, field, view, shard)
 	if err != nil {
 		return 0, err
 	}
-	if !batched {
-		changed, err := b.Add(a...)
-		if changed {
-			return 1, err
-		}
-		return 0, err
-	}
-
 	// Note: do not replace b.AddN() with b.DirectAddN().
-	// DirectAddN() does not do op-log operations inside roaring
-	// This creates a problem because RoaringTx needs the op-log
-	// to know when to flush the fragment to disk.
-	count, err := b.AddN(a...) // AddN does oplog batches. needed to keep op-log up to date.
+	// DirectAddN() does not do op-log operations inside roaring, so the
+	// on-disk representation no longer matches the in-memory operations.
+	count, err := b.AddN(a...)
 	return count, err
 }
 
@@ -226,16 +217,7 @@ func (tx *RoaringTx) Remove(index, field, view string, shard uint64, a ...uint64
 	if err != nil {
 		return 0, err
 	}
-	changed, err := b.Remove(a...) // green TestFragment_Bug_Q2DoubleDelete
-	if changed {
-		return 1, err
-	} else {
-		return 0, err
-	}
-
-	// Note: don't replace b.Remove(a...) with b.RemoveN(a...) or
-	// with b.DirectRemoveN(a...). If you do, you'll see
-	// TestFragment_Bug_Q2DoubleDelete go red.
+	return b.RemoveN(a...)
 }
 
 func (tx *RoaringTx) Contains(index, field, view string, shard uint64, v uint64) (exists bool, err error) {
diff --git a/stattx.go b/stattx.go
index 6338e7409..5b7bece29 100644
--- a/stattx.go
+++ b/stattx.go
@@ -429,7 +429,7 @@ func (c *statTx) IsDone() (done bool) {
 	return c.b.IsDone()
 }
 
-func (c *statTx) Add(index, field, view string, shard uint64, batched bool, a ...uint64) (changeCount int, err error) {
+func (c *statTx) Add(index, field, view string, shard uint64, a ...uint64) (changeCount int, err error) {
 	me := kAdd
 
 	t0 := time.Now()
@@ -443,7 +443,7 @@ func (c *statTx) Add(index, field, view string, shard uint64, batched bool, a ..
 			PanicOn(r)
 		}
 	}()
-	return c.b.Add(index, field, view, shard, batched, a...)
+	return c.b.Add(index, field, view, shard, a...)
 }
 
 func (c *statTx) Remove(index, field, view string, shard uint64, a ...uint64) (changeCount int, err error) {
diff --git a/tx.go b/tx.go
index f33648cb0..958491a39 100644
--- a/tx.go
+++ b/tx.go
@@ -22,11 +22,6 @@ import (
 	//txkey "github.com/pilosa/pilosa/v2/txkey"
 )
 
-// batch operations want Tx.Add(batched=doBatch), while bit-at-a-time want Tx.Add(batched=!doBatched)
-// Used in Tx.Add() to get consistency between RoaringTx and other Tx implementations on
-// the changeCount returned.
-const doBatched = false // must be false, do not change this without adjusting the Add() implementations correspondingly.
-
 // writable initializes Tx that update, use !writable for read-only.
 const writable = true
 
@@ -136,23 +131,7 @@ type Tx interface {
 	// in the specified fragment.
 	RemoveContainer(index, field, view string, shard uint64, ckey uint64) error
 
-	// Add adds the 'a' bits to the specified fragment.
-	//
-	// Using batched=true allows efficient bulk-import.
-	//
-	// Notes on the RoaringTx implementation:
-	// If the batched flag is true, then the roaring.Bitmap.AddN() is used, which does oplog batches.
-	// If the batched flag is false, then the roaring.Bitmap.Add() is used, which does simple opTypeAdd single adds.
-	//
-	// Beware: if batched is false, then changeCount will only ever be 0 or 1,
-	// because it calls roaring.Add().
-	// If batched is true, we call roaring.DirectAddN() and then changeCount
-	// will be accurate if the changeCount is greater than 0.
-	//
-	// Hence: only ever call Add(batched=false) if changeCount is expected to be 0 or 1.
-	// Or, must use Add(batched=true) if changeCount can be > 1.
-	//
-	Add(index, field, view string, shard uint64, batched bool, a ...uint64) (changeCount int, err error)
+	Add(index, field, view string, shard uint64, a ...uint64) (changeCount int, err error)
 
 	// Remove removes the 'a' values from the Bitmap for the fragment.
 	Remove(index, field, view string, shard uint64, a ...uint64) (changeCount int, err error)

From 014a94c9c74b11696fc1bf207668a43abc5d9660 Mon Sep 17 00:00:00 2001
From: Seebs <seebs@seebs.net>
Date: Mon, 29 Mar 2021 14:39:49 -0500
Subject: [PATCH 4/5] TxBitmap: track seen container keys

We can't assume that a container we've seen stays present in our bitmap
after possible remove operations. Solution: Track keys seen.
---
 tx.go | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/tx.go b/tx.go
index 958491a39..9a4806356 100644
--- a/tx.go
+++ b/tx.go
@@ -257,6 +257,9 @@ type TxBitmap struct {
 	field string
 	view  string
 	shard uint64
+	// Container keys we've already snagged containers for, even if
+	// those containers have since been deleted by remove ops.
+	seen map[uint64]struct{}
 }
 
 func NewTxBitmap(tx Tx, index, field, view string, shard uint64) *TxBitmap {
@@ -267,6 +270,7 @@ func NewTxBitmap(tx Tx, index, field, view string, shard uint64) *TxBitmap {
 		field: field,
 		view:  view,
 		shard: shard,
+		seen:  make(map[uint64]struct{}),
 	}
 }
 
@@ -288,14 +292,14 @@ func (b *TxBitmap) Remove(a ...uint64) (changed bool, err error) {
 func (b *TxBitmap) ensureContainers(a ...uint64) error {
 	for _, v := range a {
 		key := highbits(v)
-		if b.b.Containers.Get(key) != nil {
+		if _, ok := b.seen[key]; ok {
 			continue
 		}
-
 		c, err := b.tx.Container(b.index, b.field, b.view, b.shard, key)
 		if err != nil {
 			return err
 		}
+		b.seen[key] = struct{}{}
 		b.b.Containers.Put(key, c)
 	}
 	return nil
@@ -308,6 +312,14 @@ func (b *TxBitmap) Flush() error {
 		if err := b.tx.PutContainer(b.index, b.field, b.view, b.shard, key, c); err != nil {
 			return err
 		}
+		delete(b.seen, key)
+	}
+	// remove containers we have seen but no longer have, because that means
+	// we deleted everything from them.
+	for key := range b.seen {
+		if err := b.tx.RemoveContainer(b.index, b.field, b.view, b.shard, key); err != nil {
+			return err
+		}
 	}
 	return nil
 }

From 5a7c0971ca612f96467c493b8988c5f1fd15f67e Mon Sep 17 00:00:00 2001
From: Seebs <seebs@seebs.net>
Date: Mon, 29 Mar 2021 14:40:26 -0500
Subject: [PATCH 5/5] additional fragment tests: bitmap file growth, TxBitmap
 data loss

Checking issues encountered while tracking down an unexpected disk
usage increase.
---
 fragment_internal_test.go | 81 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/fragment_internal_test.go b/fragment_internal_test.go
index 846fd4e9c..6afaf4d2a 100644
--- a/fragment_internal_test.go
+++ b/fragment_internal_test.go
@@ -6057,3 +6057,84 @@ func TestSliceDifference(t *testing.T) {
 		compareSlices(t, name, tc.expected, result)
 	}
 }
+
+func TestBitmapGrowth(t *testing.T) {
+	roaringOnlyTest(t)
+	f, _, tx := mustOpenFragment(t, "i", "f", viewBSIGroupPrefix+"foo", 0, "")
+	path := f.path()
+	defer f.Clean(t)
+	const values = 500
+	cols := make([]uint64, values)
+	vals := make([]int64, values)
+	for i := range cols {
+		cols[i] = uint64(rand.Int63n(65536))
+		vals[i] = rand.Int63n(24)
+	}
+	err := f.importValue(tx, cols, vals, 7, false)
+	if err != nil {
+		t.Fatalf("importing values: %v", err)
+	}
+	info, err := os.Stat(path)
+	if err != nil {
+		t.Fatalf("statting %s: %v", path, err)
+	}
+	prevSize := info.Size()
+	prevOpN := f.opN
+	err = f.importValue(tx, cols, vals, 7, false)
+	if err != nil {
+		t.Fatalf("importing values: %v", err)
+	}
+	info, err = os.Stat(path)
+	if err != nil {
+		t.Fatalf("statting %s: %v", path, err)
+	}
+	deltaSize := info.Size() - prevSize
+	deltaOpN := f.opN - prevOpN
+	// This is somewhat arbitrary, but the issue tested for was that
+	// opN would grow by 0 or 1 with multiple KB of actual ops written.
+	// If deltaOpN is at least 20, we'll probably see snapshots happening
+	// at least occasionally, and if deltaSize is under 1024, the writes
+	// are probably going to be small enough that the regular backlog of
+	// snapshotting catches them anyway.
+	if deltaSize > 1024 && deltaOpN < 20 {
+		t.Fatalf("bitmap grew by %d bytes but OpN only grew by %d",
+			deltaSize, deltaOpN)
+	}
+}
+
+func TestTxBitmap(t *testing.T) {
+	f, _, tx := mustOpenFragment(t, "i", "f", viewBSIGroupPrefix+"foo", 0, "")
+	defer f.Clean(t)
+	f.MaxOpN = 8
+	cols := []uint64{1, 2, 3, 4, 5, 6, 65537, 131073}
+	vals := []int64{4, 4, 4, 4, 4, 4, 4, 4}
+	zeros := []int64{0, 0, 0, 0, 0, 0, 0, 0}
+	err := f.importValue(tx, cols, vals, 7, false)
+	if err != nil {
+		t.Fatalf("importing values: %v", err)
+	}
+	expected := []uint64{1, (4 * ShardWidth) + 1}
+	var got []uint64
+	_ = tx.ForEach("i", "f", viewBSIGroupPrefix+"foo", 0, func(i uint64) error {
+		got = append(got, i)
+		return nil
+	})
+	t.Logf("initial: %d", got)
+	err = f.importValue(tx, cols[1:], zeros[1:], 7, true)
+	if err != nil {
+		t.Fatalf("clearing values: %v", err)
+	}
+	got = got[:0]
+	_ = tx.ForEach("i", "f", viewBSIGroupPrefix+"foo", 0, func(i uint64) error {
+		got = append(got, i)
+		return nil
+	})
+	if len(got) != len(expected) {
+		t.Fatalf("bitmap clear unsuccessful: expected %d, got %d", expected, got)
+	}
+	for i := range expected {
+		if expected[i] != got[i] {
+			t.Fatalf("bitmap clear unsuccessful: expected %d, got %d", expected, got)
+		}
+	}
+}