From 584fb971e3bbdaa750b54ad6425748e9673058b7 Mon Sep 17 00:00:00 2001
From: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Date: Mon, 26 Jul 2021 01:40:33 -0400
Subject: [PATCH] kv: grab raftMu during no-op writes with local gossip
 triggers

Fixes #68011.

As of 9f8c019, it is now possible to have no-op writes that do not go through
Raft but do set one of the gossip triggers. These gossip triggers require the
raftMu to be held, so we were running into trouble when handling the local
eval results above Raft.

For instance, we see this case when a transaction sets the system config
trigger and then performs a delete range over an empty span before
committing. In this case, the transaction will have no intents to
remove, so it can auto-GC its record during an EndTxn. If its record was
never written in the first place, this is a no-op (as of 9f8c019).

There appear to be three ways we could solve this:
1. we can avoid setting gossip triggers on transactions that don't perform
   any writes.
2. we can force EndTxn requests with gossip triggers to go through Raft even
   if they are otherwise no-ops.
3. we can properly handle gossip triggers on the above Raft local eval result
   path.

This commit opts for the third option.
---
 .../replica_application_state_machine.go       |  2 +-
 pkg/kv/kvserver/replica_proposal.go            | 18 +++++++++++++++++-
 pkg/kv/kvserver/replica_raft.go                |  2 +-
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/pkg/kv/kvserver/replica_application_state_machine.go b/pkg/kv/kvserver/replica_application_state_machine.go
index 3d4fc8d53fc7..80de1ca9c959 100644
--- a/pkg/kv/kvserver/replica_application_state_machine.go
+++ b/pkg/kv/kvserver/replica_application_state_machine.go
@@ -1206,7 +1206,7 @@ func (sm *replicaStateMachine) ApplySideEffects(
 	if cmd.IsLocal() {
 		// Handle the LocalResult.
 		if cmd.localResult != nil {
-			sm.r.handleReadWriteLocalEvalResult(ctx, *cmd.localResult)
+			sm.r.handleReadWriteLocalEvalResult(ctx, *cmd.localResult, true /* raftMuHeld */)
 		}
 
 		rejected := cmd.Rejected()
diff --git a/pkg/kv/kvserver/replica_proposal.go b/pkg/kv/kvserver/replica_proposal.go
index 5837fa2bd751..f8f3fbcc3238 100644
--- a/pkg/kv/kvserver/replica_proposal.go
+++ b/pkg/kv/kvserver/replica_proposal.go
@@ -641,7 +641,9 @@ func addSSTablePreApply(
 	return copied
 }
 
-func (r *Replica) handleReadWriteLocalEvalResult(ctx context.Context, lResult result.LocalResult) {
+func (r *Replica) handleReadWriteLocalEvalResult(
+	ctx context.Context, lResult result.LocalResult, raftMuHeld bool,
+) {
 	// Fields for which no action is taken in this method are zeroed so that
 	// they don't trigger an assertion at the end of the method (which checks
 	// that all fields were handled).
@@ -707,7 +709,19 @@ func (r *Replica) handleReadWriteLocalEvalResult(ctx context.Context, lResult re
 		lResult.MaybeAddToSplitQueue = false
 	}
 
+	// The following three triggers require the raftMu to be held. If a
+	// trigger is present, acquire the mutex if it is not held already.
+	maybeAcquireRaftMu := func() func() {
+		if raftMuHeld {
+			return func() {}
+		}
+		raftMuHeld = true
+		r.raftMu.Lock()
+		return r.raftMu.Unlock
+	}
+
 	if lResult.MaybeGossipSystemConfig {
+		defer maybeAcquireRaftMu()()
 		if err := r.MaybeGossipSystemConfigRaftMuLocked(ctx); err != nil {
 			log.Errorf(ctx, "%v", err)
 		}
@@ -715,6 +729,7 @@ func (r *Replica) handleReadWriteLocalEvalResult(ctx context.Context, lResult re
 	}
 
 	if lResult.MaybeGossipSystemConfigIfHaveFailure {
+		defer maybeAcquireRaftMu()()
 		if err := r.MaybeGossipSystemConfigIfHaveFailureRaftMuLocked(ctx); err != nil {
 			log.Errorf(ctx, "%v", err)
 		}
@@ -722,6 +737,7 @@ func (r *Replica) handleReadWriteLocalEvalResult(ctx context.Context, lResult re
 	}
 
 	if lResult.MaybeGossipNodeLiveness != nil {
+		defer maybeAcquireRaftMu()()
 		if err := r.MaybeGossipNodeLivenessRaftMuLocked(ctx, *lResult.MaybeGossipNodeLiveness); err != nil {
 			log.Errorf(ctx, "%v", err)
 		}
diff --git a/pkg/kv/kvserver/replica_raft.go b/pkg/kv/kvserver/replica_raft.go
index 67125e639c3a..05a7d0b04bcd 100644
--- a/pkg/kv/kvserver/replica_raft.go
+++ b/pkg/kv/kvserver/replica_raft.go
@@ -115,7 +115,7 @@ func (r *Replica) evalAndPropose(
 	if proposal.command == nil {
 		intents := proposal.Local.DetachEncounteredIntents()
 		endTxns := proposal.Local.DetachEndTxns(pErr != nil /* alwaysOnly */)
-		r.handleReadWriteLocalEvalResult(ctx, *proposal.Local)
+		r.handleReadWriteLocalEvalResult(ctx, *proposal.Local, false /* raftMuHeld */)
 
 		pr := proposalResult{
 			Reply:              proposal.Local.Reply,