From 66b6a8879d48c5ce037e6dab0a8090f1a56b175d 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..c6a473e08607 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 /* withRaftMu */)
 		}
 
 		rejected := cmd.Rejected()
diff --git a/pkg/kv/kvserver/replica_proposal.go b/pkg/kv/kvserver/replica_proposal.go
index 5837fa2bd751..c8d89c43ce7c 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, withRaftMu 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 withRaftMu {
+			return func() {}
+		}
+		withRaftMu = 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..59981424a1b4 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 /* withRaftMu */)
 
 		pr := proposalResult{
 			Reply:              proposal.Local.Reply,