From 0c1c6d912d943d6b8ba0c483497827413e9556e6 Mon Sep 17 00:00:00 2001
From: Erik Grinaker <grinaker@cockroachlabs.com>
Date: Fri, 12 Mar 2021 15:20:27 +0100
Subject: [PATCH] storage: add version check to
 IsSeparatedIntentsEnabledForTesting

`Engine.IsSeparatedIntentsEnabledForTesting()` is a test method to
detect whether separated intents are enabled (as these are randomly
enabled during tests). However, the function only checked the setting,
and failed to check whether the cluster version was recent enough,
causing it to return false positives.

This patch adds a cluster version check to fix this, which also required
changing the interface to thread a context through.

Release note: None
---
 pkg/kv/kvserver/replica_test.go |  3 ++-
 pkg/storage/engine.go           |  2 +-
 pkg/storage/mvcc_stats_test.go  | 14 +++++++-------
 pkg/storage/pebble.go           |  6 ++++--
 4 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/pkg/kv/kvserver/replica_test.go b/pkg/kv/kvserver/replica_test.go
index 2cb9dca00c33..f2c5743b1d52 100644
--- a/pkg/kv/kvserver/replica_test.go
+++ b/pkg/kv/kvserver/replica_test.go
@@ -6245,6 +6245,7 @@ func TestRangeStatsComputation(t *testing.T) {
 	stopper := stop.NewStopper()
 	defer stopper.Stop(context.Background())
 	tc.Start(t, stopper)
+	ctx := context.Background()
 
 	baseStats := initialStats()
 	// The initial stats contain an empty lease and no prior read summary, but
@@ -6311,7 +6312,7 @@ func TestRangeStatsComputation(t *testing.T) {
 		// Account for TxnDidNotUpdateMeta
 		expMS.LiveBytes += 2
 		expMS.ValBytes += 2
-		if tc.engine.IsSeparatedIntentsEnabledForTesting() {
+		if tc.engine.IsSeparatedIntentsEnabledForTesting(ctx) {
 			expMS.SeparatedIntentCount++
 		}
 	}
diff --git a/pkg/storage/engine.go b/pkg/storage/engine.go
index 2f544f83a99a..4826b9de49aa 100644
--- a/pkg/storage/engine.go
+++ b/pkg/storage/engine.go
@@ -715,7 +715,7 @@ type Engine interface {
 	// IsSeparatedIntentsEnabledForTesting is a test only method used in tests
 	// that know that this enabled setting is not changing and need the value to
 	// adjust their expectations.
-	IsSeparatedIntentsEnabledForTesting() bool
+	IsSeparatedIntentsEnabledForTesting(ctx context.Context) bool
 }
 
 // Batch is the interface for batch specific operations.
diff --git a/pkg/storage/mvcc_stats_test.go b/pkg/storage/mvcc_stats_test.go
index 2a7a31b632bd..147cacbafd25 100644
--- a/pkg/storage/mvcc_stats_test.go
+++ b/pkg/storage/mvcc_stats_test.go
@@ -200,7 +200,7 @@ func TestMVCCStatsPutCommitMovesTimestamp(t *testing.T) {
 			if accountForTxnDidNotUpdateMeta(t, engine) {
 				// Account for TxnDidNotUpdateMeta
 				mValSize += 2
-				if engine.IsSeparatedIntentsEnabledForTesting() {
+				if engine.IsSeparatedIntentsEnabledForTesting(ctx) {
 					separatedIntentCount = 1
 				}
 			}
@@ -289,7 +289,7 @@ func TestMVCCStatsPutPushMovesTimestamp(t *testing.T) {
 			if accountForTxnDidNotUpdateMeta(t, engine) {
 				// Account for TxnDidNotUpdateMeta
 				mValSize += 2
-				if engine.IsSeparatedIntentsEnabledForTesting() {
+				if engine.IsSeparatedIntentsEnabledForTesting(ctx) {
 					separatedIntentCount = 1
 				}
 			}
@@ -391,7 +391,7 @@ func TestMVCCStatsDeleteMovesTimestamp(t *testing.T) {
 			if accountForTxnDidNotUpdateMeta(t, engine) {
 				// Account for TxnDidNotUpdateMeta
 				mVal1Size += 2
-				if engine.IsSeparatedIntentsEnabledForTesting() {
+				if engine.IsSeparatedIntentsEnabledForTesting(ctx) {
 					separatedIntentCount = 1
 				}
 			}
@@ -521,7 +521,7 @@ func TestMVCCStatsPutMovesDeletionTimestamp(t *testing.T) {
 			if accountForTxnDidNotUpdateMeta(t, engine) {
 				// Account for TxnDidNotUpdateMeta
 				mVal1Size += 2
-				if engine.IsSeparatedIntentsEnabledForTesting() {
+				if engine.IsSeparatedIntentsEnabledForTesting(ctx) {
 					separatedIntentCount = 1
 				}
 			}
@@ -657,7 +657,7 @@ func TestMVCCStatsDelDelCommitMovesTimestamp(t *testing.T) {
 			if accountForTxnDidNotUpdateMeta(t, engine) {
 				// Account for TxnDidNotUpdateMeta
 				mValSize += 2
-				if engine.IsSeparatedIntentsEnabledForTesting() {
+				if engine.IsSeparatedIntentsEnabledForTesting(ctx) {
 					separatedIntentCount = 1
 				}
 			}
@@ -813,7 +813,7 @@ func TestMVCCStatsPutDelPutMovesTimestamp(t *testing.T) {
 			if accountForTxnDidNotUpdateMeta(t, engine) {
 				// Account for TxnDidNotUpdateMeta
 				mValSize += 2
-				if engine.IsSeparatedIntentsEnabledForTesting() {
+				if engine.IsSeparatedIntentsEnabledForTesting(ctx) {
 					separatedIntentCount = 1
 				}
 			}
@@ -1040,7 +1040,7 @@ func TestMVCCStatsPutIntentTimestampNotPutTimestamp(t *testing.T) {
 			if accountForTxnDidNotUpdateMeta(t, engine) {
 				// Account for TxnDidNotUpdateMeta
 				m1ValSize += 2
-				if engine.IsSeparatedIntentsEnabledForTesting() {
+				if engine.IsSeparatedIntentsEnabledForTesting(ctx) {
 					separatedIntentCount = 1
 				}
 			}
diff --git a/pkg/storage/pebble.go b/pkg/storage/pebble.go
index a85e0e1d3557..200f7be7bc8c 100644
--- a/pkg/storage/pebble.go
+++ b/pkg/storage/pebble.go
@@ -24,6 +24,7 @@ import (
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
@@ -900,8 +901,9 @@ func (p *Pebble) SafeToWriteSeparatedIntents(ctx context.Context) (bool, error)
 }
 
 // IsSeparatedIntentsEnabledForTesting implements the Engine interface.
-func (p *Pebble) IsSeparatedIntentsEnabledForTesting() bool {
-	return SeparatedIntentsEnabled.Get(&p.settings.SV)
+func (p *Pebble) IsSeparatedIntentsEnabledForTesting(ctx context.Context) bool {
+	return !p.settings.Version.ActiveVersionOrEmpty(ctx).Less(
+		clusterversion.ByKey(clusterversion.SeparatedIntents)) && SeparatedIntentsEnabled.Get(&p.settings.SV)
 }
 
 func (p *Pebble) put(key MVCCKey, value []byte) error {