diff --git a/cluster_internal_test.go b/cluster_internal_test.go index dad42d31f..fe519ed04 100644 --- a/cluster_internal_test.go +++ b/cluster_internal_test.go @@ -158,6 +158,7 @@ func TestFragSources(t *testing.T) { c5.addNodeBasicSorted(node3) idx := newIndexWithTempPath("i") + defer idx.Close() field, err := idx.CreateFieldIfNotExists("f", OptFieldTypeDefault()) if err != nil { t.Fatal(err) diff --git a/fragment.go b/fragment.go index 85d5cb385..852b11389 100644 --- a/fragment.go +++ b/fragment.go @@ -43,7 +43,6 @@ import ( "github.com/pilosa/pilosa/v2/roaring" "github.com/pilosa/pilosa/v2/shardwidth" "github.com/pilosa/pilosa/v2/stats" - "github.com/pilosa/pilosa/v2/syswrap" "github.com/pilosa/pilosa/v2/tracing" "github.com/pkg/errors" ) @@ -110,9 +109,8 @@ type fragment struct { // File-backed storage path string flags byte // user-defined flags passed to roaring - file *os.File + gen generation storage *roaring.Bitmap - storageData []byte totalOpN int64 // total opN values totalOps int64 // total ops (across all snapshots) opN int // number of ops since snapshot (may be approximate for imports) @@ -254,6 +252,10 @@ func (f *fragment) Open() error { // Fill cache with rows persisted to disk. f.Logger.Debugf("open cache for index/field/view/fragment: %s/%s/%s/%d", f.index, f.field, f.view, f.shard) if err := f.openCache(); err != nil { + e2 := f.closeStorage() + if e2 != nil { + return errors.Wrapf(err, "closing storage: %v, after opening cache", e2) + } return errors.Wrap(err, "opening cache") } @@ -273,48 +275,103 @@ func (f *fragment) Open() error { return nil } -func (f *fragment) reopen() (mustClose bool, err error) { - if f.file == nil { - // Open the data file to be mmap'd and used as an ops log. - f.file, mustClose, err = syswrap.OpenFile(f.path, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0666) - if err != nil { - return mustClose, fmt.Errorf("open file: %s", err) - } - f.storage.OpWriter = f.file +// emptyStorage is the common case for importStorage/applyStorage where they +// get no data. It tries to write the current storage to the provided file, +// which is assumed to be the file they didn't get any data from. +func (f *fragment) emptyStorage(file *os.File) (bool, error) { + // No data. We'll mark this for no mapping, clear any existing + // mapped containers, and set the Source to nil. We also have no + // ops. + f.opN = 0 + f.ops = 0 + f.storage.SetOps(0, 0) + + f.storage.PreferMapping(false) + _, err := f.storage.RemapRoaringStorage(nil) + f.storage.SetSource(nil) + if err != nil { + return false, fmt.Errorf("applying/importing storage: no data, and clearing old mapping also failed: %v", err) } - return mustClose, nil + // Write the existing storage out to the file so it's + // a valid Roaring file thereafter. nothing to unmarshal. + // In the unlikely event that this happened even though we + // had significant data, we're not mapping it, but that's + // harmless even if it's not maximally efficient. + bi := bufio.NewWriter(file) + if _, err = f.storage.WriteTo(bi); err != nil { + return false, fmt.Errorf("init storage file: %s", err) + } + bi.Flush() + return false, nil } -// openStorage opens the storage bitmap. Usually you also want to read in -// the storage, but in the case where we just wrote that file, such as -// unprotectedWriteToFragment, we could also just... not. If we didn't -// have existing storage, we probably need to unmarshal the data. If the -// file we're asked to open is empty, we probably don't. -// -// If we already had mapped storage previously, we want to unmap that, and -// possibly remap it from the file, but we don't need a full unmarshal, just -// an update of mapped pointers. -// -// unmarshalData is somewhat overloaded. it tells us whether or not we -// need to actually create a bitmap from the data (if the data exists to -// do this from). -// -// usually unmarshalData is only set to false when we're in the middle of -// a snapshot, and unprotectedWriteToFragment just wrote the in-memory data -// out. -// -// If we have existing storage data, and we successfully get new data, -// we will unmap the existing storage data. -// -// This function's design is probably a problem -- it is trying to handle -// both cases where there was existing data before, and cases where we -// just wrote the data. -func (f *fragment) openStorage(unmarshalData bool) error { - oldStorageData := f.storageData - // there's a few places where we might encounter an error, but need - // to continue past it through other error checks, before returning it. - var lastError error +// importStorage attempts to import data from storage -- for instance, +// reading in a roaring bitmap from media. +func (f *fragment) importStorage(data []byte, file *os.File, newGen generation, mapped bool) (bool, error) { + f.storage.PreferMapping(mapped) + if len(data) == 0 { + return f.emptyStorage(file) + } + // UnmarshalBinary will have remapped the storage to newGen if it + // succeeded, or if it fails but the error is advisory-only. So we + // optimistically set the source here, but if there's a non-advisory + // error, we'll unmap it and then set the source to nil. + f.storage.SetSource(newGen) + if err := f.storage.UnmarshalBinary(data); err != nil { + // roaring can report advisory-only errors... + cause := errors.Cause(err) + _, ok := cause.(roaring.AdvisoryError) + if !ok { + _, e2 := f.storage.RemapRoaringStorage(nil) + f.storage.SetSource(nil) + if e2 != nil { + return false, fmt.Errorf("unmarshal storage: file=%s, err=%s, clearing old mapping also failed: %v", file.Name(), err, e2) + } + return false, fmt.Errorf("unmarshal storage: file=%s, err=%s", file.Name(), err) + } + f.Logger.Printf("warning: unmarshal storage, file=%s, err=%v", file.Name(), err) + trunc, ok := cause.(roaring.FileShouldBeTruncatedError) + if ok { + // generation code looks for a FileShouldBeTruncatedError + return false, trunc + } + } + f.ops, f.opN = f.storage.Ops() + // For now, we assume that UnmarshalBinary will have mapped at least + // one container if we told it the storage was mapped and it didn't + // error out. This might be wrong in occasional trivial cases, but + // it should be harmless. + return mapped, nil +} + +// applyStorage applies storage to a fragment that may already have +// usable data. For instance, this would try to remap existing containers +// to use a new storage as backing store. +func (f *fragment) applyStorage(data []byte, file *os.File, newGen generation, mapped bool) (bool, error) { + if len(data) == 0 { + return f.emptyStorage(file) + } + // Tell storage to prefer mapping if and only if we think the data + // is mmapped and valid. + f.storage.PreferMapping(mapped) + f.storage.SetSource(newGen) + // RemapRoaringStorage will fix any mapped containers to point either + // to the provided data (if PreferMapping was called with true and + // data is provided and there's a corresponding container) or to + // allocated storage, so when it's done, there's nothing in it that + // is mapped to anything *other than* the provided data. + return f.storage.RemapRoaringStorage(data) +} + +// openStorage opens the storage bitmap. +// +// This has been massively reworked recently, and now hands a lot of +// file management off to the generation object and the Done method +// of that object. Similarly, the bitmap mapping/remapping +// logic is now mostly in importStorage (reading in a bitmap) and applyStorage +// (remapping an existing bitmap to match a new backing store). +func (f *fragment) openStorage(unmarshalData bool) error { // Create a roaring bitmap to serve as storage for the shard. if f.storage == nil { f.storage = roaring.NewFileBitmap() @@ -323,152 +380,23 @@ func (f *fragment) openStorage(unmarshalData bool) error { // unmarshal this data in order to have any. unmarshalData = true } - // Open the data file to be mmap'd and used as an ops log. - file, mustClose, err := syswrap.OpenFile(f.path, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0666) - if err != nil { - return fmt.Errorf("open file: %s", err) - } - f.file = file - if mustClose { - defer f.safeClose() - } - - // Lock the underlying file. - if err := syscall.Flock(int(f.file.Fd()), syscall.LOCK_EX|syscall.LOCK_NB); err != nil { - return fmt.Errorf("flock: %s", err) - } - - // data is the data we would unmarshal from, if we're unmarshalling; it might - // be obtained by calling ReadAll on a file. - // - // newStorageData is the data we should map things to. it is set only if - // mmapped; if we didn't mmap (say, we couldn't), we won't want to unmap - // the ioutil byte slice. (Theoretically, we shouldn't be using the mapped - // flag in that case...) - var data []byte - var newStorageData []byte - - // If the file is empty then initialize it with an empty bitmap. - fi, err := f.file.Stat() - if err != nil { - return errors.Wrap(err, "statting file before") - } else if fi.Size() == 0 { - bi := bufio.NewWriter(f.file) - var err error - if _, err = f.storage.WriteTo(bi); err != nil { - return fmt.Errorf("init storage file: %s", err) - } - bi.Flush() - _, err = f.file.Stat() - if err != nil { - return errors.Wrap(err, "statting file after") - } - // there's nothing here, we're not going to try to unmarshal it. - unmarshalData = false - f.rowCache = &simpleCache{make(map[uint64]*Row)} - } else { - // Mmap the underlying file so it can be zero copied. - data, err = syswrap.Mmap(int(f.file.Fd()), 0, int(fi.Size()), syscall.PROT_READ, syscall.MAP_SHARED) - if err == syswrap.ErrMaxMapCountReached { - f.Logger.Debugf("maximum number of maps reached, reading file instead") - if unmarshalData { - data, err = ioutil.ReadAll(file) - if err != nil { - return errors.Wrap(err, "failure file readall") - } - } - } else if err != nil { - return errors.Wrap(err, "mmap failed") - } else { - newStorageData = data - } - } - + f.rowCache = &simpleCache{make(map[uint64]*Row)} + var storageOp func([]byte, *os.File, generation, bool) (bool, error) if unmarshalData { - f.storageData = newStorageData - // We're about to either re-read the bitmap, or fail to do so - // and unconditionally unmap the existing stuff. Either way, we - // want to unmap the old storage data after we're done here, but - // we can't unmap it yet because it's still live until sometime - // later, but we can't unmap it later, because we could return - // early... this is what defer is for. - if oldStorageData != nil { - defer func() { - unmapErr := syswrap.Munmap(oldStorageData) - if unmapErr != nil { - f.Logger.Printf("unmap of old storage failed: %s", err) - } - }() - } - // set the preference for mapping based on whether the data's mmapped - f.storage.PreferMapping(newStorageData != nil) - // so we have a problem here: if this fails, it's unclear whether - // *either* or *both* of old and new storage data might be in use. - // So we call the thing that should unconditionally unmap both of them... - if err := f.storage.UnmarshalBinary(data); err != nil { - // roaring can report advisory-only errors... - _, ok := err.(roaring.AdvisoryError) - if !ok { - name := f.file.Name() - f.file.Close() - f.file = nil - _, e2 := f.storage.RemapRoaringStorage(nil) - if e2 != nil { - return fmt.Errorf("unmarshal storage: file=%s, err=%s, clearing old mapping also failed: %v", name, err, e2) - } - return fmt.Errorf("unmarshal storage: file=%s, err=%s", name, err) - } else { - f.Logger.Printf("warning: unmarshal storage, file=%s, err=%v", f.file.Name(), err) - } - trunc, ok := err.(roaring.FileShouldBeTruncatedError) - if ok { - f.Logger.Printf("should probably truncate file %s to %d bytes, but can't yet", f.file.Name(), trunc.SuggestedLength()) - } - } - f.rowCache = &simpleCache{make(map[uint64]*Row)} - f.ops, f.opN = f.storage.Ops() + storageOp = f.importStorage } else { - // we're moving to new storage, so instead of using the OpN - // derived from reading that storage, we notify the bitmap that - // OpN is now effectively zero. - f.opN = 0 - f.ops = 0 - f.storage.SetOps(0, 0) - // if oldStorageData is nil, this just tries to unmap any bits that - // are currently mapped. otherwise, it will point them at this - // storage (if the containers match). - var mappedAny bool - mappedAny, lastError = f.storage.RemapRoaringStorage(newStorageData) - if oldStorageData != nil { - unmapErr := syswrap.Munmap(oldStorageData) - if unmapErr != nil { - f.Logger.Printf("unmap of old storage failed: %s", err) - } - } - if mappedAny { - // Advise the kernel that the mmap is accessed randomly. - if err := madvise(newStorageData, syscall.MADV_RANDOM); err != nil { - lastError = fmt.Errorf("madvise: %s", err) - } - } else { - // if we did map data, but for some reason none of it got used - // as backing store, we can unmap it, and set the slice to nil, - // so we don't keep the now-invalid slice in f.storageData. - if newStorageData != nil { - unmapErr := syswrap.Munmap(newStorageData) - if unmapErr != nil { - lastError = fmt.Errorf("unmapping unused storage data: %s", err) - } - newStorageData = nil - } - } - f.storageData = newStorageData + storageOp = f.applyStorage } - - // Attach the file to the bitmap to act as a write-ahead log. - f.storage.OpWriter = f.file - - return lastError + var err error + f.gen, err = newGeneration(f.gen, f.path, unmarshalData, storageOp, f.Logger) + if generationDebug { + // We might have already done this anyway, if we think we + // mapped stuff, but when debugging we want to do it + // unconditionally, because the test cases otherwise won't + // exercise this code well. + f.storage.SetSource(f.gen) + } + return err } // openCache initializes the cache from row ids persisted to disk. @@ -550,7 +478,7 @@ func (f *fragment) close() error { } // Close underlying storage. - if err := f.closeStorage(true); err != nil { + if err := f.closeStorage(); err != nil { f.Logger.Printf("fragment: error closing storage: err=%s, path=%s", err, f.path) return errors.Wrap(err, "closing storage") } @@ -561,54 +489,17 @@ func (f *fragment) close() error { return nil } -// safeClose is unprotected. -func (f *fragment) safeClose() error { - // Flush file, unlock & close. - if f.file != nil { - if err := f.file.Sync(); err != nil { - return fmt.Errorf("sync: %s", err) - } - if err := syscall.Flock(int(f.file.Fd()), syscall.LOCK_UN); err != nil { - return fmt.Errorf("unlock: %s", err) - } - if err := syswrap.CloseFile(f.file); err != nil { - return fmt.Errorf("close file: %s", err) - } - } - f.file = nil - f.storage.OpWriter = nil - - return nil -} - -// closeStorage attempts to close storage, including unmapping the old -// storage if includeMap is true. This would normally make sense if you're -// expecting to be done using the fragment, or to reload it. But it's also -// okay to just leave stuff mmapped; you don't have to keep the file -// descriptor open. So in some cases, we'll just leave the old mmapping -// in place, rather than regenerating everything from the new file. -func (f *fragment) closeStorage(includeMap bool) error { - // Clear the storage bitmap so it doesn't access the closed mmap. - - //f.storage = roaring.NewBitmap() - - // Unmap the file. - if includeMap && f.storageData != nil { - if err := syswrap.Munmap(f.storageData); err != nil { - return fmt.Errorf("munmap: %s", err) - } - f.storageData = nil - } - - if err := f.safeClose(); err != nil { - return err - } - +// closeStorage marks the current generation as done. It is not necessary +// to call this before openStorage. +func (f *fragment) closeStorage() error { // opN is determined by how many bit set/clear operations are in the storage // write log, so once the storage is closed it should be 0. Opening new // storage will set opN appropriately. f.opN = 0 + if f.gen != nil { + f.gen.Done() + } return nil } @@ -661,22 +552,17 @@ func (f *fragment) rowFromStorage(rowID uint64) *Row { func (f *fragment) setBit(rowID, columnID uint64) (changed bool, err error) { f.mu.Lock() defer f.mu.Unlock() - mustClose, err := f.reopen() - if err != nil { - return false, errors.Wrap(err, "reopening") - } - if mustClose { - defer f.safeClose() - } - - // handle mutux field type - if f.mutexVector != nil { - if err := f.handleMutex(rowID, columnID); err != nil { - return changed, errors.Wrap(err, "handling mutex") + err = f.gen.Transaction(&f.storage.OpWriter, func() error { + // handle mutux field type + if f.mutexVector != nil { + if err := f.handleMutex(rowID, columnID); err != nil { + return errors.Wrap(err, "handling mutex") + } } - } - - return f.unprotectedSetBit(rowID, columnID) + changed, err = f.unprotectedSetBit(rowID, columnID) + return err + }) + return changed, err } // handleMutex will clear an existing row and store the new row @@ -740,17 +626,14 @@ func (f *fragment) unprotectedSetBit(rowID, columnID uint64) (changed bool, err // clearBit clears a bit for a given column & row within the fragment. // This updates both the on-disk storage and the in-cache bitmap. -func (f *fragment) clearBit(rowID, columnID uint64) (bool, error) { +func (f *fragment) clearBit(rowID, columnID uint64) (changed bool, err error) { f.mu.Lock() defer f.mu.Unlock() - mustClose, err := f.reopen() - if err != nil { - return false, errors.Wrap(err, "reopening") - } - if mustClose { - defer f.safeClose() - } - return f.unprotectedClearBit(rowID, columnID) + err = f.gen.Transaction(&f.storage.OpWriter, func() error { + changed, err = f.unprotectedClearBit(rowID, columnID) + return err + }) + return changed, err } // unprotectedClearBit TODO should be replaced by an invocation of @@ -796,17 +679,14 @@ func (f *fragment) unprotectedClearBit(rowID, columnID uint64) (changed bool, er // setRow replaces an existing row (specified by rowID) with the given // Row. This updates both the on-disk storage and the in-cache bitmap. -func (f *fragment) setRow(row *Row, rowID uint64) (bool, error) { +func (f *fragment) setRow(row *Row, rowID uint64) (changed bool, err error) { f.mu.Lock() defer f.mu.Unlock() - mustClose, err := f.reopen() - if err != nil { - return false, errors.Wrap(err, "reopening") - } - if mustClose { - defer f.safeClose() - } - return f.unprotectedSetRow(row, rowID) + err = f.gen.Transaction(&f.storage.OpWriter, func() error { + changed, err = f.unprotectedSetRow(row, rowID) + return err + }) + return changed, err } func (f *fragment) unprotectedSetRow(row *Row, rowID uint64) (changed bool, err error) { @@ -855,17 +735,14 @@ func (f *fragment) unprotectedSetRow(row *Row, rowID uint64) (changed bool, err // ClearRow clears a row for a given rowID within the fragment. // This updates both the on-disk storage and the in-cache bitmap. -func (f *fragment) clearRow(rowID uint64) (bool, error) { +func (f *fragment) clearRow(rowID uint64) (changed bool, err error) { f.mu.Lock() defer f.mu.Unlock() - mustClose, err := f.reopen() - if err != nil { - return false, errors.Wrap(err, "reopening") - } - if mustClose { - defer f.safeClose() - } - return f.unprotectedClearRow(rowID) + err = f.gen.Transaction(&f.storage.OpWriter, func() error { + changed, err = f.unprotectedClearRow(rowID) + return err + }) + return changed, err } func (f *fragment) unprotectedClearRow(rowID uint64) (changed bool, err error) { @@ -991,67 +868,62 @@ func (f *fragment) positionsForValue(columnID uint64, bitDepth uint, value int64 func (f *fragment) setValueBase(columnID uint64, bitDepth uint, value int64, clear bool) (changed bool, err error) { f.mu.Lock() defer f.mu.Unlock() - mustClose, err := f.reopen() - if err != nil { - return false, errors.Wrap(err, "reopening") - } - if mustClose { - defer f.safeClose() - } + err = f.gen.Transaction(&f.storage.OpWriter, func() error { + // Convert value to an unsigned representation. + uvalue := uint64(value) + if value < 0 { + uvalue = uint64(-value) + } - // Convert value to an unsigned representation. - uvalue := uint64(value) - if value < 0 { - uvalue = uint64(-value) - } + for i := uint(0); i < bitDepth; i++ { + if uvalue&(1<= 0 || clear { + if c, err := f.unprotectedClearBit(uint64(bsiSignBit), columnID); err != nil { + return errors.Wrap(err, "clearing sign") + } else if c { + changed = true + } + } else { + if c, err := f.unprotectedSetBit(uint64(bsiSignBit), columnID); err != nil { + return errors.Wrap(err, "marking sign") + } else if c { + changed = true + } } - } else { - if c, err := f.unprotectedSetBit(uint64(bsiExistsBit), columnID); err != nil { - return changed, errors.Wrap(err, "marking not-null") - } else if c { - changed = true - } - } - // Mark sign bit (or clear). - if value >= 0 || clear { - if c, err := f.unprotectedClearBit(uint64(bsiSignBit), columnID); err != nil { - return changed, errors.Wrap(err, "clearing sign") - } else if c { - changed = true - } - } else { - if c, err := f.unprotectedSetBit(uint64(bsiSignBit), columnID); err != nil { - return changed, errors.Wrap(err, "marking sign") - } else if c { - changed = true - } - } - - return changed, nil + return nil + }) + return changed, err } // importSetValue is a more efficient SetValue just for imports. @@ -2079,52 +1951,46 @@ func (f *fragment) bulkImportStandard(rowIDs, columnIDs []uint64, options *Impor // snapshot of the fragment or just do in-memory updates while appending // operations to the op log. func (f *fragment) importPositions(set, clear []uint64, rowSet map[uint64]struct{}) error { - mustClose, err := f.reopen() - if err != nil { - return errors.Wrap(err, "reopening") - } - if mustClose { - defer f.safeClose() - } - - if len(set) > 0 { - f.stats.Count("ImportingN", int64(len(set)), 1) - changedN, err := f.storage.AddN(set...) // TODO benchmark Add/RemoveN behavior with sorted/unsorted positions - if err != nil { - return errors.Wrap(err, "adding positions") + err := f.gen.Transaction(&f.storage.OpWriter, func() error { + if len(set) > 0 { + f.stats.Count("ImportingN", int64(len(set)), 1) + changedN, err := f.storage.AddN(set...) // TODO benchmark Add/RemoveN behavior with sorted/unsorted positions + if err != nil { + return errors.Wrap(err, "adding positions") + } + f.stats.Count("ImportedN", int64(changedN), 1) + f.incrementOpN(changedN) } - f.stats.Count("ImportedN", int64(changedN), 1) - f.incrementOpN(changedN) - } - if len(clear) > 0 { - f.stats.Count("ClearingN", int64(len(clear)), 1) - changedN, err := f.storage.RemoveN(clear...) - if err != nil { - return errors.Wrap(err, "clearing positions") + if len(clear) > 0 { + f.stats.Count("ClearingN", int64(len(clear)), 1) + changedN, err := f.storage.RemoveN(clear...) + if err != nil { + return errors.Wrap(err, "clearing positions") + } + f.stats.Count("ClearedN", int64(changedN), 1) + f.incrementOpN(changedN) } - f.stats.Count("ClearedN", int64(changedN), 1) - f.incrementOpN(changedN) - } - // Update cache counts for all affected rows. - for rowID := range rowSet { - // Invalidate block checksum. - delete(f.checksums, int(rowID/HashBlockSize)) + // Update cache counts for all affected rows. + for rowID := range rowSet { + // Invalidate block checksum. + delete(f.checksums, int(rowID/HashBlockSize)) + + if f.CacheType != CacheTypeNone { + n := f.storage.CountRange(rowID*ShardWidth, (rowID+1)*ShardWidth) + f.cache.BulkAdd(rowID, n) + } + + f.rowCache.Add(rowID, nil) + } if f.CacheType != CacheTypeNone { - n := f.storage.CountRange(rowID*ShardWidth, (rowID+1)*ShardWidth) - f.cache.BulkAdd(rowID, n) + f.cache.Recalculate() } - - f.rowCache.Add(rowID, nil) - } - - if f.CacheType != CacheTypeNone { - f.cache.Recalculate() - } - - return nil + return nil + }) + return err } // bulkImportMutex performs a bulk import on a fragment while ensuring @@ -2210,7 +2076,6 @@ func (f *fragment) importValueSmallWrite(columnIDs []uint64, values []int64, bit } return nil }(); err != nil { - _ = f.closeStorage(true) _ = f.openStorage(true) return err } @@ -2258,7 +2123,6 @@ func (f *fragment) importValue(columnIDs []uint64, values []int64, bitDepth uint } return nil }(); err != nil { - _ = f.closeStorage(true) _ = f.openStorage(true) return err } @@ -2289,7 +2153,13 @@ func (f *fragment) importRoaring(ctx context.Context, data []byte, clear bool) e defer f.mu.Unlock() span.Finish() span, ctx = tracing.StartSpanFromContext(ctx, "importRoaring.ImportRoaringBits") - changed, rowSet, err := f.storage.ImportRoaringBits(data, clear, true, rowSize) + var changed int + var rowSet map[uint64]int + err := f.gen.Transaction(&f.storage.OpWriter, func() (err error) { + changed, rowSet, err = f.storage.ImportRoaringBits(data, clear, true, rowSize) + return err + }) + span.Finish() if err != nil { return err @@ -2383,22 +2253,24 @@ func unprotectedWriteToFragment(f *fragment, bm *roaring.Bitmap) (n int64, err e if err != nil { return n, fmt.Errorf("create snapshot file: %s", err) } - defer file.Close() + // No deferred close, because we want to close it sooner than the + // end of this function. // Write storage to snapshot. bw := bufio.NewWriter(file) if n, err = bm.WriteTo(bw); err != nil { + file.Close() return n, fmt.Errorf("snapshot write to: %s", err) } if err := bw.Flush(); err != nil { + file.Close() return n, fmt.Errorf("flush: %s", err) } - // Close current storage. - if err := f.closeStorage(false); err != nil { - return n, fmt.Errorf("close storage: %s", err) - } + // we close the file here so we don't still have it open when trying + // to open it in a moment. + file.Close() // Move snapshot to data file location. if err := os.Rename(snapshotPath, f.path); err != nil { @@ -2598,11 +2470,6 @@ func (f *fragment) readStorageFromArchive(r io.Reader) error { return errors.Wrap(err, "copying") } - // Close current storage. - if err := f.closeStorage(true); err != nil { - return errors.Wrap(err, "closing") - } - // Move snapshot to data file location. if err := os.Rename(path, f.path); err != nil { return errors.Wrap(err, "renaming") diff --git a/fragment_internal_test.go b/fragment_internal_test.go index 1582ecd3c..13d47181b 100644 --- a/fragment_internal_test.go +++ b/fragment_internal_test.go @@ -1397,6 +1397,7 @@ func TestFragment_WriteTo_ReadFrom(t *testing.T) { // Read into another fragment. f1 := mustOpenFragment("i", "f", viewStandard, 0, "") + defer f1.Clean(t) if rn, err := f1.ReadFrom(&buf); err != nil { t.Fatal(err) } else if wn != rn { @@ -2233,7 +2234,20 @@ func BenchmarkImportRoaringUpdate(b *testing.B) { b.Errorf("import error: %v", err) } b.StopTimer() - stat, _ := f.file.Stat() + var stat os.FileInfo + var statTarget io.Writer + err = f.gen.Transaction(&statTarget, func() error { + targetFile, ok := statTarget.(*os.File) + if ok { + stat, _ = targetFile.Stat() + } else { + b.Errorf("couldn't stat file") + } + return nil + }) + if err != nil { + b.Errorf("transaction error: %v", err) + } fileSize[name] = stat.Size() f.Clean(b) } @@ -2382,6 +2396,7 @@ func TestGetZipfRowsSliceRoaring(t *testing.T) { t.Fatalf("suspect distribution from getZipfRowsSliceRoaring") } } + f.Clean(t) } // getZipfRowsSliceRoaring generates a random fragment with the given number of @@ -2529,7 +2544,14 @@ func (f *fragment) sanityCheck(t testing.TB) { func (f *fragment) Clean(t testing.TB) { f.awaitSnapshot() f.sanityCheck(t) + if f.storage != nil && f.storage.Source != nil { + if f.storage.Source.Dead() { + t.Fatalf("cleaning up fragment %s, source %s, source already dead", f.path, f.storage.Source.ID()) + } + } errc := f.Close() + // prevent double-closes of generation during testing. + f.gen = nil errf := os.Remove(f.path) errp := os.Remove(f.cachePath()) if errc != nil || errf != nil { @@ -3272,7 +3294,7 @@ func TestImportClearRestart(t *testing.T) { f2.MaxOpN = maxOpN f2.CacheType = f.CacheType - err = f.closeStorage(true) + err = f.closeStorage() if err != nil { t.Fatalf("closing storage: %v", err) } @@ -3306,7 +3328,7 @@ func TestImportClearRestart(t *testing.T) { f3.MaxOpN = maxOpN f3.CacheType = f.CacheType - err = f2.closeStorage(true) + err = f2.closeStorage() if err != nil { t.Fatalf("f2 closing storage: %v", err) } @@ -3354,6 +3376,7 @@ func check(t *testing.T, f *fragment, exp map[uint64]map[uint64]struct{}) { func TestImportValueConcurrent(t *testing.T) { f := mustOpenBSIFragment("i", "f", viewBSIGroupPrefix+"foo", 0) + defer f.Clean(t) eg := &errgroup.Group{} for i := 0; i < 4; i++ { i := i diff --git a/generation.go b/generation.go new file mode 100644 index 000000000..34bf2b18b --- /dev/null +++ b/generation.go @@ -0,0 +1,407 @@ +// Copyright 2019 Pilosa Corp. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package pilosa + +import ( + "fmt" + "io" + "io/ioutil" + "os" + "runtime" + "sync" + "syscall" + "time" + + "github.com/pilosa/pilosa/v2/logger" + "github.com/pilosa/pilosa/v2/roaring" + "github.com/pilosa/pilosa/v2/syswrap" + "github.com/pkg/errors" +) + +// generation represents one "generation" of opening a data file. +// This is what determines when it's safe to unmap a data file, if it +// got mapped, and handles closing/reopening files if we need to +// manage file handle availability. It's an interface because this +// lets us write simpler code for specific cases, rather than handling +// the whole matrix of mapped/unmapped, staying open/being reopened, +// etcetera. +// +// You create a generation by calling newGeneration with a file +// path. If it succeeds in opening that path, it calls a provided +// setup function with the data from the generation, and a flag +// indicating whether the data is mmapped. If the setup function +// fails, newGeneration cleans things up and closes. Otherwise, +// it returns a generation. +// +// The generation itself uses runtime.SetFinalizer to clean up when +// the last reference to it goes away. You should store a pointer +// to the generation in any object which is reliant on the generation. +// +// When you anticipate a generation should be done (for instance, +// opening a new generation), the old one gets marked done, which +// stashes a timestamp in it. Later operations can check whether +// the timestamp is a while back, and if so, complain that something +// might be wrong. +// +// In some cases, we don't have enough open file limit to keep every +// file actually open. To address this, use the `Transaction` function, +// which ensures that the file is open, stores a reference to it in +// a provided `*io.Writer`, and then restores the previous value of +// the io.Writer when it's done. For instance, for a bitmap, this might +// be used with `&b.OpWriter`. +// +// newGeneration takes an optional previous generation; it calls +// that generation's Done function after running the provided setup, +// and bumps the generation count. +type generation interface { + // Transaction runs the given transaction with the generation's + // file open. If the **os.File parameter is + // non-nil, the generation's file will be open, and stored + // into that pointer, during the execution of func, after + // which the previous contents are restored. Otherwise + // the file may or may not be open during the operation. + Transaction(*io.Writer, func() error) error + // Done() should be called exactly once, to indicate that a + // generation is expected not to be in use for long -- for instance, + // when a new generation replaces it. + Done() + // Generation count. + Generation() int64 + // ID indicates the source -- path and generation number -- that + // this generation represents. + ID() string + Dead() bool +} + +type mmapGeneration struct { + mu sync.Mutex // mutex guards modifiers of generation, not of data + transMu sync.Mutex // guards transactions, specifically + path string + id string + file *os.File + data []byte + generation int64 // generation counter + dead bool // we think this generation is dead + deadSince time.Time // when this generation was marked dead + retries int // for cases where we're retrying + logger logger.Logger +} + +func (m *mmapGeneration) Dead() bool { + m.mu.Lock() + defer m.mu.Unlock() + return m.dead +} + +func (m *mmapGeneration) ID() string { + return m.id +} + +func (m *mmapGeneration) Generation() int64 { + return m.generation +} + +// Transaction runs an exclusive call, ensuring that the file is open if +// the *io.Writer parameter is present. +func (m *mmapGeneration) Transaction(fileP *io.Writer, fn func() error) (transactionErr error) { + m.transMu.Lock() + defer m.transMu.Unlock() + // HEY LOOK CAREFULLY AT THIS BIT: + // We can't just defer this unlock. We specifically want to be + // sure to unlock the regular mutex *before* this function is over, + // and if we error out trying to open the file, we want to do it + // even sooner. If we deferred this, the transaction would block + // *everything*, including things like sanity checks against the + // generation being Dead(), but also including the deferred + // re-close-the-file. + m.mu.Lock() + // if we've been asked for a file pointer, we need to ensure that + // our file is open, and that the file pointer to it is stored in + // the requested location, then revert that when we're done. + // if we aren't asked for a file pointer, nothing needs the file + // open. + if m.dead { + elapsed := time.Since(m.deadSince) + m.logger.Printf("WARNING: transaction against %s, which has been dead for %v\n", m.id, elapsed) + } + if fileP != nil { + if m.file == nil { + // we ignore the shouldClose response here; if this + // fragment was previously not being kept open, we're + // going to stick with that. + _, err := m.openFile() + if err != nil { + m.mu.Unlock() + return err + } + defer func() { + // report a close error if we have no other error to report + m.mu.Lock() + defer m.mu.Unlock() + err := m.closeFile() + if transactionErr == nil { + transactionErr = err + } + }() + } + var fileStash io.Writer + fileStash, *fileP = *fileP, m.file + defer func() { + *fileP = fileStash + }() + } + // We are done locking the generation itself for now. + m.mu.Unlock() + return fn() +} + +// Done marks the generation done, and closes its file, but may not unmap it. +// It's still conceptually possible to end up doing a Transaction against a +// done generation, but it's a red flag. +func (m *mmapGeneration) Done() { + if m == nil { + return + } + m.mu.Lock() + defer m.mu.Unlock() + if m.dead { + oops := fmt.Sprintf("generation %s, marked done again at %v, previously marked dead at %v", + m.id, time.Now(), m.deadSince) + panic(oops) + } + m.dead = true + m.deadSince = time.Now() + err := m.closeFile() + if err != nil { + m.logger.Printf("error closing generation %s: %v", m.id, err) + } + // If we're not debugging, the finalizer won't have been enabled + // previously. Finalizers have non-zero cost, so having them not be + // created until they're needed seems rewarding? + if !generationDebug { + runtime.SetFinalizer(m, generationFinalizer) + } + endGeneration(m.id) + // note, Done() doesn't close the file; only the finalizer actually + // does the shutdown. +} + +// Try to close the file if it's currently open. +func (m *mmapGeneration) closeFile() error { + var lastErr error + // report the most serious error encountered, but still close + // file even if something else failed. + if m.file != nil { + if err := m.file.Sync(); err != nil { + lastErr = fmt.Errorf("sync: %s", err) + } + if err := syscall.Flock(int(m.file.Fd()), syscall.LOCK_UN); err != nil { + lastErr = fmt.Errorf("unlock: %s", err) + } + if err := syswrap.CloseFile(m.file); err != nil { + lastErr = fmt.Errorf("close file: %s", err) + } + m.file = nil + } + return lastErr +} + +// openFile ensures the file is open and locked, or fails. If it does +// open the file, it will also report the "you need to close this file +// when you're done" flag from syswrap. +func (m *mmapGeneration) openFile() (shouldClose bool, err error) { + if m.file != nil { + return false, nil + } + m.file, shouldClose, err = syswrap.OpenFile(m.path, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0666) + if err != nil { + return false, err + } + // do we actually want this in every openFile? I don't know. + if err := syscall.Flock(int(m.file.Fd()), syscall.LOCK_EX|syscall.LOCK_NB); err != nil { + m.file.Close() + m.file = nil + return false, fmt.Errorf("flock: %s", err) + } + return shouldClose, nil +} + +func generationFinalizer(m *mmapGeneration) { + m.mu.Lock() + if !m.dead { + m.logger.Printf("finalizing generation %s which isn't dead yet\n", + m.id) + } + m.mu.Unlock() + err := m.closeFile() + if err != nil { + m.logger.Printf("finalizing generation, closing file: %v\n", err) + } + if m.data != nil { + err := syswrap.Munmap(m.data) + if err != nil { + m.logger.Printf("finalizing generation, munmap: %v\n", err) + } + m.data = nil + } + finalizeGeneration(m.id) +} + +// Cancel closes a generation out entirely. It cancels any finalizer, +// unmaps any data, ends generation tracking, and closes any files. +// It does each of these separately whether or not the others need to be done, +// or succeed. It's used to handle failures from newGeneration; it makes sure +// the generation isn't holding any resources and doesn't need to be cleaned +// up otherwise. +// +// Mostly a helper function because there's several cases where newGeneration +// might fail. +func (m *mmapGeneration) Cancel() { + if m.data != nil { + _ = syswrap.Munmap(m.data) + m.data = nil + } + err := m.closeFile() + if err != nil { + m.logger.Printf("error cancelling generation %s: %v", m.id, err) + } + runtime.SetFinalizer(m, nil) + m.dead = true + m.deadSince = time.Now() + cancelGeneration(m.id) +} + +// newGeneration creates a new generation using the given file path. It +// then calls the provided setup function with the allocated storage, a +// file handle, the new generation, and a flag indicatting whether the storage +// is memory-mapped. If the setup function returns a non-nil error, the +// generation is cleaned up, and newGeneration fails. The setup function +// also returns a boolean indicating whether it used the mapping; if it +// didn't, newGeneration discards the mapping and returns a nil generation. +// +// If generationDebug is enabled, we track the generation even if no mapping +// is actually in use, so we can verify that the tracking is working. +// +// On failure, newGeneration returns nil values for generation and func, +// and an error. On success, the func returned is the close func to use +// when the generation is no longer needed by the caller. +func newGeneration(existing generation, path string, readData bool, setup func([]byte, *os.File, generation, bool) (bool, error), logger logger.Logger) (generation, error) { + m := mmapGeneration{path: path, logger: logger} + if existing != nil { + m.generation = existing.Generation() + 1 + // we might keep a previous generation around just for its generation count. + if !existing.Dead() { + defer existing.Done() + } + } + shouldClose, err := m.openFile() + if err != nil { + return nil, err + } + m.id = fmt.Sprintf("%s:%d", m.path, m.generation) + // possibly assign new generation ID if this one's been used, which can + // happen with reopens, especially during testing. + m.id = registerGeneration(m.id) + // if debugging, we always want the finalizer on so we notice if a + // generation is finalized without being closed. for non-debugging + // use, we only need it when the generation is closed. + if generationDebug { + runtime.SetFinalizer(&m, generationFinalizer) + } + // Mmap the underlying file so it can be zero copied. + var mapped bool + var data []byte + fi, err := m.file.Stat() + if err == nil && fi.Size() > 0 { + data, err = syswrap.Mmap(int(m.file.Fd()), 0, int(fi.Size()), syscall.PROT_READ, syscall.MAP_SHARED) + if err == syswrap.ErrMaxMapCountReached { + // I have no idea where/how to display this message. + m.logger.Printf("maximum number of maps reached, reading file '%s' instead", m.path) + } else if err != nil { + m.Cancel() + return nil, errors.Wrap(err, "mmap failed") + } else { + mapped = true + } + } + if data == nil && readData { + data, err = ioutil.ReadAll(m.file) + if err != nil { + m.Cancel() + return nil, errors.Wrap(err, "failure file readall") + } + } + // if we got here, data's the expected data, so let's try to use it + mappedAny, err := setup(data, m.file, &m, mapped) + + // if the setup failed, we unmap data if we previously mapped it, + // and exit. Note that having no data, or having only trivial + // data (like a zero-container Roaring file) isn't "failed". + if err != nil { + m.Cancel() + // Unless, that is, we think the file probably ought to + // be truncated: For instance, if a bitmap has a corrupted + // ops log, we could truncate that part of it and retry. + if err, ok := err.(roaring.FileShouldBeTruncatedError); ok && m.retries < 1 { + m.logger.Printf("file %s read partially, but should-be-truncated at %d bytes\n", m.path, err.SuggestedLength()) + // close this generation, then try again. once. + m.retries++ + err := os.Truncate(m.path, err.SuggestedLength()) + if err != nil { + m.logger.Printf("truncating file failed [but retrying anyway]: %v\n", err) + } + return newGeneration(&m, path, readData, setup, logger) + } + return nil, err + } + + if mapped { + // when generationDebug is on, we want to track this even + // if it's not being used. + if generationDebug || mappedAny { + // Advise the kernel that the mmap is accessed randomly. + // We don't care much about errors with this. + _ = madvise(data, syscall.MADV_RANDOM) + // store the data, so we can unmap it when this generation + // gets finalized. + m.data = data + } else { + // unmap the data and don't stash the pointer in this + // generation. It's not being used. This generation + // doesn't need to exist, yay. + unmapErr := syswrap.Munmap(data) + if unmapErr != nil { + m.logger.Printf("error unmapping (probably harmless): %v", unmapErr) + } + } + } + // shouldClose comes from underlying syswrap.OpenFile, which checks + // a count of open files to hint at us when we need to start closing + // files to preserve open file descriptor limit. + if shouldClose { + err := m.closeFile() + if err != nil { + m.logger.Printf("closing file to preserve open files failed: %v\n", err) + } + } + // It's possible that the generation has no actual data to track, + // because nothing's mapped, in which case there won't be any bitmap + // sources following this, just the fragment source. (Bitmaps won't + // be attached to the source unless they're actually mapped to it, + // or generationDebug is true). That's okay. We pay a tiny cost + // for the finalizer, but we also get higher confidence that it really + // does get cleaned up. + return &m, nil +} diff --git a/generation_debug.go b/generation_debug.go new file mode 100644 index 000000000..a425deabe --- /dev/null +++ b/generation_debug.go @@ -0,0 +1,160 @@ +// Copyright 2019 Pilosa Corp. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// +build generationdebug + +package pilosa + +import ( + "fmt" + "math/rand" + "runtime" + "sort" + "sync" + "time" +) + +const generationDebug = true + +type lifespan struct { + from, to, finalized time.Time +} + +var knownGenerations map[string]lifespan +var knownGenerationLock sync.Mutex + +var timeZero time.Time + +func registerGeneration(id string) string { + knownGenerationLock.Lock() + defer knownGenerationLock.Unlock() + if knownGenerations == nil { + knownGenerations = make(map[string]lifespan) + } + newSpan := lifespan{from: time.Now()} + origId := id + + // if you have more than 65k of the same file open, maybe you have bigger + // problems than this. + for span, exists := knownGenerations[id]; exists; span, exists = knownGenerations[id] { + suffix := fmt.Sprintf("::%04x", rand.Int63n(65536)) + if span.finalized != timeZero { + fmt.Printf("new generation %s: adding %s, previously existed, created %v, died %v, finalized %v\n", + id, suffix, span.from, span.to, span.finalized) + } else { + if span.to != timeZero { + fmt.Printf("new generation %s: adding %s, previously existed, created %v, died %v\n", id, suffix, span.from, span.to) + } else { + fmt.Printf("new generation %s: adding %s, already exists, created %v", id, suffix, span.from) + } + } + id = origId + suffix + } + fmt.Printf("new generation %s\n", id) + knownGenerations[id] = newSpan + return id +} + +func endGeneration(id string) { + knownGenerationLock.Lock() + defer knownGenerationLock.Unlock() + span, exists := knownGenerations[id] + if !exists { + oops := fmt.Sprintf("ending generation %s: unknown", id) + panic(oops) + } + if span.finalized != timeZero || span.to != timeZero { + oops := fmt.Sprintf("ending generation %s: already died at %v, finalized at %v", id, span.to, span.finalized) + panic(oops) + } + span.to = time.Now() + knownGenerations[id] = span +} + +// cancelGeneration marks the generation as finalized. In principle it's +// only used in cases where we just started a generation but something +// went wrong. it's not fancier than this because of the weird cases +// where the same generation shows up again, such as when closing and +// reopening an index so we don't know about previous instances of the +// same files. +func cancelGeneration(id string) { + knownGenerationLock.Lock() + defer knownGenerationLock.Unlock() + span, exists := knownGenerations[id] + if exists { + span.finalized = time.Now() + span.to = span.finalized + knownGenerations[id] = span + } +} + +func finalizeGeneration(id string) { + knownGenerationLock.Lock() + defer knownGenerationLock.Unlock() + span, exists := knownGenerations[id] + if !exists { + oops := fmt.Sprintf("finalizing generation %s: unknown", id) + panic(oops) + } + if span.finalized != timeZero { + var oops string + if span.to != timeZero { + oops = fmt.Sprintf("finalizing generation %s: already finalized at %v, but not dead", id, span.finalized) + } else { + oops = fmt.Sprintf("finalizing generation %s: already finalized at %v, dead at %v", id, span.finalized, span.to) + } + panic(oops) + } + span.finalized = time.Now() + knownGenerations[id] = span +} + +func reportGenerations() []string { + runtime.GC() + knownGenerationLock.Lock() + defer knownGenerationLock.Unlock() + var surviving []string + times := make([]int64, 0, len(knownGenerations)) + for id, span := range knownGenerations { + if span.to == timeZero { + if span.finalized == timeZero { + surviving = append(surviving, fmt.Sprintf("%s: %v, not ended or finalized", id, span.from)) + } else { + surviving = append(surviving, fmt.Sprintf("%s: %v, finalized %v, not ended", id, span.from, span.finalized)) + } + } else { + if span.finalized == timeZero { + surviving = append(surviving, fmt.Sprintf("%s: %v to %v, not finalized", id, span.from, span.to)) + } else { + times = append(times, int64(span.finalized.Sub(span.to))) + } + } + } + if len(times) > 0 { + sort.Slice(times, func(i, j int) bool { return times[i] < times[j] }) + var total int64 + for _, d := range times { + total += d + } + var mean, median, p90, p99, worst int64 + mean = total / int64(len(times)) + median = times[len(times)/2] + p90 = times[(len(times)*9)/10] + p99 = times[(len(times)*99)/100] + worst = times[len(times)-1] + surviving = append(surviving, fmt.Sprintf("%d finalized spans. lag: mean %v, median %v, p90 %v, p99 %v, worst %v", + len(times), time.Duration(mean), time.Duration(median), time.Duration(p90), time.Duration(p99), time.Duration(worst))) + } + return surviving +} diff --git a/generation_nodebug.go b/generation_nodebug.go new file mode 100644 index 000000000..a2d2dd2f6 --- /dev/null +++ b/generation_nodebug.go @@ -0,0 +1,37 @@ +// Copyright 2019 Pilosa Corp. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// +build !generationdebug + +package pilosa + +const generationDebug = false + +func registerGeneration(id string) string { + return id +} + +func endGeneration(id string) { +} + +func cancelGeneration(id string) { +} + +func finalizeGeneration(id string) { +} + +//lint:ignore U1000 this is conditional on a build flag, see generation_test.go. +func reportGenerations() []string { //nolint:unused,deadcode + return nil +} diff --git a/generation_test.go b/generation_test.go new file mode 100644 index 000000000..3dd86fd8f --- /dev/null +++ b/generation_test.go @@ -0,0 +1,39 @@ +// Copyright 2019 Pilosa Corp. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// +build generationdebug + +package pilosa + +import ( + "fmt" + "os" + "testing" +) + +func examineResults() { + results := reportGenerations() + if len(results) > 0 { + fmt.Printf("generations:\n") + for _, res := range results { + fmt.Printf(" %s\n", res) + } + } +} + +func TestMain(m *testing.M) { + ret := m.Run() + examineResults() + os.Exit(ret) +} diff --git a/holder_test.go b/holder_test.go index 3f88703e5..09ec03d97 100644 --- a/holder_test.go +++ b/holder_test.go @@ -197,7 +197,26 @@ func TestHolder_Open(t *testing.T) { t.Fatalf("unexpected error: %s", err) } }) + t.Run("ErrFragmentStorageRecoverable", func(t *testing.T) { + h := test.MustOpenHolder() + defer h.Close() + if idx, err := h.CreateIndex("foo", pilosa.IndexOptions{}); err != nil { + t.Fatal(err) + } else if field, err := idx.CreateField("bar", pilosa.OptFieldTypeDefault()); err != nil { + t.Fatal(err) + } else if _, err := field.SetBit(0, 0, nil); err != nil { + t.Fatal(err) + } else if err := h.Holder.Close(); err != nil { + t.Fatal(err) + } else if err := os.Truncate(filepath.Join(h.Path, "foo", "bar", "views", "standard", "fragments", "0"), 20); err != nil { + t.Fatal(err) + } + + if err := h.Reopen(); err != nil { + t.Fatalf("unexpected error: %s", err) + } + }) } func TestHolder_HasData(t *testing.T) { diff --git a/roaring/container_stash.go b/roaring/container_stash.go index fe03f9d2e..c1dee9fa4 100644 --- a/roaring/container_stash.go +++ b/roaring/container_stash.go @@ -267,9 +267,6 @@ func (c *Container) Freeze() *Container { if c.flags&flagFrozen != 0 { return c } - // unmapOrClone should unmap-in-place because the existing - // container isn't frozen (or we'd already have returned it). - c = c.unmapOrClone() c.flags |= flagFrozen return c } diff --git a/roaring/generation_debug.go b/roaring/generation_debug.go new file mode 100644 index 000000000..e5ac9f7b8 --- /dev/null +++ b/roaring/generation_debug.go @@ -0,0 +1,19 @@ +// Copyright 2019 Pilosa Corp. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// +build generationdebug + +package roaring + +const generationDebug = true diff --git a/roaring/generation_nodebug.go b/roaring/generation_nodebug.go new file mode 100644 index 000000000..4ce3f4ab1 --- /dev/null +++ b/roaring/generation_nodebug.go @@ -0,0 +1,19 @@ +// Copyright 2019 Pilosa Corp. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// +build !generationdebug + +package roaring + +const generationDebug = false diff --git a/roaring/roaring.go b/roaring/roaring.go index 046e1f859..2c985358b 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -184,6 +184,7 @@ type ContainerIterator interface { // Bitmap represents a roaring bitmap. type Bitmap struct { Containers Containers + Source Source // User-defined flags. Flags byte @@ -258,6 +259,7 @@ func (b *Bitmap) Freeze() *Bitmap { // Create a copy of the bitmap structure. other := &Bitmap{ Containers: b.Containers.Freeze(), + Source: b.Source, } return other @@ -589,13 +591,21 @@ func (b *Bitmap) OffsetRange(offset, start, end uint64) *Bitmap { hi0, hi1 := highbits(start), highbits(end) citer, _ := b.Containers.Iterator(hi0) other := NewSliceBitmap() + mappedAny := false for citer.Next() { k, c := citer.Value() if k >= hi1 { break } + if c.Mapped() { + mappedAny = true + } other.Containers.Put(off+(k-hi0), c.Freeze()) } + // if b.Source != nil && mappedAny { + if b.Source != nil && (generationDebug || mappedAny) { + other.Source = b.Source + } return other } @@ -634,6 +644,7 @@ func (b *Bitmap) IntersectionCount(other *Bitmap) uint64 { // Intersect returns the intersection of b and other. func (b *Bitmap) Intersect(other *Bitmap) *Bitmap { output := NewBitmap() + usedB, usedOther := false, false iiter, _ := b.Containers.Iterator(0) jiter, _ := other.Containers.Iterator(0) i, j := iiter.Next(), jiter.Next() @@ -647,12 +658,27 @@ func (b *Bitmap) Intersect(other *Bitmap) *Bitmap { j = jiter.Next() kj, cj = jiter.Value() } else { // ki == kj - output.Containers.Put(ki, intersect(ci, cj)) + newC := intersect(ci, cj) + if newC == ci { + usedB = true + } + if newC == cj { + usedOther = true + } + output.Containers.Put(ki, newC) i, j = iiter.Next(), jiter.Next() ki, ci = iiter.Value() kj, cj = jiter.Value() } } + switch { + case usedB && usedOther: + output.Source = MergeSources(b.Source, other.Source) + case usedB: + output.Source = b.Source + case usedOther: + output.Source = other.Source + } return output } @@ -680,25 +706,43 @@ func (b *Bitmap) UnionInPlace(others ...*Bitmap) { func (b *Bitmap) unionIntoTargetSingle(target *Bitmap, other *Bitmap) { iiter, _ := b.Containers.Iterator(0) jiter, _ := other.Containers.Iterator(0) + usedB, usedOther := false, false i, j := iiter.Next(), jiter.Next() ki, ci := iiter.Value() kj, cj := jiter.Value() for i || j { if i && (!j || ki < kj) { target.Containers.Put(ki, ci.Freeze()) + usedB = true i = iiter.Next() ki, ci = iiter.Value() } else if j && (!i || ki > kj) { target.Containers.Put(kj, cj.Freeze()) + usedOther = true j = jiter.Next() kj, cj = jiter.Value() } else { // ki == kj - target.Containers.Put(ki, union(ci, cj)) + newC := union(ci, cj) + target.Containers.Put(ki, newC) + if newC == ci { + usedB = true + } + if newC == cj { + usedOther = true + } i, j = iiter.Next(), jiter.Next() ki, ci = iiter.Value() kj, cj = jiter.Value() } } + switch { + case usedB && usedOther: + target.Source = MergeSources(b.Source, other.Source) + case usedB: + target.Source = b.Source + case usedOther: + target.Source = other.Source + } } // unionInPlace stores the union of b and others into b. The others will @@ -792,7 +836,14 @@ func (b *Bitmap) unionInPlace(others ...*Bitmap) { bitmapIters = make(handledIters, 0, requiredSliceSize) } + var sources []Source + if b.Source != nil { + sources = append(sources, b.Source) + } for _, other := range others { + if other.Source != nil { + sources = append(sources, other.Source) + } otherIter, _ := other.Containers.Iterator(0) if otherIter.Next() { bitmapIters = append(bitmapIters, handledIter{ @@ -802,6 +853,8 @@ func (b *Bitmap) unionInPlace(others ...*Bitmap) { }) } } + // new bitmap might have containers from any of those bitmaps in it + b.Source = MergeSources(sources...) // Loop until we've exhausted every iter. hasNext := true @@ -930,6 +983,7 @@ func (b *Bitmap) unionInPlace(others ...*Bitmap) { // Difference returns the difference of b and other. func (b *Bitmap) Difference(other *Bitmap) *Bitmap { output := NewBitmap() + output.Source = b.Source iiter, _ := b.Containers.Iterator(0) jiter, _ := other.Containers.Iterator(0) @@ -957,6 +1011,9 @@ func (b *Bitmap) Difference(other *Bitmap) *Bitmap { // Xor returns the bitwise exclusive or of b and other. func (b *Bitmap) Xor(other *Bitmap) *Bitmap { output := NewBitmap() + // Xor can end up with containers from either parent if the other + // had no container or an empty container. + output.Source = MergeSources(b.Source, other.Source) iiter, _ := b.Containers.Iterator(0) jiter, _ := other.Containers.Iterator(0) @@ -1473,7 +1530,11 @@ func (b *Bitmap) RemapRoaringStorage(data []byte) (mappedAny bool, returnErr err var itrPointer *uint16 var itrErr error - if data != nil { + // If we got no data, we don't want to do the actual mapping, just + // the unmapping. If preferMapping is false, we also don't want to + // map to the data. We still need to do the UpdateEvery loop, we + // just won't have an iterator for it. + if data != nil && b.preferMapping { itr, err = newRoaringIterator(data) } // don't return early: we still have to do the unmapping @@ -3311,9 +3372,9 @@ func intersectRunRun(a, b *Container) *Container { output.setN(n) runs := output.runs() if n < ArrayMaxSize && int32(len(runs)) > n/2 { - output.runToArray() + output = output.runToArray() } else if len(runs) > runMaxSize { - output.runToBitmap() + output = output.runToBitmap() } return output } diff --git a/roaring/source.go b/roaring/source.go new file mode 100644 index 000000000..4cd7ae244 --- /dev/null +++ b/roaring/source.go @@ -0,0 +1,98 @@ +// Copyright 2017 Pilosa Corp. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package roaring + +import ( + "strings" +) + +// A Source represents the source a given bitmap gets its data from, +// such as a memory-mapped file. When combining bitmaps, we might +// track them together in a single combined-source of some sort. +type Source interface { + ID() string + Dead() bool +} + +// MergeSources combines sources. If you have two bitmaps, and you're +// combining them, then the combination's source is a combination of +// those two sources. +func MergeSources(sources ...Source) Source { + sourceCount := 0 + totalCount := 0 + var lastSource Source + for _, s := range sources { + if s == nil { + continue + } + lastSource = s + if s, ok := s.(combinedSource); ok { + sourceCount++ + totalCount += len(s) + } else { + sourceCount++ + totalCount++ + } + } + // if there's no sources (this includes all sources being + // empty combinedSources), we don't have a source. + if totalCount == 0 { + return nil + } + // if there's exactly one source, combined or otherwise, that's + // fine, we'll just return it. + if sourceCount == 1 { + return lastSource + } + // make a new combinedSource, flattening any combinedSources + // already present. + newSources := make([]Source, 0, totalCount) + for _, s := range sources { + if s == nil { + continue + } + if s, ok := s.(combinedSource); ok { + newSources = append(newSources, s...) + } else { + newSources = append(newSources, s) + } + } + return combinedSource(newSources) +} + +// SetSource tells the bitmap what source to associate with new things it +// creates. This is possibly logically incorrect. +func (b *Bitmap) SetSource(s Source) { + b.Source = s +} + +type combinedSource []Source + +func (c combinedSource) ID() string { + ids := make([]string, len(c)) + for i := range c { + ids[i] = c[i].ID() + } + return strings.Join(ids, ",") +} + +func (c combinedSource) Dead() bool { + for i := range c { + if c[i].Dead() { + return true + } + } + return false +} diff --git a/roaring/unmarshal_binary.go b/roaring/unmarshal_binary.go index c4feb5bd1..e80e834cf 100644 --- a/roaring/unmarshal_binary.go +++ b/roaring/unmarshal_binary.go @@ -30,7 +30,9 @@ func (b *Bitmap) UnmarshalBinary(data []byte) error { return nil } statsHit("Bitmap/UnmarshalBinary") - b.opN = 0 // reset opN since we're reading new data. + // reset ops/opN since we're reading new data. + b.ops = 0 + b.opN = 0 fileMagic := uint32(binary.LittleEndian.Uint16(data[0:2])) if fileMagic == MagicNumber { // if pilosa roaring return errors.Wrap(b.unmarshalPilosaRoaring(data), "unmarshaling as pilosa roaring") @@ -213,7 +215,7 @@ func (b *Bitmap) unmarshalPilosaRoaring(data []byte) error { b.opN += opr.count() opsOffset += opr.size() // Move the buffer forward. - buf = buf[opsOffset:] + buf = data[opsOffset:] } return nil diff --git a/row.go b/row.go index 519416420..601d5101e 100644 --- a/row.go +++ b/row.go @@ -359,7 +359,7 @@ type rowSegment struct { } func (s *rowSegment) Freeze() { - s.data.Freeze() + s.data = s.data.Freeze() } /* @@ -392,7 +392,7 @@ func (s *rowSegment) IntersectionCount(other *rowSegment) uint64 { // Intersect returns the itersection of s and other. func (s *rowSegment) Intersect(other *rowSegment) *rowSegment { data := s.data.Intersect(other.data) - data.Freeze() + data = data.Freeze() return &rowSegment{ data: data, @@ -422,7 +422,7 @@ func (s *rowSegment) Union(others ...*rowSegment) *rowSegment { // Difference returns the diff of s and other. func (s *rowSegment) Difference(other *rowSegment) *rowSegment { data := s.data.Difference(other.data) - data.Freeze() + data = data.Freeze() return &rowSegment{ data: data, @@ -435,7 +435,7 @@ func (s *rowSegment) Difference(other *rowSegment) *rowSegment { // Xor returns the xor of s and other. func (s *rowSegment) Xor(other *rowSegment) *rowSegment { data := s.data.Xor(other.data) - data.Freeze() + data = data.Freeze() return &rowSegment{ data: data, @@ -452,7 +452,7 @@ func (s *rowSegment) Shift() (*rowSegment, error) { if err != nil { return nil, errors.Wrap(err, "shifting roaring data") } - data.Freeze() + data = data.Freeze() return &rowSegment{ data: data, diff --git a/utils_internal_test.go b/utils_internal_test.go index 76e70fb9e..fe8e0d278 100644 --- a/utils_internal_test.go +++ b/utils_internal_test.go @@ -301,6 +301,9 @@ func (t *ClusterCluster) Close() error { if err != nil { return err } + // Make sure open indexes get shut down too. we wouldn't do + // this normally for a cluster, but we want to for test cases. + c.holder.Close() } return nil } diff --git a/view.go b/view.go index 89484fa3f..69ae1b22f 100644 --- a/view.go +++ b/view.go @@ -483,7 +483,7 @@ func upgradeViewBSIv2(v *view, bitDepth uint) (ok bool, _ error) { if tmpPath, err := upgradeRoaringBSIv2(frag, bitDepth); err != nil { return ok, errors.Wrap(err, "upgrading bsi v2") - } else if err := frag.closeStorage(true); err != nil { + } else if err := frag.closeStorage(); err != nil { return ok, errors.Wrap(err, "closing after bsi v2 upgrade") } else if err := os.Rename(tmpPath, frag.path); err != nil { return ok, errors.Wrap(err, "renaming after bsi v2 upgrade")