kvserver, storage: Always enable separated intents.

This change moves the SeparatedIntentsEnabled setting to
a testing knob, away from the cluster setting. It also
removes all logic to handle pre-SeparatedIntents cluster
versions, as that was part of the 20.2 release and 21.2
does not need to accommodate that distinction. We always
assume we're in a new-enough cluster version that can read
separated intents.

The only way interleaved intents can
be written now is when the `DisableSeparatedIntents` testing
knob is true.

Release note: None.

pkg/base/config.go

@@ -515,6 +515,9 @@ type StorageConfig struct {
 	// UseFileRegistry is true if the file registry is needed (eg: encryption-at-rest).
 	// This may force the store version to versionFileRegistry if currently lower.
 	UseFileRegistry bool
+	// DisableSeparatedIntents is true if separated intents should not be written
+	// by intent writers. Only true for tests.
+	DisableSeparatedIntents bool
 	// EncryptionOptions is a serialized protobuf set by Go CCL code and passed
 	// through to C CCL code to set up encryption-at-rest.  Must be set if and
 	// only if encryption is enabled, otherwise left empty.

pkg/kv/kvserver/batcheval/cmd_revert_range_test.go

@@ -55,7 +55,7 @@ func getStats(t *testing.T, reader storage.Reader) enginepb.MVCCStats {
 // createTestPebbleEngine returns a new in-memory Pebble storage engine.
 func createTestPebbleEngine(ctx context.Context) (storage.Engine, error) {
 	return storage.Open(ctx, storage.InMemory(),
-		storage.MaxSize(1<<20), storage.SettingsForTesting())
+		storage.MaxSize(1<<20), storage.ForTesting)
 }
 
 var engineImpls = []struct {

pkg/kv/kvserver/replica_test.go

@@ -238,7 +238,7 @@ func (tc *testContext) StartWithStoreConfigAndVersion(
 			storage.InMemory(),
 			storage.Attributes(roachpb.Attributes{Attrs: []string{"dc1", "mem"}}),
 			storage.MaxSize(1<<20),
-			storage.SettingsForTesting())
+			storage.ForTesting)
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -6357,12 +6357,6 @@ func verifyRangeStats(
 	return nil
 }
 
-func accountForTxnDidNotUpdateMeta(t *testing.T, eng storage.Engine) bool {
-	accountFor, err := eng.SafeToWriteSeparatedIntents(context.Background())
-	require.NoError(t, err)
-	return accountFor
-}
-
 // TestRangeStatsComputation verifies that commands executed against a
 // range update the range stat counters. The stat values are
 // empirically derived; we're really just testing that they increment
@@ -6431,22 +6425,17 @@ func TestRangeStatsComputation(t *testing.T) {
 	}
 	expMS = baseStats
 	expMS.Add(enginepb.MVCCStats{
-		LiveBytes:   101,
+		LiveBytes:   103,
 		KeyBytes:    28,
-		ValBytes:    73,
+		ValBytes:    75,
 		IntentBytes: 23,
 		LiveCount:   2,
 		KeyCount:    2,
 		ValCount:    2,
 		IntentCount: 1,
 	})
-	if accountForTxnDidNotUpdateMeta(t, tc.engine) {
-		// Account for TxnDidNotUpdateMeta
-		expMS.LiveBytes += 2
-		expMS.ValBytes += 2
-		if tc.engine.IsSeparatedIntentsEnabledForTesting(ctx) {
-			expMS.SeparatedIntentCount++
-		}
+	if tc.engine.IsSeparatedIntentsEnabledForTesting(ctx) {
+		expMS.SeparatedIntentCount++
 	}
 	if err := verifyRangeStats(tc.engine, tc.repl.RangeID, expMS); err != nil {
 		t.Fatal(err)

pkg/kv/kvserver/spanset/batch.go

@@ -644,10 +644,6 @@ func (s spanSetWriter) PutEngineKey(key storage.EngineKey, value []byte) error {
 	return s.w.PutEngineKey(key, value)
 }
 
-func (s spanSetWriter) SafeToWriteSeparatedIntents(ctx context.Context) (bool, error) {
-	return s.w.SafeToWriteSeparatedIntents(ctx)
-}
-
 func (s spanSetWriter) LogData(data []byte) error {
 	return s.w.LogData(data)
 }

pkg/kv/kvserver/testing_knobs.go

@@ -19,6 +19,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/tenantrate"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/txnwait"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
 )
@@ -35,6 +36,7 @@ type StoreTestingKnobs struct {
 	TxnWaitKnobs            txnwait.TestingKnobs
 	ConsistencyTestingKnobs ConsistencyTestingKnobs
 	TenantRateKnobs         tenantrate.TestingKnobs
+	StorageKnobs            storage.TestingKnobs
 
 	// TestingRequestFilter is called before evaluating each request on a
 	// replica. The filter is run before the request acquires latches, so

pkg/migration/migrations/separated_intents_external_test.go

@@ -12,7 +12,8 @@ package migrations_test
 
 import (
 	"context"
-	"strconv"
+	"fmt"
+	"path"
 	"testing"
 	"time"
 
@@ -44,26 +45,28 @@ func TestSeparatedIntentsMigration(t *testing.T) {
 		clusterversion.ByKey(clusterversion.SeparatedIntentsMigration-1),
 		false, /* initializeVersion */
 	)
-	storage.SeparatedIntentsEnabled.Override(ctx, &settings.SV, false)
-	stickyEngineRegistry := server.NewStickyInMemEnginesRegistry()
-	defer stickyEngineRegistry.CloseAllStickyInMemEngines()
 	const numServers int = 3
 	stickyServerArgs := make(map[int]base.TestServerArgs)
+	storeKnobs := &kvserver.StoreTestingKnobs{
+		StorageKnobs: storage.TestingKnobs{DisableSeparatedIntents: true},
+	}
+	tempDir, cleanup := testutils.TempDir(t)
+	defer cleanup()
 	for i := 0; i < numServers; i++ {
 		stickyServerArgs[i] = base.TestServerArgs{
 			Settings: settings,
 			StoreSpecs: []base.StoreSpec{
 				{
-					InMemory:               true,
-					StickyInMemoryEngineID: strconv.FormatInt(int64(i), 10),
+					InMemory: false,
+					Path:     path.Join(tempDir, fmt.Sprintf("engine-%d", i)),
 				},
 			},
 			Knobs: base.TestingKnobs{
 				Server: &server.TestingKnobs{
 					DisableAutomaticVersionUpgrade: 1,
 					BinaryVersionOverride:          clusterversion.ByKey(clusterversion.SeparatedIntentsMigration - 1),
-					StickyEngineRegistry:           stickyEngineRegistry,
 				},
+				Store: storeKnobs,
 			},
 		}
 	}
@@ -143,7 +146,8 @@ func TestSeparatedIntentsMigration(t *testing.T) {
 	}
 	require.True(t, interleavedIntentFound)
 
-	storage.SeparatedIntentsEnabled.Override(ctx, &settings.SV, true)
+	// Start writing separated intents.
+	storeKnobs.StorageKnobs.DisableSeparatedIntents = false
 	require.NoError(t, tc.Restart())
 	time.Sleep(10 * time.Second)
 	tdb = tc.ServerConn(0)

pkg/server/config.go

@@ -479,6 +479,8 @@ func (cfg *Config) CreateEngines(ctx context.Context) (Engines, error) {
 
 	skipSizeCheck := cfg.TestingKnobs.Store != nil &&
 		cfg.TestingKnobs.Store.(*kvserver.StoreTestingKnobs).SkipMinSizeCheck
+	disableSeparatedIntents := cfg.TestingKnobs.Store != nil &&
+		cfg.TestingKnobs.Store.(*kvserver.StoreTestingKnobs).StorageKnobs.DisableSeparatedIntents
 	for i, spec := range cfg.Stores.Specs {
 		log.Eventf(ctx, "initializing %+v", spec)
 		var sizeInBytes = spec.Size.InBytes
@@ -542,12 +544,13 @@ func (cfg *Config) CreateEngines(ctx context.Context) (Engines, error) {
 				i, humanizeutil.IBytes(sizeInBytes), openFileLimitPerStore))
 
 			storageConfig := base.StorageConfig{
-				Attrs:             spec.Attributes,
-				Dir:               spec.Path,
-				MaxSize:           sizeInBytes,
-				Settings:          cfg.Settings,
-				UseFileRegistry:   spec.UseFileRegistry,
-				EncryptionOptions: spec.EncryptionOptions,
+				Attrs:                   spec.Attributes,
+				Dir:                     spec.Path,
+				MaxSize:                 sizeInBytes,
+				Settings:                cfg.Settings,
+				UseFileRegistry:         spec.UseFileRegistry,
+				DisableSeparatedIntents: disableSeparatedIntents,
+				EncryptionOptions:       spec.EncryptionOptions,
 			}
 			pebbleConfig := storage.PebbleConfig{
 				StorageConfig: storageConfig,

pkg/server/settings_cache_test.go

@@ -44,7 +44,7 @@ func TestCachedSettingsStoreAndLoad(t *testing.T) {
 	ctx := context.Background()
 	engine, err := storage.Open(ctx, storage.InMemory(),
 		storage.MaxSize(512<<20 /* 512 MiB */),
-		storage.SettingsForTesting())
+		storage.ForTesting)
 	require.NoError(t, err)
 	defer engine.Close()
 

pkg/server/settingswatcher/BUILD.bazel

@@ -50,7 +50,6 @@ go_test(
         "//pkg/settings",
         "//pkg/settings/cluster",
         "//pkg/sql",
-        "//pkg/storage",
         "//pkg/testutils",
         "//pkg/testutils/serverutils",
         "//pkg/testutils/sqlutils",

pkg/server/settingswatcher/settings_watcher_external_test.go

@@ -21,7 +21,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql"
-	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
@@ -93,9 +92,6 @@ func TestSettingWatcher(t *testing.T) {
 		s0.ExecutorConfig().(sql.ExecutorConfig).RangeFeedFactory,
 		tc.Stopper())
 	require.NoError(t, sw.Start(ctx))
-	// TestCluster randomizes the value of SeparatedIntentsEnabled, so set it to
-	// the same as in fakeSettings for the subsequent equality check.
-	storage.SeparatedIntentsEnabled.Override(ctx, &s0.ClusterSettings().SV, storage.SeparatedIntentsEnabled.Get(&fakeSettings.SV))
 	require.NoError(t, checkSettingsValuesMatch(s0.ClusterSettings(), fakeSettings))
 	for k, v := range toSet {
 		tdb.Exec(t, "SET CLUSTER SETTING "+k+" = $1", v[1])

pkg/server/sticky_engine.go

@@ -112,7 +112,7 @@ func (registry *stickyInMemEnginesRegistryImpl) GetOrCreateStickyInMemEngine(
 		storage.Attributes(spec.Attributes),
 		storage.CacheSize(cfg.CacheSize),
 		storage.MaxSize(spec.Size.InBytes),
-		storage.SettingsForTesting())
+		storage.ForTesting)
 
 	engineEntry := &stickyInMemEngine{
 		id:     spec.StickyInMemoryEngineID,

pkg/server/testserver.go

@@ -15,7 +15,6 @@ import (
 	"context"
 	"encoding/base64"
 	"fmt"
-	"math/rand"
 	"net/http"
 	"net/http/cookiejar"
 	"net/url"
@@ -56,7 +55,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/ts"
 	"github.com/cockroachdb/cockroach/pkg/util"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
-	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/metric"
 	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/cockroach/pkg/util/retry"
@@ -129,10 +127,6 @@ func makeTestConfigFromParams(params base.TestServerArgs) Config {
 	st := params.Settings
 	if params.Settings == nil {
 		st = cluster.MakeClusterSettings()
-		enabledSeparated := rand.Intn(2) == 0
-		log.Infof(context.Background(),
-			"test Config is randomly setting enabledSeparated: %t", enabledSeparated)
-		storage.SeparatedIntentsEnabled.Override(context.Background(), &st.SV, enabledSeparated)
 	}
 	st.ExternalIODir = params.ExternalIODir
 	cfg := makeTestConfig(st)

pkg/storage/BUILD.bazel

@@ -35,12 +35,12 @@ go_library(
         "sst_writer.go",
         "temp_dir.go",
         "temp_engine.go",
+        "testing_knobs.go",
     ],
     importpath = "github.com/cockroachdb/cockroach/pkg/storage",
     visibility = ["//visibility:public"],
     deps = [
         "//pkg/base",
-        "//pkg/clusterversion",
         "//pkg/keys",
         "//pkg/kv/kvserver/concurrency/lock",
         "//pkg/kv/kvserver/diskmap",

pkg/storage/bench_pebble_test.go

@@ -17,7 +17,6 @@ import (
 	"testing"
 
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
-	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/util/encoding"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
@@ -31,25 +30,22 @@ func setupMVCCPebble(b testing.TB, dir string) Engine {
 	peb, err := Open(
 		context.Background(),
 		Filesystem(dir),
-		CacheSize(testCacheSize),
-		Settings(makeSettingsForSeparatedIntents(
-			false /* oldClusterVersion */, true /* enabled */)))
+		CacheSize(testCacheSize))
 	if err != nil {
 		b.Fatalf("could not create new pebble instance at %s: %+v", dir, err)
 	}
 	return peb
 }
 
 func setupMVCCInMemPebble(b testing.TB, loc string) Engine {
-	return setupMVCCInMemPebbleWithSettings(b, makeSettingsForSeparatedIntents(
-		false /* oldClusterVersion */, true /* enabled */))
+	return setupMVCCInMemPebbleWithSeparatedIntents(b, false /* disabledSeparatedIntents */)
 }
 
-func setupMVCCInMemPebbleWithSettings(b testing.TB, settings *cluster.Settings) Engine {
+func setupMVCCInMemPebbleWithSeparatedIntents(b testing.TB, disableSeparatedIntents bool) Engine {
 	peb, err := Open(
 		context.Background(),
 		InMemory(),
-		Settings(settings),
+		SetSeparatedIntents(disableSeparatedIntents),
 		CacheSize(testCacheSize))
 	if err != nil {
 		b.Fatalf("could not create new in-mem pebble instance: %+v", err)

pkg/storage/bench_test.go

@@ -212,8 +212,7 @@ func BenchmarkIntentScan(b *testing.B) {
 				b.Run(fmt.Sprintf("versions=%d", numVersions), func(b *testing.B) {
 					for _, percentFlushed := range []int{0, 50, 80, 90, 100} {
 						b.Run(fmt.Sprintf("percent-flushed=%d", percentFlushed), func(b *testing.B) {
-							eng := setupMVCCInMemPebbleWithSettings(
-								b, makeSettingsForSeparatedIntents(false, sep))
+							eng := setupMVCCInMemPebbleWithSeparatedIntents(b, sep)
 							numFlushedVersions := (percentFlushed * numVersions) / 100
 							setupKeysWithIntent(b, eng, numVersions, numFlushedVersions, false /* resolveAll */)
 							lower := makeKey(nil, 0)
@@ -276,8 +275,7 @@ func BenchmarkScanAllIntentsResolved(b *testing.B) {
 				b.Run(fmt.Sprintf("versions=%d", numVersions), func(b *testing.B) {
 					for _, percentFlushed := range []int{0, 50, 90, 100} {
 						b.Run(fmt.Sprintf("percent-flushed=%d", percentFlushed), func(b *testing.B) {
-							eng := setupMVCCInMemPebbleWithSettings(
-								b, makeSettingsForSeparatedIntents(false, sep))
+							eng := setupMVCCInMemPebbleWithSeparatedIntents(b, sep)
 							numFlushedVersions := (percentFlushed * numVersions) / 100
 							setupKeysWithIntent(b, eng, numVersions, numFlushedVersions, true /* resolveAll */)
 							lower := makeKey(nil, 0)
@@ -347,8 +345,7 @@ func BenchmarkScanOneAllIntentsResolved(b *testing.B) {
 				b.Run(fmt.Sprintf("versions=%d", numVersions), func(b *testing.B) {
 					for _, percentFlushed := range []int{0, 50, 90, 100} {
 						b.Run(fmt.Sprintf("percent-flushed=%d", percentFlushed), func(b *testing.B) {
-							eng := setupMVCCInMemPebbleWithSettings(
-								b, makeSettingsForSeparatedIntents(false, sep))
+							eng := setupMVCCInMemPebbleWithSeparatedIntents(b, sep)
 							numFlushedVersions := (percentFlushed * numVersions) / 100
 							setupKeysWithIntent(b, eng, numVersions, numFlushedVersions, true /* resolveAll */)
 							lower := makeKey(nil, 0)
@@ -399,8 +396,7 @@ func BenchmarkIntentResolution(b *testing.B) {
 				b.Run(fmt.Sprintf("versions=%d", numVersions), func(b *testing.B) {
 					for _, percentFlushed := range []int{0, 50, 80, 90, 100} {
 						b.Run(fmt.Sprintf("percent-flushed=%d", percentFlushed), func(b *testing.B) {
-							eng := setupMVCCInMemPebbleWithSettings(
-								b, makeSettingsForSeparatedIntents(false, sep))
+							eng := setupMVCCInMemPebbleWithSeparatedIntents(b, sep)
 							numFlushedVersions := (percentFlushed * numVersions) / 100
 							lockUpdate := setupKeysWithIntent(b, eng, numVersions, numFlushedVersions, false /* resolveAll */)
 							keys := make([]roachpb.Key, numIntentKeys)
@@ -443,8 +439,7 @@ func BenchmarkIntentRangeResolution(b *testing.B) {
 				b.Run(fmt.Sprintf("versions=%d", numVersions), func(b *testing.B) {
 					for _, percentFlushed := range []int{0, 50, 80, 90, 100} {
 						b.Run(fmt.Sprintf("percent-flushed=%d", percentFlushed), func(b *testing.B) {
-							eng := setupMVCCInMemPebbleWithSettings(
-								b, makeSettingsForSeparatedIntents(false, sep))
+							eng := setupMVCCInMemPebbleWithSeparatedIntents(b, sep)
 							numFlushedVersions := (percentFlushed * numVersions) / 100
 							lockUpdate := setupKeysWithIntent(b, eng, numVersions, numFlushedVersions, false /* resolveAll */)
 							keys := make([]roachpb.Key, numIntentKeys+1)

pkg/storage/engine.go

@@ -654,11 +654,6 @@ type Writer interface {
 	//
 	// It is safe to modify the contents of the arguments after Put returns.
 	PutEngineKey(key EngineKey, value []byte) error
-	// SafeToWriteSeparatedIntents is only for internal use in the storage
-	// package. Returns an error if the callee does not know whether it is safe.
-	// This method is temporary, to handle the transition from clusters where
-	// not all nodes understand separated intents.
-	SafeToWriteSeparatedIntents(ctx context.Context) (bool, error)
 
 	// LogData adds the specified data to the RocksDB WAL. The data is
 	// uninterpreted by RocksDB (i.e. not added to the memtable or sstables).

pkg/storage/engine_test.go

@@ -1639,7 +1639,7 @@ func TestScanSeparatedIntents(t *testing.T) {
 	for name, enableSeparatedIntents := range map[string]bool{"interleaved": false, "separated": true} {
 		t.Run(name, func(t *testing.T) {
 			eng, err := Open(ctx, InMemory(), CacheSize(1<<20 /* 1 MiB */),
-				Settings(makeSettingsForSeparatedIntents(false, enableSeparatedIntents)))
+				SetSeparatedIntents(!enableSeparatedIntents))
 			require.NoError(t, err)
 			defer eng.Close()