From cccf8825558cf8005a3d41ef8f6de4d58c9753d2 Mon Sep 17 00:00:00 2001
From: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Date: Thu, 7 Dec 2017 18:28:05 -0500
Subject: [PATCH] storage: remove unnecessary sideload inlining, add assertion

`Replica.sendRaftMessages` used to contain logic to inline sideloaded entries
for `MsgApps`. This was unnecessary, as `MsgApps` would never contain thin entries.
This is because `replicaRaftStorage.Entries` already performs the sideload inlining
for stable entries and `raft.unstable` always contain fat entries. Since these are
the only two sources that `raft.sendAppend` gathers entries from to populate `MsgApps`,
we should never see thin entries in `MsgApps`.

This change replaces the inlining attempt with an assertion.

Release note: None
---
 pkg/storage/replica.go          | 42 +++++++--------------------------
 pkg/storage/replica_sideload.go | 21 +++++++++++++++++
 2 files changed, 30 insertions(+), 33 deletions(-)

diff --git a/pkg/storage/replica.go b/pkg/storage/replica.go
index 711fb3d6787e..d6052bfa02ed 100644
--- a/pkg/storage/replica.go
+++ b/pkg/storage/replica.go
@@ -3504,10 +3504,6 @@ func (r *Replica) handleRaftReadyRaftMuLocked(
 	// Update protected state (last index, last term, raft log size and raft
 	// leader ID) and set raft log entry cache. We clear any older, uncommitted
 	// log entries and cache the latest ones.
-	//
-	// Note also that we're likely to send messages related to the Entries we
-	// just appended, and these entries need to be inlined when sending them to
-	// followers - populating the cache here saves a lot of that work.
 	r.mu.Lock()
 	r.store.raftEntryCache.addEntries(r.RangeID, rd.Entries)
 	r.mu.lastIndex = lastIndex
@@ -4180,35 +4176,15 @@ func (r *Replica) sendRaftMessages(ctx context.Context, messages []raftpb.Messag
 		drop := false
 		switch message.Type {
 		case raftpb.MsgApp:
-			// Iterate over the entries to inline sideloaded commands.
-			for j := range message.Entries {
-				cow := false
-				newEnt, err := maybeInlineSideloadedRaftCommand(
-					ctx,
-					r.RangeID,
-					message.Entries[j],
-					r.raftMu.sideloaded,
-					r.store.raftEntryCache,
-				)
-				if err != nil {
-					// We can simply drop the message since it could always get lost
-					// in transit anyway.
-					log.Errorf(ctx, "while inlining sideloaded commands: %s", err)
-					drop = true
-					continue
-				}
-				if newEnt != nil {
-					if !cow {
-						cow = true
-						// Copy the whole slice to avoid data races. Entries are
-						// usually shared between multiple outgoing messages,
-						// and while it would be possible to only modify them
-						// only the first time around, that isn't easy to
-						// implement (since you have to do nontrivial work to
-						// decide whether inlining needs to happen).
-						message.Entries = append([]raftpb.Entry(nil), message.Entries...)
-					}
-					message.Entries[j] = *newEnt
+			if util.RaceEnabled {
+				// Iterate over the entries to assert that all sideloaded commands
+				// are already inlined. replicaRaftStorage.Entries already performs
+				// the sideload inlining for stable entries and raft.unstable always
+				// contain fat entries. Since these are the only two sources that
+				// raft.sendAppend gathers entries from to populate MsgApps, we
+				// should never see thin entries here.
+				for j := range message.Entries {
+					assertSideloadedRaftCommandInlined(ctx, &message.Entries[j])
 				}
 			}
 
diff --git a/pkg/storage/replica_sideload.go b/pkg/storage/replica_sideload.go
index 41fa33b09b7a..0aa9b84ef57e 100644
--- a/pkg/storage/replica_sideload.go
+++ b/pkg/storage/replica_sideload.go
@@ -261,3 +261,24 @@ func maybeInlineSideloadedRaftCommand(
 	}
 	return &ent, nil
 }
+
+// assertSideloadedRaftCommandInlined asserts that if the provided entry is a
+// sideloaded entry, then its payload has already been inlined. Doing so
+// requires unmarshalling the raft command, so this assertion should be kept out
+// of performance critical paths.
+func assertSideloadedRaftCommandInlined(ctx context.Context, ent *raftpb.Entry) {
+	if !sniffSideloadedRaftCommand(ent.Data) {
+		return
+	}
+
+	var command storagebase.RaftCommand
+	_, data := DecodeRaftCommand(ent.Data)
+	if err := protoutil.Unmarshal(data, &command); err != nil {
+		log.Fatal(ctx, err)
+	}
+
+	if len(command.ReplicatedEvalResult.AddSSTable.Data) == 0 {
+		// The entry is "thin", which is what this assertion is checking for.
+		log.Fatalf(ctx, "found thin sideloaded raft command: %+v", command)
+	}
+}