From c88192b4ac14f6588a28aa0a30bfb2569d011d0f Mon Sep 17 00:00:00 2001 From: Seebs Date: Tue, 13 Oct 2020 16:49:43 -0500 Subject: [PATCH] Check more carefully for, and also fix, containers with invalid N In rare cases, RBF can produce containers which have a recorded N value which is incorrect. This rarely affects anything, but on some particular queries, this can result in very strange outcomes, like array containers with more than 1<<16 entries. To fix this, we have toContainer specify that it doesn't know the correct N for the bitmap containers it's creating, which costs extra time for counting, and should be considered a temporary workaround. Also, we add a CheckN() function which is controlled by the roaringparanoia flag, and add a number of calls to it, for instance, as deferred calls after every container operation when roaringparanoia is enabled. This means that we get improved confidence that we've caught the relevant errors, but is not suitable for production use. --- rbf/cursor.go | 4 +- rbf/cursorx.go | 8 +++- roaring/container_stash.go | 64 +++++--------------------------- roaring/roaring.go | 36 ++++++++++++++---- roaring/roaring_internal_test.go | 6 ++- roaring/roaring_nop_paranoia.go | 7 ++++ roaring/roaring_paranoia.go | 15 ++++++++ 7 files changed, 72 insertions(+), 68 deletions(-) diff --git a/rbf/cursor.go b/rbf/cursor.go index 20d8c0e7a..40d0c5e86 100644 --- a/rbf/cursor.go +++ b/rbf/cursor.go @@ -1180,7 +1180,7 @@ func (c *Cursor) merge(key uint64, data *roaring.Container) (bool, error) { if err != nil { return false, errors.Wrap(err, "cursor.merge") } - container = roaring.NewContainerBitmap(cell.BitN, d) + container = roaring.NewContainerBitmap(-1, d) case ContainerTypeRLE: d := toInterval16(cell.Data) container = roaring.NewContainerRun(d) @@ -1267,7 +1267,7 @@ func (c *Cursor) difference(key uint64, data *roaring.Container) (bool, error) { if err != nil { return false, errors.Wrap(err, "cursor.difference") } - container = roaring.NewContainerBitmap(cell.N, d) + container = roaring.NewContainerBitmap(-1, d) case ContainerTypeRLE: d := toInterval16(cell.Data) container = roaring.NewContainerRun(d) diff --git a/rbf/cursorx.go b/rbf/cursorx.go index 26d0c002d..5cf75af24 100644 --- a/rbf/cursorx.go +++ b/rbf/cursorx.go @@ -179,12 +179,16 @@ func toContainer(l leafCell, tx *Tx) (c *roaring.Container) { cloneMaybe = make([]uint64, len(bm)) copy(cloneMaybe, bm) } - c = roaring.NewContainerBitmap(l.N, cloneMaybe) + c = roaring.NewContainerBitmap(-1, cloneMaybe) case ContainerTypeBitmap: - c = roaring.NewContainerBitmap(l.N, toArray64(cpMaybe)) + c = roaring.NewContainerBitmap(-1, toArray64(cpMaybe)) case ContainerTypeRLE: c = roaring.NewContainerRun(toInterval16(cpMaybe)) } + // Note: If the "roaringparanoia" build tag isn't set, this + // should be optimized away entirely. Otherwise it's moderately + // expensive. + c.CheckN() c.SetMapped(mapped) return c } diff --git a/roaring/container_stash.go b/roaring/container_stash.go index e87a65fb8..f6a9f8f05 100644 --- a/roaring/container_stash.go +++ b/roaring/container_stash.go @@ -146,6 +146,9 @@ func NewContainerBitmap(n int, bitmap []uint64) *Container { c.bitmapRepair() } else { c.setN(int32(n)) + if roaringParanoia { + c.CheckN() + } } return c } @@ -164,6 +167,9 @@ func NewContainerBitmapN(bitmap []uint64, n int32) *Container { } else { c.setBitmap(bitmap) } + if roaringParanoia { + c.CheckN() + } return c } @@ -219,6 +225,9 @@ func NewContainerRunCopy(set []Interval16) *Container { func NewContainerRunN(set []Interval16, n int32) *Container { c := &Container{typeID: ContainerRun, n: n} c.setRuns(set) + if roaringParanoia { + c.CheckN() + } return c } @@ -624,61 +633,6 @@ func (c *Container) setRunsMaybeCopy(runs []Interval16, doCopy bool) { c.pointer, c.len, c.cap = &runs[0].Start, int32(len(runs)), int32(cap(runs)) } -// UpdateOrMake updates the container, yielding a new container if necessary. -func (c *Container) UpdateOrMake(typ byte, n int32, mapped bool) *Container { - if c == nil { - switch typ { - case ContainerRun: - c = NewContainerRunN(nil, n) - case ContainerBitmap: - c = NewContainerBitmapN(nil, n) - default: - c = NewContainerArrayN(nil, n) - } - c.flags |= flagMapped - return c - } - // ensure that we are allowed to modify this container - c = c.Thaw() - c.typeID = typ - c.n = n - // note: this probably shouldn't be happening, the decision should be getting - // made when we specify the storage. - c.setMapped(mapped) - // we don't know that any existing slice is usable, so let's ditch it - switch c.typeID { - case ContainerArray: - c.pointer, c.len, c.cap = &c.data[0], 0, stashedArraySize - case ContainerRun: - c.pointer, c.len, c.cap = &c.data[0], 0, stashedRunSize - default: - c.pointer, c.len, c.cap = nil, 0, 0 - } - return c -} - -// Update updates the container if possible. It is an error to -// call Update on a frozen container. -func (c *Container) Update(typ byte, n int32, mapped bool) { - if c == nil || c.frozen() { - panic("cannot Update a nil or frozen container") - } - c.typeID = typ - c.n = n - // note: this probably shouldn't be happening, the decision should be getting - // made when we specify the storage. - c.setMapped(mapped) - // we don't know that any existing slice is usable, so let's ditch it - switch c.typeID { - case ContainerArray: - c.pointer, c.len, c.cap = nil, 0, 0 - case ContainerRun: - c.pointer, c.len, c.cap = nil, 0, 0 - default: - c.pointer, c.len, c.cap = nil, 0, 0 - } -} - // isArray returns true if the container is an array container. func (c *Container) isArray() bool { if c == nil { diff --git a/roaring/roaring.go b/roaring/roaring.go index 89c9618bf..0ce08906d 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -3465,8 +3465,11 @@ func (c *Container) bitmapToArray() *Container { } // arrayToBitmap converts from array format to bitmap format. -func (c *Container) arrayToBitmap() *Container { +func (c *Container) arrayToBitmap() (out *Container) { statsHit("arrayToBitmap") + if roaringParanoia { + defer func() { out.CheckN() }() + } if c == nil { if roaringParanoia { panic("nil container for arrayToBitmap") @@ -3498,8 +3501,11 @@ func (c *Container) arrayToBitmap() *Container { } // runToBitmap converts from RLE format to bitmap format. -func (c *Container) runToBitmap() *Container { +func (c *Container) runToBitmap() (out *Container) { statsHit("runToBitmap") + if roaringParanoia { + defer func() { c.CheckN() }() + } if c == nil { if roaringParanoia { panic("nil container for runToBitmap") @@ -3725,6 +3731,9 @@ func (c *Container) runToArray() *Container { // Clone returns a copy of c. func (c *Container) Clone() (out *Container) { + if roaringParanoia { + defer func() { out.CheckN() }() + } statsHit("Container/Clone") if c == nil { return nil @@ -3735,8 +3744,9 @@ func (c *Container) Clone() (out *Container) { out = NewContainerArrayCopy(c.array()) case ContainerBitmap: statsHit("Container/Clone/Bitmap") - other := NewContainerBitmapN(nil, c.N()) + other := NewContainerBitmapN(nil, 0) copy(other.bitmap(), c.bitmap()) + other.n = c.n out = other case ContainerRun: statsHit("Container/Clone/Run") @@ -4098,7 +4108,10 @@ func intersectionCountBitmapBitmap(a, b *Container) (n int32) { return int32(popcountAndSlice(a.bitmap(), b.bitmap())) } -func intersect(a, b *Container) *Container { +func intersect(a, b *Container) (c *Container) { + if roaringParanoia { + defer func() { c.CheckN() }() + } if a.N() == MaxContainerVal+1 { return b.Freeze() } @@ -4320,7 +4333,10 @@ func intersectBitmapBitmap(a, b *Container) *Container { return output } -func union(a, b *Container) *Container { +func union(a, b *Container) (c *Container) { + if roaringParanoia { + defer func() { c.CheckN() }() + } if a.N() == MaxContainerVal+1 || b.N() == MaxContainerVal+1 { return fullContainer } @@ -5029,7 +5045,10 @@ func appendInterval16At(a []Interval16, val Interval16, off int) ([]Interval16, return a, off } -func difference(a, b *Container) *Container { +func difference(a, b *Container) (c *Container) { + if roaringParanoia { + defer func() { c.CheckN() }() + } if a.N() == 0 || b.N() == MaxContainerVal+1 { return nil } @@ -5386,7 +5405,10 @@ func differenceBitmapBitmap(a, b *Container) *Container { return output } -func xor(a, b *Container) *Container { +func xor(a, b *Container) (c *Container) { + if roaringParanoia { + defer func() { c.CheckN() }() + } if a.N() == 0 { return b.Freeze() } diff --git a/roaring/roaring_internal_test.go b/roaring/roaring_internal_test.go index 1a459fd8a..c5aa803ba 100644 --- a/roaring/roaring_internal_test.go +++ b/roaring/roaring_internal_test.go @@ -1895,9 +1895,10 @@ func TestWriteReadArray(t *testing.T) { func TestWriteReadBitmap(t *testing.T) { // create bitmap containing > 4096 bits - cb := NewContainerBitmapN(nil, 129*32) + cb := NewContainerBitmapN(nil, 0) for i := 0; i < 129; i++ { cb.bitmap()[i] = 0x5555555555555555 + cb.n += 32 } bb := NewFileBitmap() bb.Containers.Put(0, cb) @@ -1918,9 +1919,10 @@ func TestWriteReadBitmap(t *testing.T) { func TestWriteReadFullBitmap(t *testing.T) { // create bitmap containing > 4096 bits - cb := NewContainerBitmapN(nil, 65536) + cb := NewContainerBitmapN(nil, 0) for i := 0; i < bitmapN; i++ { cb.bitmap()[i] = 0xffffffffffffffff + cb.n += 64 } bb := NewFileBitmap() bb.Containers.Put(0, cb) diff --git a/roaring/roaring_nop_paranoia.go b/roaring/roaring_nop_paranoia.go index 6f2fa354b..d89ade062 100644 --- a/roaring/roaring_nop_paranoia.go +++ b/roaring/roaring_nop_paranoia.go @@ -17,3 +17,10 @@ package roaring const roaringParanoia = false + +// CheckN verifies that a container's cached count is correct, but +// there are two versions; this is the one which doesn't actually +// do anything, because the check is expensive. Which one you get is +// controlled by the roaringparanoia build tag. +func (c *Container) CheckN() { +} diff --git a/roaring/roaring_paranoia.go b/roaring/roaring_paranoia.go index 6f7d0c57e..1ba98ae29 100644 --- a/roaring/roaring_paranoia.go +++ b/roaring/roaring_paranoia.go @@ -16,4 +16,19 @@ package roaring +import "fmt" + const roaringParanoia = true + +// CheckN verifies that the container's cached count is correct. Note +// that this has two definitions, depending on the presence of the +// roaringparanoia build tag. +func (c *Container) CheckN() { + if c == nil { + return + } + count := c.count() + if count != c.n { + panic(fmt.Sprintf("CheckN (%p): n %d, count %d", c, c.n, count)) + } +}