From dbeef9f06cc4b4ae08fea46763b3deed4531e524 Mon Sep 17 00:00:00 2001
From: Lucas Wang <lucas@dgraph.io>
Date: Mon, 26 Aug 2019 14:48:05 -0700
Subject: [PATCH 1/2] Fixes the toBackupList function by removing the loop

---
 ee/backup/backup.go | 113 ++++++++++++++++++--------------------------
 1 file changed, 46 insertions(+), 67 deletions(-)

diff --git a/ee/backup/backup.go b/ee/backup/backup.go
index bffc4c25881..e9604a98409 100644
--- a/ee/backup/backup.go
+++ b/ee/backup/backup.go
@@ -13,7 +13,6 @@
 package backup
 
 import (
-	"bytes"
 	"compress/gzip"
 	"context"
 	"encoding/binary"
@@ -198,86 +197,66 @@ func (m *Manifest) GoString() string {
 func (pr *Processor) toBackupList(key []byte, itr *badger.Iterator) (*bpb.KVList, error) {
 	list := &bpb.KVList{}
 
-	// TODO: This for loop doesn't seem right. We should only have one iteration here.
-	for itr.Valid() {
-		item := itr.Item()
-		if !bytes.Equal(item.Key(), key) {
-			return list, nil
+	item := itr.Item()
+	if item.Version() < pr.Request.SinceTs || item.IsDeletedOrExpired() {
+		// Ignore versions less than given timestamp, or skip older versions of
+		// the given key by returning an empty list.
+		return list, nil
+	}
+
+	switch item.UserMeta() {
+	case posting.BitEmptyPosting, posting.BitCompletePosting, posting.BitDeltaPosting:
+		l, err := posting.ReadPostingList(key, itr)
+		if err != nil {
+			return nil, errors.Wrapf(err, "while reading posting list")
 		}
-		if item.Version() < pr.Request.SinceTs {
-			// Ignore versions less than given timestamp, or skip older versions of
-			// the given key.
-			return list, nil
+		kvs, err := l.Rollup()
+		if err != nil {
+			return nil, errors.Wrapf(err, "while rolling up list")
 		}
 
-		switch item.UserMeta() {
-		case posting.BitEmptyPosting, posting.BitCompletePosting, posting.BitDeltaPosting:
-			l, err := posting.ReadPostingList(key, itr)
-			if err != nil {
-				return nil, errors.Wrapf(err, "while reading posting list")
-			}
-			kvs, err := l.Rollup()
+		for _, kv := range kvs {
+			backupKey, err := toBackupKey(kv.Key)
 			if err != nil {
-				return nil, errors.Wrapf(err, "while rolling up list")
-			}
-
-			for _, kv := range kvs {
-				backupKey, err := toBackupKey(kv.Key)
-				if err != nil {
-					return nil, err
-				}
-				kv.Key = backupKey
-
-				backupPl, err := toBackupPostingList(kv.Value)
-				if err != nil {
-					return nil, err
-				}
-				kv.Value = backupPl
-			}
-			list.Kv = append(list.Kv, kvs...)
-
-		case posting.BitSchemaPosting:
-			var valCopy []byte
-			if !item.IsDeletedOrExpired() {
-				// No need to copy value if item is deleted or expired.
-				var err error
-				valCopy, err = item.ValueCopy(nil)
-				if err != nil {
-					return nil, errors.Wrapf(err, "while copying value")
-				}
+				return nil, err
 			}
+			kv.Key = backupKey
 
-			backupKey, err := toBackupKey(key)
+			backupPl, err := toBackupPostingList(kv.Value)
 			if err != nil {
 				return nil, err
 			}
-
-			kv := &bpb.KV{
-				Key:       backupKey,
-				Value:     valCopy,
-				UserMeta:  []byte{item.UserMeta()},
-				Version:   item.Version(),
-				ExpiresAt: item.ExpiresAt(),
-			}
-			list.Kv = append(list.Kv, kv)
-
-			if item.DiscardEarlierVersions() || item.IsDeletedOrExpired() {
-				return list, nil
+			kv.Value = backupPl
+		}
+		list.Kv = append(list.Kv, kvs...)
+	case posting.BitSchemaPosting:
+		var valCopy []byte
+		if !item.IsDeletedOrExpired() {
+			// No need to copy value if item is deleted or expired.
+			var err error
+			valCopy, err = item.ValueCopy(nil)
+			if err != nil {
+				return nil, errors.Wrapf(err, "while copying value")
 			}
+		}
 
-			// Manually advance the iterator. This cannot be done in the for
-			// statement because ReadPostingList advances the iterator so this
-			// only needs to be done for BitSchemaPosting entries.
-			itr.Next()
+		backupKey, err := toBackupKey(key)
+		if err != nil {
+			return nil, err
+		}
 
-		default:
-			return nil, errors.Errorf(
-				"Unexpected meta: %d for key: %s", item.UserMeta(), hex.Dump(key))
+		kv := &bpb.KV{
+			Key:       backupKey,
+			Value:     valCopy,
+			UserMeta:  []byte{item.UserMeta()},
+			Version:   item.Version(),
+			ExpiresAt: item.ExpiresAt(),
 		}
+		list.Kv = append(list.Kv, kv)
+	default:
+		return nil, errors.Errorf(
+			"Unexpected meta: %d for key: %s", item.UserMeta(), hex.Dump(key))
 	}
-
-	// This shouldn't be reached but it's being added here because the golang
-	// compiler complains about the missing return statement.
 	return list, nil
 }
 

From a651ab9e25984a5c9ce66591d20768fb1e6c50d5 Mon Sep 17 00:00:00 2001
From: Lucas Wang <lucas@dgraph.io>
Date: Mon, 26 Aug 2019 16:11:08 -0700
Subject: [PATCH 2/2] Removed one if condition check which should always be
 true

---
 ee/backup/backup.go | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/ee/backup/backup.go b/ee/backup/backup.go
index e9604a98409..674c84e38ab 100644
--- a/ee/backup/backup.go
+++ b/ee/backup/backup.go
@@ -230,14 +230,9 @@ func (pr *Processor) toBackupList(key []byte, itr *badger.Iterator) (*bpb.KVList
 		}
 		list.Kv = append(list.Kv, kvs...)
 	case posting.BitSchemaPosting:
-		var valCopy []byte
-		if !item.IsDeletedOrExpired() {
-			// No need to copy value if item is deleted or expired.
-			var err error
-			valCopy, err = item.ValueCopy(nil)
-			if err != nil {
-				return nil, errors.Wrapf(err, "while copying value")
-			}
+		valCopy, err := item.ValueCopy(nil)
+		if err != nil {
+			return nil, errors.Wrapf(err, "while copying value")
 		}
 
 		backupKey, err := toBackupKey(key)