perf(GC): Remove move keys (#1539)

Move keys are used to fix the invalid value pointers when vlog GC runs.
This PR removes the move keys. To ensure we return the correct results
for Get calls, we will search all the levels of the LSM tree to find the
correct entry. Earlier, we used to return the first entry whose version
was less than or equal to the required version.

Author: Ibrahim Jarif

badger/cmd/write_bench.go

@@ -292,7 +292,6 @@ func writeBench(cmd *cobra.Command, args []string) error {
 func showKeysStats(db *badger.DB) {
 	var (
 		internalKeyCount uint32
-		moveKeyCount     uint32
 		invalidKeyCount  uint32
 		validKeyCount    uint32
 	)
@@ -311,18 +310,14 @@ func showKeysStats(db *badger.DB) {
 		if bytes.HasPrefix(i.Key(), []byte("!badger!")) {
 			internalKeyCount++
 		}
-		if bytes.HasPrefix(i.Key(), []byte("!badger!Move")) {
-			moveKeyCount++
-		}
 		if i.IsDeletedOrExpired() {
 			invalidKeyCount++
 		} else {
 			validKeyCount++
 		}
 	}
-	fmt.Printf("Valid Keys: %d Invalid Keys: %d Move Keys:"+
-		" %d Internal Keys: %d\n", validKeyCount, invalidKeyCount,
-		moveKeyCount, internalKeyCount)
+	fmt.Printf("Valid Keys: %d Invalid Keys: %d Internal Keys: %d\n",
+		validKeyCount, invalidKeyCount, internalKeyCount)
 }
 
 func reportStats(c *z.Closer, db *badger.DB) {

db.go

@@ -46,7 +46,6 @@ var (
 	badgerPrefix      = []byte("!badger!")        // Prefix for internal keys used by badger.
 	head              = []byte("!badger!head")    // For storing value offset for replay.
 	txnKey            = []byte("!badger!txn")     // For indicating end of entries in txn.
-	badgerMove        = []byte("!badger!move")    // For key-value pairs which got moved during GC.
 	lfDiscardStatsKey = []byte("!badger!discard") // For storing lfDiscardStats
 )
 
@@ -704,29 +703,20 @@ func (db *DB) getMemTables() ([]*skl.Skiplist, func()) {
 // that all versions of a key are always present in the same table from level 1, because compaction
 // can push any table down.
 //
-// Update (Sep 22, 2018): To maintain the above invariant, and to allow keys to be moved from one
-// value log to another (while reclaiming space during value log GC), we have logically moved this
-// need to write "old versions after new versions" to the badgerMove keyspace. Thus, for normal
-// gets, we can stop going down the LSM tree once we find any version of the key (note however that
-// we will ALWAYS skip versions with ts greater than the key version).  However, if that key has
-// been moved, then for the corresponding movekey, we'll look through all the levels of the tree
-// to ensure that we pick the highest version of the movekey present.
+// Update(23/09/2020) - We have dropped the move key implementation. Earlier we
+// were inserting move keys to fix the invalid value pointers but we no longer
+// do that. For every get("fooX") call where X is the version, we will search
+// for "fooX" in all the levels of the LSM tree. This is expensive but it
+// removes the overhead of handling move keys completely.
 func (db *DB) get(key []byte) (y.ValueStruct, error) {
 	if db.IsClosed() {
 		return y.ValueStruct{}, ErrDBClosed
 	}
 	tables, decr := db.getMemTables() // Lock should be released.
 	defer decr()
 
-	var maxVs *y.ValueStruct
-	var version uint64
-	if bytes.HasPrefix(key, badgerMove) {
-		// If we are checking badgerMove key, we should look into all the
-		// levels, so we can pick up the newer versions, which might have been
-		// compacted down the tree.
-		maxVs = &y.ValueStruct{}
-		version = y.ParseTs(key)
-	}
+	var maxVs y.ValueStruct
+	version := y.ParseTs(key)
 
 	y.NumGets.Add(1)
 	for i := 0; i < len(tables); i++ {
@@ -735,13 +725,12 @@ func (db *DB) get(key []byte) (y.ValueStruct, error) {
 		if vs.Meta == 0 && vs.Value == nil {
 			continue
 		}
-		// Found a version of the key. For user keyspace, return immediately. For move keyspace,
-		// continue iterating, unless we found a version == given key version.
-		if maxVs == nil || vs.Version == version {
+		// Found the required version of the key, return immediately.
+		if vs.Version == version {
 			return vs, nil
 		}
 		if maxVs.Version < vs.Version {
-			*maxVs = vs
+			maxVs = vs
 		}
 	}
 	return db.lc.get(key, maxVs, 0)
@@ -1307,7 +1296,7 @@ func (db *DB) RunValueLogGC(discardRatio float64) error {
 	// Find head on disk
 	headKey := y.KeyWithTs(head, math.MaxUint64)
 	// Need to pass with timestamp, lsm get removes the last 8 bytes and compares key
-	val, err := db.lc.get(headKey, nil, startLevel)
+	val, err := db.lc.get(headKey, y.ValueStruct{}, startLevel)
 	if err != nil {
 		return errors.Wrap(err, "Retrieving head from on-disk LSM")
 	}

errors.go

@@ -55,11 +55,6 @@ var (
 	// reserved for internal usage.
 	ErrInvalidKey = errors.New("Key is using a reserved !badger! prefix")
 
-	// ErrRetry is returned when a log file containing the value is not found.
-	// This usually indicates that it may have been garbage collected, and the
-	// operation needs to be retried.
-	ErrRetry = errors.New("Unable to find log file. Please retry")
-
 	// ErrThresholdZero is returned if threshold is set to zero, and value log GC is called.
 	// In such a case, GC can't be run.
 	ErrThresholdZero = errors.New(

iterator.go

@@ -151,66 +151,29 @@ func (item *Item) DiscardEarlierVersions() bool {
 
 func (item *Item) yieldItemValue() ([]byte, func(), error) {
 	key := item.Key() // No need to copy.
-	for {
-		if !item.hasValue() {
-			return nil, nil, nil
-		}
+	if !item.hasValue() {
+		return nil, nil, nil
+	}
 
-		if item.slice == nil {
-			item.slice = new(y.Slice)
-		}
+	if item.slice == nil {
+		item.slice = new(y.Slice)
+	}
 
-		if (item.meta & bitValuePointer) == 0 {
-			val := item.slice.Resize(len(item.vptr))
-			copy(val, item.vptr)
-			return val, nil, nil
-		}
+	if (item.meta & bitValuePointer) == 0 {
+		val := item.slice.Resize(len(item.vptr))
+		copy(val, item.vptr)
+		return val, nil, nil
+	}
 
-		var vp valuePointer
-		vp.Decode(item.vptr)
-		db := item.txn.db
-		result, cb, err := db.vlog.Read(vp, item.slice)
-		if err != ErrRetry {
-			if err != nil {
-				db.opt.Logger.Errorf(`Unable to read: Key: %v, Version : %v,
+	var vp valuePointer
+	vp.Decode(item.vptr)
+	db := item.txn.db
+	result, cb, err := db.vlog.Read(vp, item.slice)
+	if err != nil {
+		db.opt.Logger.Errorf(`Unable to read: Key: %v, Version : %v,
 				meta: %v, userMeta: %v`, key, item.version, item.meta, item.userMeta)
-			}
-			return result, cb, err
-		}
-		if bytes.HasPrefix(key, badgerMove) {
-			// err == ErrRetry
-			// Error is retry even after checking the move keyspace. So, let's
-			// just assume that value is not present.
-			return nil, cb, nil
-		}
-
-		// The value pointer is pointing to a deleted value log. Look for the
-		// move key and read that instead.
-		runCallback(cb)
-		// Do not put badgerMove on the left in append. It seems to cause some sort of manipulation.
-		keyTs := y.KeyWithTs(item.Key(), item.Version())
-		key = make([]byte, len(badgerMove)+len(keyTs))
-		n := copy(key, badgerMove)
-		copy(key[n:], keyTs)
-		// Note that we can't set item.key to move key, because that would
-		// change the key user sees before and after this call. Also, this move
-		// logic is internal logic and should not impact the external behavior
-		// of the retrieval.
-		vs, err := item.txn.db.get(key)
-		if err != nil {
-			return nil, nil, err
-		}
-		if vs.Version != item.Version() {
-			return nil, nil, nil
-		}
-		// Bug fix: Always copy the vs.Value into vptr here. Otherwise, when item is reused this
-		// slice gets overwritten.
-		item.vptr = y.SafeCopy(item.vptr, vs.Value)
-		item.meta &^= bitValuePointer // Clear the value pointer bit.
-		if vs.Meta&bitValuePointer > 0 {
-			item.meta |= bitValuePointer // This meta would only be about value pointer.
-		}
 	}
+	return result, cb, err
 }
 
 func runCallback(cb func()) {

levels.go

@@ -269,21 +269,13 @@ func (s *levelsController) dropTree() (int, error) {
 }
 
 // dropPrefix runs a L0->L1 compaction, and then runs same level compaction on the rest of the
-// levels. For L0->L1 compaction, it runs compactions normally, but skips over all the keys with the
-// provided prefix and also the internal move keys for the same prefix.
+// levels. For L0->L1 compaction, it runs compactions normally, but skips over
+// all the keys with the provided prefix.
 // For Li->Li compactions, it picks up the tables which would have the prefix. The
 // tables who only have keys with this prefix are quickly dropped. The ones which have other keys
 // are run through MergeIterator and compacted to create new tables. All the mechanisms of
 // compactions apply, i.e. level sizes and MANIFEST are updated as in the normal flow.
 func (s *levelsController) dropPrefixes(prefixes [][]byte) error {
-	// Internal move keys related to the given prefix should also be skipped.
-	for _, prefix := range prefixes {
-		key := make([]byte, 0, len(badgerMove)+len(prefix))
-		key = append(key, badgerMove...)
-		key = append(key, prefix...)
-		prefixes = append(prefixes, key)
-	}
-
 	opt := s.kv.opt
 	// Iterate levels in the reverse order because if we were to iterate from
 	// lower level (say level 0) to a higher level (say level 3) we could have
@@ -1078,8 +1070,10 @@ func (s *levelsController) close() error {
 	return errors.Wrap(err, "levelsController.Close")
 }
 
-// get returns the found value if any. If not found, we return nil.
-func (s *levelsController) get(key []byte, maxVs *y.ValueStruct, startLevel int) (
+// get searches for a given key in all the levels of the LSM tree. It returns
+// key version <= the expected version (maxVs). If not found, it returns an empty
+// y.ValueStruct.
+func (s *levelsController) get(key []byte, maxVs y.ValueStruct, startLevel int) (
 	y.ValueStruct, error) {
 	if s.kv.IsClosed() {
 		return y.ValueStruct{}, ErrDBClosed
@@ -1102,17 +1096,14 @@ func (s *levelsController) get(key []byte, maxVs *y.ValueStruct, startLevel int)
 		if vs.Value == nil && vs.Meta == 0 {
 			continue
 		}
-		if maxVs == nil || vs.Version == version {
+		if vs.Version == version {
 			return vs, nil
 		}
 		if maxVs.Version < vs.Version {
-			*maxVs = vs
+			maxVs = vs
 		}
 	}
-	if maxVs != nil {
-		return *maxVs, nil
-	}
-	return y.ValueStruct{}, nil
+	return maxVs, nil
 }
 
 func appendIteratorsReversed(out []y.Iterator, th []*table.Table, opt int) []y.Iterator {