From 0502ed86318815272335d00b19fd9614c52279c5 Mon Sep 17 00:00:00 2001
From: Tobias Schottdorf <tobias.schottdorf@gmail.com>
Date: Tue, 29 Oct 2019 15:38:29 +0100
Subject: [PATCH] storage: fix stats inconsistency due to WriteTooOld
 optimization

This fixes a bug introduced in #22315 with the introduction of a "local
retry" to avoid WriteTooOldError inside of 1PC transactions that were
not carrying out any reads. Such a transaction can simply be
re-evaluated at a higher timestamp, which can allow it to commit.

The bug was that when such a re-evaluation was carried out, we were
not discarding the MVCCStats accrued from the first attempt. In
effect, the request would thus be double-counted in stats, which
would set off the consistency checker.

The fix is easy: discard the delta before re-evaluating.

Fixes #31870.

Release note (bug fix): remove a source of (benign) stats
inconsistencies (i.e. the stats of a range not accurately reflecting its
contents).
---
 pkg/storage/replica_test.go  | 13 +++++++++++++
 pkg/storage/replica_write.go |  2 ++
 2 files changed, 15 insertions(+)

diff --git a/pkg/storage/replica_test.go b/pkg/storage/replica_test.go
index bb7f1ecd87fe..e50e1e5a6221 100644
--- a/pkg/storage/replica_test.go
+++ b/pkg/storage/replica_test.go
@@ -9380,6 +9380,19 @@ func TestReplicaLocalRetries(t *testing.T) {
 		if pErr != nil {
 			return hlc.Timestamp{}, pErr.GetDetail()
 		}
+
+		// Check that we didn't mess up the stats.
+		// Regression test for #31870.
+		snap := tc.engine.NewSnapshot()
+		defer snap.Close()
+		res, err := tc.repl.sha512(context.Background(), *tc.repl.Desc(), tc.engine, nil /* diff */, roachpb.ChecksumMode_CHECK_FULL)
+		if err != nil {
+			return hlc.Timestamp{}, err
+		}
+		if res.PersistedMS != res.RecomputedMS {
+			return hlc.Timestamp{}, errors.Errorf("stats are inconsistent:\npersisted:\n%+v\nrecomputed:\n%+v", res.PersistedMS, res.RecomputedMS)
+		}
+
 		return br.Timestamp, nil
 	}
 	get := func(key string) (hlc.Timestamp, error) {
diff --git a/pkg/storage/replica_write.go b/pkg/storage/replica_write.go
index 9ce99772ee0a..b44b66f9a40f 100644
--- a/pkg/storage/replica_write.go
+++ b/pkg/storage/replica_write.go
@@ -362,8 +362,10 @@ func (r *Replica) evaluateWriteBatchWithLocalRetries(
 	spans *spanset.SpanSet,
 	canRetry bool,
 ) (batch engine.Batch, br *roachpb.BatchResponse, res result.Result, pErr *roachpb.Error) {
+	goldenMS := *ms
 	for retries := 0; ; retries++ {
 		if batch != nil {
+			*ms = goldenMS
 			batch.Close()
 		}
 		batch = r.store.Engine().NewBatch()