Add an alternative to DeleteVersions that leaves no garbage

CHANGELOG.md

@@ -1,5 +1,12 @@
 # Changelog
 
+## Unreleased
+
+### Bug Fixes
+
+- [\#324](https://github.com/cosmos/iavl/pull/324) Fix `DeleteVersions` causing a memory leak. 
+  Add `DeleteVersionsRange` to delete in range (@klim0v)
+
 ## 0.14.2 (October 12, 2020)
 
 ### Bug Fixes

go.mod

@@ -7,6 +7,6 @@ require (
 	github.com/stretchr/testify v1.6.1
 	github.com/tendermint/go-amino v0.14.1
 	github.com/tendermint/tendermint v0.33.5
-	github.com/tendermint/tm-db v0.5.1
+	github.com/tendermint/tm-db v0.5.2
 	golang.org/x/crypto v0.0.0-20200429183012-4b2356b1ed79
 )

go.sum

@@ -4,6 +4,7 @@ github.com/BurntSushi/toml v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ
 github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU=
 github.com/ChainSafe/go-schnorrkel v0.0.0-20200405005733-88cbf1b4c40d/go.mod h1:URdX5+vg25ts3aCh8H5IFZybJYKWhJHYMTnf+ULtoC4=
 github.com/Knetic/govaluate v3.0.1-0.20171022003610-9aa49832a739+incompatible/go.mod h1:r7JcOSlj0wfOMncg0iLm8Leh48TZaKVeNIfJntJ2wa0=
+github.com/OneOfOne/xxhash v1.2.2 h1:KMrpdQIwFcEqXDklaen+P1axHaj9BSKzvpUUfnHldSE=
 github.com/OneOfOne/xxhash v1.2.2/go.mod h1:HSdplMjZKSmBqAxg5vPj2TmRDmfkzw+cTzAElWljhcU=
 github.com/Shopify/sarama v1.19.0/go.mod h1:FVkBWblsNy7DGZRfXLU0O9RCGt5g3g3yEuWXgklEdEo=
 github.com/Shopify/toxiproxy v2.1.4+incompatible/go.mod h1:OXgGpZ6Cli1/URJOF1DMxUHB2q5Ap20/P/eIdh4G0pI=
@@ -41,6 +42,7 @@ github.com/btcsuite/winsvc v1.0.0/go.mod h1:jsenWakMcC0zFBFurPLEAyrnc/teJEM1O46f
 github.com/casbin/casbin/v2 v2.1.2/go.mod h1:YcPU1XXisHhLzuxH9coDNf2FbKpjGlbCg3n9yuLkIJQ=
 github.com/cenkalti/backoff v2.2.1+incompatible/go.mod h1:90ReRw6GdpyfrHakVjL/QHaoyV4aDUVVkXQJJJ3NXXM=
 github.com/census-instrumentation/opencensus-proto v0.2.1/go.mod h1:f6KPmirojxKA12rnyqOA5BBL4O983OfeGPqjHWSTneU=
+github.com/cespare/xxhash v1.1.0 h1:a6HrQnmkObjyL+Gs60czilIUGqrzKutQD6XZog3p+ko=
 github.com/cespare/xxhash v1.1.0/go.mod h1:XrSqR1VqqWfGrhpAt58auRo0WTKS1nRRg3ghfAqPWnc=
 github.com/cespare/xxhash/v2 v2.1.1/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs=
 github.com/clbanning/x2j v0.0.0-20191024224557-825249438eec/go.mod h1:jMjuTZXRI4dUb/I5gc9Hdhagfvm9+RyrPryS/auMzxE=
@@ -344,6 +346,8 @@ github.com/tendermint/tendermint v0.33.5 h1:jYgRd9ImkzA9iOyhpmgreYsqSB6tpDa6/rXY
 github.com/tendermint/tendermint v0.33.5/go.mod h1:0yUs9eIuuDq07nQql9BmI30FtYGcEC60Tu5JzB5IezM=
 github.com/tendermint/tm-db v0.5.1 h1:H9HDq8UEA7Eeg13kdYckkgwwkQLBnJGgX4PgLJRhieY=
 github.com/tendermint/tm-db v0.5.1/go.mod h1:g92zWjHpCYlEvQXvy9M168Su8V1IBEeawpXVVBaK4f4=
+github.com/tendermint/tm-db v0.5.2 h1:QG3IxQZBubWlr7kGQcYIavyTNmZRO+r//nENxoq0g34=
+github.com/tendermint/tm-db v0.5.2/go.mod h1:VrPTx04QJhQ9d8TFUTc2GpPBvBf/U9vIdBIzkjBk7Lk=
 github.com/tmc/grpc-websocket-proxy v0.0.0-20170815181823-89b8d40f7ca8/go.mod h1:ncp9v5uamzpCO7NfCPTXjqaC+bZgJeR0sMTm6dMHP7U=
 github.com/tmc/grpc-websocket-proxy v0.0.0-20190109142713-0ad062ec5ee5/go.mod h1:ncp9v5uamzpCO7NfCPTXjqaC+bZgJeR0sMTm6dMHP7U=
 github.com/ugorji/go v1.1.4/go.mod h1:uQMGLiO92mf5W77hV/PUCpI3pbzQx3CRekS0kk+RGrc=
@@ -354,6 +358,8 @@ github.com/xordataexchange/crypt v0.0.3-0.20170626215501-b2862e3d0a77/go.mod h1:
 go.etcd.io/bbolt v1.3.2/go.mod h1:IbVyRI1SCnLcuJnV2u8VeU0CEYM7e686BmAb1XKL+uU=
 go.etcd.io/bbolt v1.3.3 h1:MUGmc65QhB3pIlaQ5bB4LwqSj6GIonVJXpZiaKNyaKk=
 go.etcd.io/bbolt v1.3.3/go.mod h1:IbVyRI1SCnLcuJnV2u8VeU0CEYM7e686BmAb1XKL+uU=
+go.etcd.io/bbolt v1.3.4 h1:hi1bXHMVrlQh6WwxAy+qZCV/SYIlqo+Ushwdpa4tAKg=
+go.etcd.io/bbolt v1.3.4/go.mod h1:G5EMThwa9y8QZGBClrRx5EY+Yw9kAhnjy3bSjsnlVTQ=
 go.etcd.io/etcd v0.0.0-20191023171146-3cf2f69b5738/go.mod h1:dnLIgRNXwCJa5e+c6mIZCrds/GIG4ncV9HhK5PX7jPg=
 go.opencensus.io v0.20.1/go.mod h1:6WKK9ahsWS3RSO+PY9ZHZUfv2irvY6gN279GOPZjmmk=
 go.opencensus.io v0.20.2/go.mod h1:6WKK9ahsWS3RSO+PY9ZHZUfv2irvY6gN279GOPZjmmk=
@@ -431,6 +437,7 @@ golang.org/x/sys v0.0.0-20190726091711-fc99dfbffb4e/go.mod h1:h1NjWce9XRLGQEsW7w
 golang.org/x/sys v0.0.0-20190826190057-c7b8b68b1456/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20191220142924-d4481acd189f/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20200122134326-e047566fdf82/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
+golang.org/x/sys v0.0.0-20200202164722-d101bd2416d5/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20200323222414-85ca7c5b95cd h1:xhmwyvizuTgC2qz7ZlMluP20uW+C3Rm0FD/WLDX8884=
 golang.org/x/sys v0.0.0-20200323222414-85ca7c5b95cd/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
@@ -478,6 +485,7 @@ google.golang.org/grpc v1.25.1/go.mod h1:c3i+UQWmh7LiEpx4sFZnkU36qjEYZ0imhYfXVyQ
 google.golang.org/grpc v1.26.0/go.mod h1:qbnxyOmOxrQa7FizSgH+ReBfzJrCY1pSN7KXBS8abTk=
 google.golang.org/grpc v1.28.0/go.mod h1:rpkK4SK4GF4Ach/+MFLZUBavHOvF2JJB5uozKKal+60=
 google.golang.org/grpc v1.28.1/go.mod h1:rpkK4SK4GF4Ach/+MFLZUBavHOvF2JJB5uozKKal+60=
+google.golang.org/grpc v1.29.1/go.mod h1:itym6AZVZYACWQqET3MqgPpjcuV5QH3BxFS3IjizoKk=
 google.golang.org/protobuf v0.0.0-20200109180630-ec00e32a8dfd/go.mod h1:DFci5gLYBciE7Vtevhsrf46CRTquxDuWsQurQQe4oz8=
 google.golang.org/protobuf v0.0.0-20200221191635-4d8936d0db64/go.mod h1:kwYJMbMJ01Woi6D6+Kah6886xMZcty6N08ah7+eCXa0=
 google.golang.org/protobuf v0.0.0-20200228230310-ab0ca4ff8a60/go.mod h1:cfTl7dwQJ+fmap5saPgwCLgHXTUD7jkjRqWcaiX5VyM=

mutable_tree.go

@@ -530,23 +530,51 @@ func (tree *MutableTree) SetInitialVersion(version uint64) {
 	tree.ndb.opts.InitialVersion = version
 }
 
-// DeleteVersions deletes a series of versions from the MutableTree. An error
-// is returned if any single version is invalid or the delete fails. All writes
-// happen in a single batch with a single commit.
+// DeleteVersions deletes a series of versions from the MutableTree.
+// Deprecated: please use DeleteVersionsRange instead.
 func (tree *MutableTree) DeleteVersions(versions ...int64) error {
 	debug("DELETING VERSIONS: %v\n", versions)
 
-	for _, version := range versions {
-		if err := tree.deleteVersion(version); err != nil {
+	if len(versions) == 0 {
+		return nil
+	}
+
+	sort.Slice(versions, func(i, j int) bool {
+		return versions[i] < versions[j]
+	})
+
+	// Find ordered data and delete by interval
+	intervals := map[int64]int64{}
+	var fromVersion int64
+	for i := 0; i < len(versions); i++ {
+		if versions[i]-versions[fromVersion] != intervals[versions[fromVersion]] {
+			fromVersion = int64(i)
+		}
+		intervals[versions[fromVersion]]++
+	}
+
+	for fromVersion, sortedBatchSize := range intervals {
+		if err := tree.DeleteVersionsRange(fromVersion, fromVersion+sortedBatchSize); err != nil {
 			return err
 		}
 	}
 
+	return nil
+}
+
+// DeleteVersionsRange removes versions from an interval from the MutableTree (not inclusive).
+// An error is returned if any single version has active readers.
+// All writes happen in a single batch with a single commit.
+func (tree *MutableTree) DeleteVersionsRange(fromVersion, toVersion int64) error {
+	if err := tree.ndb.DeleteVersionsRange(fromVersion, toVersion); err != nil {
+		return err
+	}
+
 	if err := tree.ndb.Commit(); err != nil {
 		return err
 	}
 
-	for _, version := range versions {
+	for version := fromVersion; version < toVersion; version++ {
 		delete(tree.versions, version)
 	}
 

mutable_tree_test.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"fmt"
 	"runtime"
+	"strconv"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -106,6 +107,83 @@ func TestMutableTree_DeleteVersions(t *testing.T) {
 	}
 }
 
+func TestMutableTree_DeleteVersionsRange(t *testing.T) {
+	require := require.New(t)
+
+	mdb := db.NewMemDB()
+	tree, err := NewMutableTree(mdb, 0)
+	require.NoError(err)
+
+	const maxLength = 100
+	const fromLength = 10
+
+	versions := make([]int64, 0, maxLength)
+	for count := 1; count <= maxLength; count++ {
+		versions = append(versions, int64(count))
+		countStr := strconv.Itoa(count)
+		// Set kv pair and save version
+		tree.Set([]byte("aaa"), []byte("bbb"))
+		tree.Set([]byte("key"+countStr), []byte("value"+countStr))
+		_, _, err = tree.SaveVersion()
+		require.NoError(err, "SaveVersion should not fail")
+	}
+
+	tree, err = NewMutableTree(mdb, 0)
+	require.NoError(err)
+	targetVersion, err := tree.LoadVersion(int64(maxLength))
+	require.NoError(err)
+	require.Equal(targetVersion, int64(maxLength), "targetVersion shouldn't larger than the actual tree latest version")
+
+	err = tree.DeleteVersionsRange(fromLength, int64(maxLength/2))
+	require.NoError(err, "DeleteVersionsTo should not fail")
+
+	for _, version := range versions[:fromLength-1] {
+		require.True(tree.versions[version], "versions %d no more than 10 should exist", version)
+
+		v, err := tree.LazyLoadVersion(version)
+		require.NoError(err, version)
+		require.Equal(v, version)
+
+		_, value := tree.Get([]byte("aaa"))
+		require.Equal(string(value), "bbb")
+
+		for _, count := range versions[:version] {
+			countStr := strconv.Itoa(int(count))
+			_, value := tree.Get([]byte("key" + countStr))
+			require.Equal(string(value), "value"+countStr)
+		}
+	}
+
+	for _, version := range versions[fromLength : int64(maxLength/2)-1] {
+		require.False(tree.versions[version], "versions %d more 10 and no more than 50 should have been deleted", version)
+
+		_, err := tree.LazyLoadVersion(version)
+		require.Error(err)
+	}
+
+	for _, version := range versions[int64(maxLength/2)-1:] {
+		require.True(tree.versions[version], "versions %d more than 50 should exist", version)
+
+		v, err := tree.LazyLoadVersion(version)
+		require.NoError(err)
+		require.Equal(v, version)
+
+		_, value := tree.Get([]byte("aaa"))
+		require.Equal(string(value), "bbb")
+
+		for _, count := range versions[:fromLength] {
+			countStr := strconv.Itoa(int(count))
+			_, value := tree.Get([]byte("key" + countStr))
+			require.Equal(string(value), "value"+countStr)
+		}
+		for _, count := range versions[int64(maxLength/2)-1 : version] {
+			countStr := strconv.Itoa(int(count))
+			_, value := tree.Get([]byte("key" + countStr))
+			require.Equal(string(value), "value"+countStr)
+		}
+	}
+}
+
 func TestMutableTree_InitialVersion(t *testing.T) {
 	memDB := db.NewMemDB()
 	tree, err := NewMutableTreeWithOpts(memDB, 0, &Options{InitialVersion: 9})

nodedb.go

@@ -237,6 +237,55 @@ func (ndb *nodeDB) DeleteVersionsFrom(version int64) error {
 	return nil
 }
 
+// DeleteVersionsRange deletes versions from an interval (not inclusive).
+func (ndb *nodeDB) DeleteVersionsRange(fromVersion, toVersion int64) error {
+	if fromVersion >= toVersion {
+		return errors.New("toVersion must be greater than fromVersion")
+	}
+	if toVersion == 0 {
+		return errors.New("toVersion must be greater than 0")
+	}
+
+	ndb.mtx.Lock()
+	defer ndb.mtx.Unlock()
+
+	latest := ndb.getLatestVersion()
+	if latest < toVersion {
+		return errors.Errorf("cannot delete latest saved version (%d)", latest)
+	}
+
+	predecessor := ndb.getPreviousVersion(fromVersion)
+
+	for v, r := range ndb.versionReaders {
+		if v < toVersion && v > predecessor && r != 0 {
+			return errors.Errorf("unable to delete version %v with %v active readers", v, r)
+		}
+	}
+
+	// If the predecessor is earlier than the beginning of the lifetime, we can delete the orphan.
+	// Otherwise, we shorten its lifetime, by moving its endpoint to the predecessor version.
+	for version := fromVersion; version < toVersion; version++ {
+		ndb.traverseOrphansVersion(version, func(key, hash []byte) {
+			var from, to int64
+			orphanKeyFormat.Scan(key, &to, &from)
+			ndb.batch.Delete(key)
+			if from > predecessor {
+				ndb.batch.Delete(ndb.nodeKey(hash))
+				ndb.uncacheNode(hash)
+			} else {
+				ndb.saveOrphan(hash, from, predecessor)
+			}
+		})
+	}
+
+	// Delete the version root entries
+	ndb.traverseRange(rootKeyFormat.Key(fromVersion), rootKeyFormat.Key(toVersion), func(k, v []byte) {
+		ndb.batch.Delete(k)
+	})
+
+	return nil
+}
+
 // deleteNodesFrom deletes the given node and any descendants that have versions after the given
 // (inclusive). It is mainly used via LoadVersionForOverwriting, to delete the current version.
 func (ndb *nodeDB) deleteNodesFrom(version int64, hash []byte) error {

tree_random_test.go

@@ -41,13 +41,15 @@ func testRandomOperations(t *testing.T, randSeed int64) {
 		keySize   = 16 // before base64-encoding
 		valueSize = 16 // before base64-encoding
 
-		versions     = 32   // number of final versions to generate
-		reloadChance = 0.1  // chance of tree reload after save
-		deleteChance = 0.2  // chance of random version deletion after save
-		revertChance = 0.05 // chance to revert tree to random version with LoadVersionForOverwriting
-		syncChance   = 0.2  // chance of enabling sync writes on tree load
-		cacheChance  = 0.4  // chance of enabling caching
-		cacheSizeMax = 256  // maximum size of cache (will be random from 1)
+		versions            = 32   // number of final versions to generate
+		reloadChance        = 0.1  // chance of tree reload after save
+		deleteChance        = 0.2  // chance of random version deletion after save
+		revertChance        = 0.05 // chance to revert tree to random version with LoadVersionForOverwriting
+		syncChance          = 0.2  // chance of enabling sync writes on tree load
+		cacheChance         = 0.4  // chance of enabling caching
+		cacheSizeMax        = 256  // maximum size of cache (will be random from 1)
+		deleteRangeChance   = 0.5  // chance deletion versions in range
+		deleteRangeMaxBatch = 5    // small range to delete
 
 		versionOps  = 64  // number of operations (create/update/delete) per version
 		updateRatio = 0.4 // ratio of updates out of all operations
@@ -148,12 +150,30 @@ func testRandomOperations(t *testing.T, randSeed int64) {
 		if r.Float64() < deleteChance {
 			versions := getMirrorVersions(diskMirrors, memMirrors)
 			if len(versions) > 2 {
-				deleteVersion := int64(versions[r.Intn(len(versions)-1)])
-				t.Logf("Deleting version %v", deleteVersion)
-				err = tree.DeleteVersion(deleteVersion)
-				require.NoError(t, err)
-				delete(diskMirrors, deleteVersion)
-				delete(memMirrors, deleteVersion)
+				if r.Float64() < deleteRangeChance {
+					indexFrom := r.Intn(len(versions) - 1)
+					from := versions[indexFrom]
+					batch := r.Intn(deleteRangeMaxBatch)
+					if batch > len(versions[indexFrom:])-2 {
+						batch = len(versions[indexFrom:]) - 2
+					}
+					to := versions[indexFrom+batch] + 1
+					t.Logf("Deleting versions range %v - %v", from, to)
+					err = tree.DeleteVersionsRange(int64(from), int64(to))
+					require.NoError(t, err)
+					for version := from; version < to; version++ {
+						delete(diskMirrors, int64(version))
+						delete(memMirrors, int64(version))
+					}
+				} else {
+					i := r.Intn(len(versions) - 1)
+					deleteVersion := int64(versions[i])
+					t.Logf("Deleting version %v", deleteVersion)
+					err = tree.DeleteVersion(deleteVersion)
+					require.NoError(t, err)
+					delete(diskMirrors, deleteVersion)
+					delete(memMirrors, deleteVersion)
+				}
 			}
 		}
 
@@ -211,14 +231,23 @@ func testRandomOperations(t *testing.T, randSeed int64) {
 	// Once we're done, delete all prior versions in random order, make sure all orphans have been
 	// removed, and check that the latest versions matches the mirror.
 	remaining := tree.AvailableVersions()
-	remaining = remaining[:len(remaining)-1]
-	for len(remaining) > 0 {
-		i := r.Intn(len(remaining))
-		deleteVersion := int64(remaining[i])
-		remaining = append(remaining[:i], remaining[i+1:]...)
-		t.Logf("Deleting version %v", deleteVersion)
-		err = tree.DeleteVersion(deleteVersion)
-		require.NoError(t, err)
+
+	if r.Float64() < deleteRangeChance {
+		if len(remaining) > 0 {
+			t.Logf("Deleting versions range %v - %v", remaining[0], remaining[len(remaining)-2])
+			err = tree.DeleteVersionsRange(int64(remaining[0]), int64(remaining[len(remaining)-1]))
+			require.NoError(t, err)
+		}
+	} else {
+		remaining = remaining[:len(remaining)-1]
+		for len(remaining) > 0 {
+			i := r.Intn(len(remaining))
+			deleteVersion := int64(remaining[i])
+			remaining = append(remaining[:i], remaining[i+1:]...)
+			t.Logf("Deleting version %v", deleteVersion)
+			err = tree.DeleteVersion(deleteVersion)
+			require.NoError(t, err)
+		}
 	}
 	require.EqualValues(t, []int{int(version)}, tree.AvailableVersions())