Merge pull request #43942 from nvanbenschoten/backport19.1-43937

release-19.1: storage/engine: fix re-issued KV writes with most recent seq num

pkg/storage/engine/enginepb/mvcc.go

@@ -144,7 +144,7 @@ func (meta *MVCCMetadata) GetPrevIntentSeq(seq int32) (int32, bool) {
 	index := sort.Search(len(meta.IntentHistory), func(i int) bool {
 		return meta.IntentHistory[i].Sequence >= seq
 	})
-	if index > 0 && index < len(meta.IntentHistory) {
+	if index > 0 {
 		return meta.IntentHistory[index-1].Sequence, true
 	}
 	return 0, false

pkg/storage/engine/enginepb/mvcc_test.go

@@ -0,0 +1,70 @@
+// Copyright 2020 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package enginepb
+
+import (
+	fmt "fmt"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestMetadataGetPrevIntentSeq(t *testing.T) {
+	for _, tc := range []struct {
+		history      []int32
+		search       int32
+		expPrevSeq   int32
+		expPrevFound bool
+	}{
+		{[]int32{}, 0, 0, false},
+		{[]int32{}, 1, 0, false},
+		{[]int32{}, 2, 0, false},
+		{[]int32{}, 3, 0, false},
+		{[]int32{0}, 0, 0, false},
+		{[]int32{0}, 1, 0, true},
+		{[]int32{0}, 2, 0, true},
+		{[]int32{0}, 3, 0, true},
+		{[]int32{1}, 0, 0, false},
+		{[]int32{1}, 1, 0, false},
+		{[]int32{1}, 2, 1, true},
+		{[]int32{1}, 3, 1, true},
+		{[]int32{0, 1}, 0, 0, false},
+		{[]int32{0, 1}, 1, 0, true},
+		{[]int32{0, 1}, 2, 1, true},
+		{[]int32{0, 1}, 3, 1, true},
+		{[]int32{0, 2}, 0, 0, false},
+		{[]int32{0, 2}, 1, 0, true},
+		{[]int32{0, 2}, 2, 0, true},
+		{[]int32{0, 2}, 3, 2, true},
+		{[]int32{1, 2}, 0, 0, false},
+		{[]int32{1, 2}, 1, 0, false},
+		{[]int32{1, 2}, 2, 1, true},
+		{[]int32{1, 2}, 3, 2, true},
+		{[]int32{0, 1, 2}, 0, 0, false},
+		{[]int32{0, 1, 2}, 1, 0, true},
+		{[]int32{0, 1, 2}, 2, 1, true},
+		{[]int32{0, 1, 2}, 3, 2, true},
+	} {
+		name := fmt.Sprintf("%v/%d", tc.history, tc.search)
+		t.Run(name, func(t *testing.T) {
+			var meta MVCCMetadata
+			for _, seq := range tc.history {
+				meta.IntentHistory = append(meta.IntentHistory, MVCCMetadata_SequencedIntent{
+					Sequence: seq,
+				})
+			}
+
+			prevSeq, prevFound := meta.GetPrevIntentSeq(tc.search)
+			require.Equal(t, tc.expPrevSeq, prevSeq)
+			require.Equal(t, tc.expPrevFound, prevFound)
+		})
+	}
+}

pkg/storage/engine/mvcc_test.go

@@ -5126,7 +5126,7 @@ func TestMVCCIdempotentTransactions(t *testing.T) {
 	}
 	txn.Sequence += 3
 
-	// on a separate key, start an increment.
+	// On a separate key, start an increment.
 	val, err := MVCCIncrement(ctx, engine, nil, testKey1, txn.OrigTimestamp, txn, 1)
 	if val != 1 || err != nil {
 		t.Fatalf("expected val=1 (got %d): %s", val, err)
@@ -5146,8 +5146,17 @@ func TestMVCCIdempotentTransactions(t *testing.T) {
 	if val != 2 || err != nil {
 		t.Fatalf("expected val=2 (got %d): %s", val, err)
 	}
-	txn.Sequence--
+	// Replaying the latest increment doesn't increase the value.
+	// This is a regression test against #43928. Before that was fixed, the
+	// function would return a "different value after recomputing" error.
+	for i := 0; i < 10; i++ {
+		val, err = MVCCIncrement(ctx, engine, nil, testKey1, txn.OrigTimestamp, txn, 1)
+		if val != 2 || err != nil {
+			t.Fatalf("expected val=2 (got %d): %+v", val, err)
+		}
+	}
 	// Replaying an older increment doesn't increase the value.
+	txn.Sequence--
 	for i := 0; i < 10; i++ {
 		val, err = MVCCIncrement(ctx, engine, nil, testKey1, txn.OrigTimestamp, txn, 1)
 		if val != 1 || err != nil {