From 4640f7f1a35a07de645e43f4e490c68695d0ea82 Mon Sep 17 00:00:00 2001
From: Seebs <seebs@seebs.net>
Date: Fri, 19 Jun 2020 13:10:56 -0500
Subject: [PATCH] further checking about possible row cache errors

Check also on bit setting for cache/storage mismatches.

Also, add diagnostic printing to a couple of points inside roaring
where we think the code can do something wrong. All of these are
cases where what the code does is actually wrong -- we're leaving
it wrong because we want to confirm/deny that this is happening
when the strange behavior happens.

We also filter these bugs a little bit -- we only print some of
them when they would result in a change to a mapped-or-frozen
container, which would be a bug that could affect things. There's
actually cases where this is wrong -- if these bugs affected
something like one of the keep/filter rows in unsignedLT, that could
affect things -- but it shouldn't apply in the cases we're
concerned with, and without the filtering, our regular tests
produce about 500,000 of one of these messages, all from removing
the last bit in array containers in a test where they actually
won't be used again.

We also mark all the test containers frozen so they *will* show
errors if this happens to them.
---
 fragment.go                     | 11 +++++++++++
 roaring/roaring.go              | 13 +++++++++++++
 roaring/roaring_helpers_test.go |  6 +++---
 3 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/fragment.go b/fragment.go
index 81f644e3c..550e4afb7 100644
--- a/fragment.go
+++ b/fragment.go
@@ -556,6 +556,17 @@ func (f *fragment) unprotectedSetBit(rowID, columnID uint64) (changed bool, err
 
 	// Don't update the cache if nothing changed.
 	if !changed {
+		row, _ := f.rowCache.Fetch(rowID)
+		if row != nil && !row.Includes(columnID) {
+			// paranoia time. It should be impossible for the
+			// rowCache to have an entry which lacks a bit which is
+			// in storage, but we've seen some behavior which
+			// indicated this is happening... so we'll check and
+			// clear the rowCache rather than risking returning
+			// inconsistent data in a query.
+			f.Logger.Printf("INCONSISTENT: index:%s field:%s found bit row/col %d/%d not in cache after in storage; dropping cached row, consider restarting Pilosa.", f.index, f.field, rowID, columnID)
+			f.rowCache.Add(rowID, nil)
+		}
 		return changed, nil
 	}
 
diff --git a/roaring/roaring.go b/roaring/roaring.go
index 22d2821db..fef47bfda 100644
--- a/roaring/roaring.go
+++ b/roaring/roaring.go
@@ -21,6 +21,7 @@ import (
 	"hash/fnv"
 	"io"
 	"math/bits"
+	"os"
 	"reflect"
 	"sort"
 	"unsafe"
@@ -3190,6 +3191,9 @@ func (c *Container) arrayRemove(v uint16) (*Container, bool) {
 	}
 	// removing the last item? we can just return the empty container.
 	if c.N() == 1 {
+		if c.flags&(flagFrozen|flagMapped) != 0 {
+			fmt.Fprintf(os.Stderr, "INCONSISTENT: array remove of %d setting N in another container\n", v)
+		}
 		c.n = 0
 		return nil, true
 	}
@@ -3207,6 +3211,9 @@ func (c *Container) bitmapRemove(v uint16) (*Container, bool) {
 	}
 	// removing the last item? we can just return the empty container.
 	if c.N() == 1 {
+		if c.flags&(flagFrozen|flagMapped) != 0 {
+			fmt.Fprintf(os.Stderr, "INCONSISTENT: bitmap remove of %d setting N in another container\n", v)
+		}
 		c.n = 0
 		return nil, true
 	}
@@ -3233,6 +3240,9 @@ func (c *Container) runRemove(v uint16) (*Container, bool) {
 	}
 	// removing the last item? we can just return the empty container.
 	if c.N() == 1 {
+		if c.flags&(flagFrozen|flagMapped) != 0 {
+			fmt.Fprintf(os.Stderr, "INCONSISTENT: run remove of %d setting N in another container\n", v)
+		}
 		c.n = 0
 		return nil, true
 	}
@@ -4292,6 +4302,9 @@ func unionArrayArrayInPlace(a, b *Container) *Container {
 			// ... but we also want to be sure we don't end up
 			// copying in a mapped object into our not-mapped
 			// object.
+			if !b.Mapped() {
+				fmt.Fprintf(os.Stderr, "INCONSISTENT: unionArrayArray referencing array of %d elements\n", b.N())
+			}
 			a.setArrayMaybeCopy(b.array(), b.Mapped())
 			return a.optimize()
 		}
diff --git a/roaring/roaring_helpers_test.go b/roaring/roaring_helpers_test.go
index 41d4059fb..c07e530e0 100644
--- a/roaring/roaring_helpers_test.go
+++ b/roaring/roaring_helpers_test.go
@@ -251,12 +251,12 @@ type testOp struct {
 func doContainer(typ byte, data interface{}) *Container {
 	switch typ {
 	case containerArray:
-		return NewContainerArray(data.([]uint16))
+		return NewContainerArray(data.([]uint16)).Freeze()
 	case containerBitmap:
-		c := NewContainerBitmap(-1, data.([]uint64))
+		c := NewContainerBitmap(-1, data.([]uint64)).Freeze()
 		return c
 	case containerRun:
-		return NewContainerRun(data.([]interval16))
+		return NewContainerRun(data.([]interval16)).Freeze()
 	}
 	return nil
 }