Support for skipping subtree revisions to increase read performance a…

…nd reduce disk usage (#3201)

TL;DR: existing trees will continue to be stored and queried as they were before, but new trees
created with the MySQL storage layer will be stored and queried in a way that uses less space
and allows for simpler and faster queries. No schema changes are required by log operators.

The Trillian MySQL implementation stores the internal state of the log as Subtrees in the database.
These are essentially tiles as described by [tlog: Tiling a log](https://research.swtch.com/tlog).
Trees created with previous versions of Trillian stored a different revision of each Subtree when
the tree was updated. This is somewhat redundant for append-only logs because an earlier version
of a Subtree can always be derived from a later one by simply removing entries from the right of
the Subtree. This change removes this Subtree revision history, and updates Subtrees in place when
they are updated.

Measurements from @n-canter show that revisionless storage saves around 75% storage costs for the
Subtree table, and queries over this table are more than 15% faster.

The same schema is used for both revisioned and unrevisioned subtrees. The difference is that we
always write a revision of 0 in the unrevisioned case, which still means that there will only be
a single entry per subtree.

Support is maintained for the old way of revisioning Subtrees in order to avoid breaking changes
to existing trees. There is no simple code change that would safely allow a previously revisioned
tree to start becoming a revisionless tree. This new revisionless Subtree feature is only available
for trees created with new versions of Trillian.

Users with legacy revisioned trees that wish to take advantage of smaller storage costs and faster
queries of the new revisionless storage should come speak to us on
[transparency-dev Slack](https://join.slack.com/t/transparency-dev/shared_invite/zt-27pkqo21d-okUFhur7YZ0rFoJVIOPznQ).
The safest option we have available is to use [migrillian](https://github.com/google/certificate-transparency-go/tree/master/trillian/migrillian) to create a new copy of trees, but this will be quite a manual
process and will only work for CT logs.
Other migration options are conceivable and we're eager to work with the community to develop
and test tools for upgrading trees in place.

CHANGELOG.md

@@ -5,6 +5,42 @@
 * Bump CI version of MySQL from 5.7 to 8.0
 * Bump golangci-lint from 1.51.1 to 1.55.1 (developers should update to this version)
 
+### MySQL: Changes to Subtree Revisions
+
+Support for skipping subtree revisions to increase read performance and reduce disk usage: added in #3201
+
+TL;DR: existing trees will continue to be stored and queried as they were before, but new trees
+created with the MySQL storage layer will be stored and queried in a way that uses less space
+and allows for simpler and faster queries. No schema changes are required by log operators.
+
+The Trillian MySQL implementation stores the internal state of the log as Subtrees in the database.
+These are essentially tiles as described by [tlog: Tiling a log](https://research.swtch.com/tlog).
+Trees created with previous versions of Trillian stored a different revision of each Subtree when
+the tree was updated. This is somewhat redundant for append-only logs because an earlier version
+of a Subtree can always be derived from a later one by simply removing entries from the right of
+the Subtree. PR #3201 removes this Subtree revision history, and updates Subtrees in place when
+they are updated.
+
+Measurements from @n-canter show that revisionless storage saves around 75% storage costs for the
+Subtree table, and queries over this table are more than 15% faster.
+
+The same schema is used for both revisioned and unrevisioned subtrees. The difference is that we
+always write a revision of 0 in the unrevisioned case, which still means that there will only be
+a single entry per subtree.
+
+Support is maintained for the old way of revisioning Subtrees in order to avoid breaking changes
+to existing trees. There is no simple code change that would safely allow a previously revisioned
+tree to start becoming a revisionless tree. This new revisionless Subtree feature is only available
+for trees created with new versions of Trillian.
+
+Users with legacy revisioned trees that wish to take advantage of smaller storage costs and faster
+queries of the new revisionless storage should come speak to us on
+[transparency-dev Slack](https://join.slack.com/t/transparency-dev/shared_invite/zt-27pkqo21d-okUFhur7YZ0rFoJVIOPznQ).
+The safest option we have available is to use [migrillian](https://github.com/google/certificate-transparency-go/tree/master/trillian/migrillian) to create a new copy of trees, but this will be quite a manual
+process and will only work for CT logs.
+Other migration options are conceivable and we're eager to work with the community to develop
+and test tools for upgrading trees in place.
+
 ## v1.5.3
 
 * Recommended go version for development: 1.20

integration/admin/admin_integration_test.go

@@ -220,8 +220,9 @@ func TestAdminServer_UpdateTree(t *testing.T) {
 		want.TreeId = tree.TreeId
 		want.CreateTime = tree.CreateTime
 		want.UpdateTime = tree.UpdateTime
+		want.StorageSettings = tree.StorageSettings
 		if !proto.Equal(tree, want) {
-			diff := cmp.Diff(tree, want)
+			diff := cmp.Diff(tree, want, cmp.Comparer(proto.Equal))
 			t.Errorf("%v: post-UpdateTree diff:\n%v", test.desc, diff)
 		}
 	}
@@ -357,7 +358,7 @@ func TestAdminServer_DeleteTree(t *testing.T) {
 		want.Deleted = true
 		want.DeleteTime = deletedTree.DeleteTime
 		if got := deletedTree; !proto.Equal(got, want) {
-			diff := cmp.Diff(got, want)
+			diff := cmp.Diff(got, want, cmp.Comparer(proto.Equal))
 			t.Errorf("%v: post-DeleteTree() diff (-got +want):\n%v", test.desc, diff)
 		}
 
@@ -366,7 +367,7 @@ func TestAdminServer_DeleteTree(t *testing.T) {
 			t.Fatalf("%v: GetTree() returned err = %v", test.desc, err)
 		}
 		if got, want := storedTree, deletedTree; !proto.Equal(got, want) {
-			diff := cmp.Diff(got, want)
+			diff := cmp.Diff(got, want, cmp.Comparer(proto.Equal))
 			t.Errorf("%v: post-GetTree() diff (-got +want):\n%v", test.desc, diff)
 		}
 	}
@@ -447,7 +448,7 @@ func TestAdminServer_UndeleteTree(t *testing.T) {
 			continue
 		}
 		if got, want := undeletedTree, createdTree; !proto.Equal(got, want) {
-			diff := cmp.Diff(got, want)
+			diff := cmp.Diff(got, want, cmp.Comparer(proto.Equal))
 			t.Errorf("%v: post-UndeleteTree() diff (-got +want):\n%v", test.desc, diff)
 		}
 
@@ -456,7 +457,7 @@ func TestAdminServer_UndeleteTree(t *testing.T) {
 			t.Fatalf("%v: GetTree() returned err = %v", test.desc, err)
 		}
 		if got, want := storedTree, createdTree; !proto.Equal(got, want) {
-			diff := cmp.Diff(got, want)
+			diff := cmp.Diff(got, want, cmp.Comparer(proto.Equal))
 			t.Errorf("%v: post-GetTree() diff (-got +want):\n%v", test.desc, diff)
 		}
 	}

storage/mysql/admin_storage.go

@@ -15,17 +15,21 @@
 package mysql
 
 import (
+	"bytes"
 	"context"
 	"database/sql"
+	"encoding/gob"
 	"fmt"
 	"sync"
 	"time"
 
 	"github.com/google/trillian"
 	"github.com/google/trillian/storage"
+	"github.com/google/trillian/storage/mysql/mysqlpb"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/proto"
+	"google.golang.org/protobuf/types/known/anypb"
 	"google.golang.org/protobuf/types/known/timestamppb"
 	"k8s.io/klog/v2"
 )
@@ -47,8 +51,8 @@ const (
 			Description,
 			CreateTimeMillis,
 			UpdateTimeMillis,
-			PrivateKey,
-			PublicKey,
+			PrivateKey, -- Unused
+			PublicKey, -- Used to store StorageSettings
 			MaxRootDurationMillis,
 			Deleted,
 			DeleteTimeMillis
@@ -62,7 +66,7 @@ const (
 )
 
 // NewAdminStorage returns a MySQL storage.AdminStorage implementation backed by DB.
-func NewAdminStorage(db *sql.DB) storage.AdminStorage {
+func NewAdminStorage(db *sql.DB) *mysqlAdminStorage {
 	return &mysqlAdminStorage{db}
 }
 
@@ -223,6 +227,39 @@ func (t *adminTX) CreateTree(ctx context.Context, tree *trillian.Tree) (*trillia
 	}
 	rootDuration := newTree.MaxRootDuration.AsDuration()
 
+	// When creating a new tree we automatically add StorageSettings to allow us to
+	// determine that this tree can support newer storage features. When reading
+	// trees that do not have this StorageSettings populated, it must be assumed that
+	// the tree was created with the oldest settings.
+	// The gist of this code is super simple: create a new StorageSettings with the most
+	// modern defaults if the created tree does not have one, and then create a struct that
+	// represents this to store in the DB. Unfortunately because this involves anypb, struct
+	// copies, marshalling, and proper error handling this turns into a scary amount of code.
+	if tree.StorageSettings != nil {
+		newTree.StorageSettings = proto.Clone(tree.StorageSettings).(*anypb.Any)
+	} else {
+		o := &mysqlpb.StorageOptions{
+			SubtreeRevisions: false, // Default behaviour for new trees is to skip writing subtree revisions.
+		}
+		a, err := anypb.New(o)
+		if err != nil {
+			return nil, fmt.Errorf("failed to create new StorageOptions: %v", err)
+		}
+		newTree.StorageSettings = a
+	}
+	o := &mysqlpb.StorageOptions{}
+	if err := anypb.UnmarshalTo(newTree.StorageSettings, o, proto.UnmarshalOptions{}); err != nil {
+		return nil, fmt.Errorf("failed to unmarshal StorageOptions: %v", err)
+	}
+	ss := storageSettings{
+		Revisioned: o.SubtreeRevisions,
+	}
+	buff := &bytes.Buffer{}
+	enc := gob.NewEncoder(buff)
+	if err := enc.Encode(ss); err != nil {
+		return nil, fmt.Errorf("failed to encode storageSettings: %v", err)
+	}
+
 	insertTreeStmt, err := t.tx.PrepareContext(
 		ctx,
 		`INSERT INTO Trees(
@@ -236,8 +273,8 @@ func (t *adminTX) CreateTree(ctx context.Context, tree *trillian.Tree) (*trillia
 			Description,
 			CreateTimeMillis,
 			UpdateTimeMillis,
-			PrivateKey,
-			PublicKey,
+			PrivateKey, -- Unused
+			PublicKey, -- Used to store StorageSettings
 			MaxRootDurationMillis)
 		VALUES(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`)
 	if err != nil {
@@ -261,8 +298,8 @@ func (t *adminTX) CreateTree(ctx context.Context, tree *trillian.Tree) (*trillia
 		newTree.Description,
 		nowMillis,
 		nowMillis,
-		[]byte{}, // Unused, filling in for backward compatibility.
-		[]byte{}, // Unused, filling in for backward compatibility.
+		[]byte{},     // PrivateKey: Unused, filling in for backward compatibility.
+		buff.Bytes(), // Using the otherwise unused PublicKey for storing StorageSettings.
 		rootDuration/time.Millisecond,
 	)
 	if err != nil {
@@ -356,7 +393,10 @@ func (t *adminTX) UpdateTree(ctx context.Context, treeID int64, updateFunc func(
 		tree.Description,
 		nowMillis,
 		rootDuration/time.Millisecond,
-		[]byte{}, // Unused, filling in for backward compatibility.
+		[]byte{}, // PrivateKey: Unused, filling in for backward compatibility.
+		// PublicKey should not be updated with any storageSettings here without
+		// a lot of thought put into it. At the moment storageSettings are inferred
+		// when reading the tree, even if no value is stored in the database.
 		tree.TreeId); err != nil {
 		return nil, err
 	}
@@ -419,8 +459,21 @@ func validateDeleted(ctx context.Context, tx *sql.Tx, treeID int64, wantDeleted
 }
 
 func validateStorageSettings(tree *trillian.Tree) error {
-	if tree.StorageSettings != nil {
-		return fmt.Errorf("storage_settings not supported, but got %v", tree.StorageSettings)
+	if tree.StorageSettings.MessageIs(&mysqlpb.StorageOptions{}) {
+		return nil
 	}
-	return nil
+	if tree.StorageSettings == nil {
+		// No storage settings is OK, we'll just use the defaults for new trees
+		return nil
+	}
+	return fmt.Errorf("storage_settings must be nil or mysqlpb.StorageOptions, but got %v", tree.StorageSettings)
+}
+
+// storageSettings allows us to persist storage settings to the DB.
+// It is a tempting trap to use protos for this, but the way they encode
+// makes it impossible to tell the difference between no value ever written
+// and a value that was written with the default values for each field.
+// Using an explicit struct and gob encoding allows us to tell the difference.
+type storageSettings struct {
+	Revisioned bool
 }