[DNM] force enabled to check timing of LogicTest

storage,server: enable separated intents

Separated intents are enabled by default, and the setting
is randomized on the testing path for testserver.go.

The intent resolution performance, which caused these
to be disabled, has been improved due to
cockroachdb#60622 and
cockroachdb#60698

TestLogic/aggregate is tweaked to reduce the number of
intents created, as explained in the TODO added where
the change was made.

Informs cockroachdb#41720

Release note (ops change): The default value of the
storage.transaction.separated_intents.enabled cluster
setting is changed to true.

pkg/server/config.go

@@ -484,6 +484,7 @@ func (cfg *Config) CreateEngines(ctx context.Context) (Engines, error) {
 		log.Eventf(ctx, "initializing %+v", spec)
 		var sizeInBytes = spec.Size.InBytes
 		if spec.InMemory {
+			log.Infof(ctx, "cache sizeInBytes: %d, percent: %f", sizeInBytes, spec.Size.Percent)
 			if spec.Size.Percent > 0 {
 				sysMem, err := status.GetTotalMemory(ctx)
 				if err != nil {
@@ -516,6 +517,8 @@ func (cfg *Config) CreateEngines(ctx context.Context) (Engines, error) {
 				engines = append(engines, e)
 			} else {
 				engines = append(engines, storage.NewInMem(ctx, spec.Attributes, sizeInBytes, cfg.Settings))
+				// engines = append(engines,
+				//	storage.NewInMemWithCache(ctx, spec.Attributes, pebbleCache, cfg.Settings))
 			}
 		} else {
 			if spec.Size.Percent > 0 {
@@ -557,6 +560,7 @@ func (cfg *Config) CreateEngines(ctx context.Context) (Engines, error) {
 			if len(spec.RocksDBOptions) > 0 {
 				return nil, errors.Errorf("store %d: using Pebble storage engine but StoreSpec provides RocksDB options", i)
 			}
+			log.Infof(ctx, "calling NewPebble with cache")
 			eng, err := storage.NewPebble(ctx, pebbleConfig)
 			if err != nil {
 				return Engines{}, err

pkg/server/settingswatcher/BUILD.bazel

@@ -48,6 +48,7 @@ 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,6 +21,7 @@ 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"
@@ -92,6 +93,10 @@ 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(
+		&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/testserver.go

@@ -132,12 +132,10 @@ func makeTestConfigFromParams(params base.TestServerArgs) Config {
 	st := params.Settings
 	if params.Settings == nil {
 		st = cluster.MakeClusterSettings()
-		// TODO(sumeer): re-introduce this randomization.
-		// enabledSeparated := rand.Intn(2) == 0
-		// log.Infof(context.Background(),
-		//	"test Config is randomly setting enabledSeparated: %t",
-		//	enabledSeparated)
-		// storage.SeparatedIntentsEnabled.Override(&st.SV, enabledSeparated)
+		enabledSeparated := true
+		log.Infof(context.Background(),
+			"test Config is randomly setting enabledSeparated: %t", enabledSeparated)
+		storage.SeparatedIntentsEnabled.Override(&st.SV, enabledSeparated)
 	}
 	st.ExternalIODir = params.ExternalIODir
 	cfg := makeTestConfig(st)

pkg/sql/logictest/testdata/logic_test/aggregate

@@ -1,5 +1,11 @@
 subtest other
 
+# Reduce the need for ranged intent resolution, since the test environment
+# tends to accumulate lots of un-compacted intent tombstones that can slow it
+# down.
+statement ok
+SET CLUSTER SETTING kv.transaction.max_intents_bytes = '1048576'
+
 statement ok
 CREATE TABLE kv (
   k INT PRIMARY KEY,
@@ -892,32 +898,38 @@ CREATE TABLE mnop (
   p BIGINT
 )
 
+# TODO(sumeer): increase this series back to 2e4 once we have more performance
+# improvements to ranged intent resolution. This test is somewhat artificial
+# in that it accumulates a large number of deletion markers for intents for
+# each key, in the LSM memtable, without them being flushed to ssts. This
+# slows down ranged intent resolution. With 1e4 keys, the txn tracks each
+# intent individually, and intent resolution is fast.
 statement ok
 INSERT INTO mnop (m, n) SELECT i, (1e9 + i/2e4)::float FROM
-  generate_series(1, 2e4) AS i(i)
+  generate_series(1, 5e3) AS i(i)
 
 statement ok
 UPDATE mnop SET o = n::decimal, p = (n * 10)::bigint
 
 query RRR
 SELECT round(variance(n), 2), round(variance(n), 2), round(variance(p)) FROM mnop
 ----
-0.08 0.08 8
+0.01  0.01  1
 
 query RRR
 SELECT round(var_pop(n), 2), round(var_pop(n), 2), round(var_pop(p)) FROM mnop
 ----
-0.08 0.08 8
+0.01  0.01  1
 
 query RRR
 SELECT round(stddev_samp(n), 2), round(stddev(n), 2), round(stddev_samp(p)) FROM mnop
 ----
-0.29 0.29 3
+0.07  0.07  1
 
 query RRR
 SELECT round(stddev_pop(n), 2), round(stddev_pop(n), 2), round(stddev_pop(p)) FROM mnop
 ----
-0.29 0.29 3
+0.07  0.07  1
 
 query RRR
 SELECT avg(1::int)::float, avg(2::float)::float, avg(3::decimal)::float

pkg/sql/logictest/testdata/logic_test/fk

@@ -1,5 +1,11 @@
 # LogicTest: !3node-tenant(49854)
 
+# Reduce the need for ranged intent resolution, since the test environment
+# tends to accumulate lots of un-compacted intent tombstones that can slow it
+# down.
+statement ok
+SET CLUSTER SETTING kv.transaction.max_intents_bytes = "1048576"
+
 statement ok
 SET CLUSTER SETTING sql.cross_db_fks.enabled = TRUE
 

pkg/sql/logictest/testdata/logic_test/tenant

@@ -1,4 +1,5 @@
 # LogicTest: !3node-tenant
+
 query IBT colnames
 SELECT * FROM system.tenants ORDER BY id
 ----

pkg/sql/logictest/testdata/logic_test/upsert

@@ -1,5 +1,11 @@
 subtest strict
 
+# Reduce the need for ranged intent resolution, since the test environment
+# tends to accumulate lots of un-compacted intent tombstones that can slow it
+# down.
+statement ok
+SET CLUSTER SETTING kv.transaction.max_intents_bytes = "1048576"
+
 statement ok
 CREATE TABLE ex(
   foo INT PRIMARY KEY,

pkg/storage/bench_test.go

@@ -141,7 +141,7 @@ func BenchmarkExportToSst(b *testing.B) {
 const numIntentKeys = 1000
 
 func setupKeysWithIntent(
-	b *testing.B, eng Engine, numVersions int, numFlushedVersions int,
+	b *testing.B, eng Engine, numVersions int, numFlushedVersions int, resolveAll bool,
 ) roachpb.LockUpdate {
 	txnIDCount := 2 * numVersions
 	val := []byte("value")
@@ -179,7 +179,7 @@ func setupKeysWithIntent(
 			Txn:    txn.TxnMeta,
 			Status: roachpb.COMMITTED,
 		}
-		if i < numVersions {
+		if i < numVersions || resolveAll {
 			batch := eng.NewBatch()
 			for j := 0; j < numIntentKeys; j++ {
 				key := makeKey(nil, j)
@@ -209,7 +209,7 @@ func BenchmarkIntentScan(b *testing.B) {
 							eng := setupMVCCInMemPebbleWithSettings(
 								b, makeSettingsForSeparatedIntents(false, sep))
 							numFlushedVersions := (percentFlushed * numVersions) / 100
-							setupKeysWithIntent(b, eng, numVersions, numFlushedVersions)
+							setupKeysWithIntent(b, eng, numVersions, numFlushedVersions, false)
 							lower := makeKey(nil, 0)
 							iter := eng.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{
 								LowerBound: lower,
@@ -258,6 +258,101 @@ func BenchmarkIntentScan(b *testing.B) {
 	}
 }
 
+func BenchmarkScanAllIntentDeleted(b *testing.B) {
+	skip.UnderShort(b, "setting up unflushed data takes too long")
+	for _, sep := range []bool{false, true} {
+		b.Run(fmt.Sprintf("separated=%t", sep), func(b *testing.B) {
+			for _, numVersions := range []int{200} {
+				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))
+							numFlushedVersions := (percentFlushed * numVersions) / 100
+							setupKeysWithIntent(b, eng, numVersions, numFlushedVersions, true)
+							lower := makeKey(nil, 0)
+							iter := eng.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{
+								LowerBound: lower,
+								UpperBound: makeKey(nil, numIntentKeys),
+							})
+							iter.SeekGE(MVCCKey{Key: lower})
+							var buf []byte
+							b.ResetTimer()
+							for i := 0; i < b.N; i++ {
+								valid, err := iter.Valid()
+								if err != nil {
+									b.Fatal(err)
+								}
+								if !valid {
+									iter.SeekGE(MVCCKey{Key: lower})
+								} else {
+									// Read latest version.
+									k := iter.UnsafeKey()
+									if !k.IsValue() {
+										b.Fatalf("expected value %s", k.String())
+									}
+									// Skip to next key. This will do one Next and then Seek.
+									// iter.NextKey()
+									buf = append(buf[:0], k.Key...)
+									buf = roachpb.BytesNext(buf)
+									iter.SeekGE(MVCCKey{Key: buf})
+								}
+							}
+						})
+					}
+				})
+			}
+		})
+	}
+}
+
+func BenchmarkNextPrevAllIntentDeleted(b *testing.B) {
+	skip.UnderShort(b, "setting up unflushed data takes too long")
+	for _, sep := range []bool{false, true} {
+		b.Run(fmt.Sprintf("separated=%t", sep), func(b *testing.B) {
+			for _, numVersions := range []int{200} {
+				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))
+							numFlushedVersions := (percentFlushed * numVersions) / 100
+							setupKeysWithIntent(b, eng, numVersions, numFlushedVersions, true)
+							lower := makeKey(nil, 0)
+							iter := eng.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{
+								LowerBound: lower,
+								UpperBound: makeKey(nil, numIntentKeys),
+							})
+							iter.SeekGE(MVCCKey{Key: lower})
+							iter.NextKey()
+							iter.NextKey()
+							b.ResetTimer()
+							for i := 0; i < b.N; i++ {
+								iter.Next()
+								valid, err := iter.Valid()
+								if err != nil {
+									b.Fatal(err)
+								}
+								if !valid {
+									b.Fatal("should be valid")
+								}
+								iter.Prev()
+								valid, err = iter.Valid()
+								if err != nil {
+									b.Fatal(err)
+								}
+								if !valid {
+									b.Fatal("should be valid")
+								}
+							}
+						})
+					}
+				})
+			}
+		})
+	}
+}
+
 func BenchmarkIntentResolution(b *testing.B) {
 	skip.UnderShort(b, "setting up unflushed data takes too long")
 	for _, sep := range []bool{false, true} {
@@ -269,7 +364,7 @@ func BenchmarkIntentResolution(b *testing.B) {
 							eng := setupMVCCInMemPebbleWithSettings(
 								b, makeSettingsForSeparatedIntents(false, sep))
 							numFlushedVersions := (percentFlushed * numVersions) / 100
-							lockUpdate := setupKeysWithIntent(b, eng, numVersions, numFlushedVersions)
+							lockUpdate := setupKeysWithIntent(b, eng, numVersions, numFlushedVersions, false)
 							keys := make([]roachpb.Key, numIntentKeys)
 							for i := range keys {
 								keys[i] = makeKey(nil, i)
@@ -309,7 +404,7 @@ func BenchmarkIntentRangeResolution(b *testing.B) {
 							eng := setupMVCCInMemPebbleWithSettings(
 								b, makeSettingsForSeparatedIntents(false, sep))
 							numFlushedVersions := (percentFlushed * numVersions) / 100
-							lockUpdate := setupKeysWithIntent(b, eng, numVersions, numFlushedVersions)
+							lockUpdate := setupKeysWithIntent(b, eng, numVersions, numFlushedVersions, false)
 							keys := make([]roachpb.Key, numIntentKeys+1)
 							for i := range keys {
 								keys[i] = makeKey(nil, i)

pkg/storage/in_mem.go

@@ -14,10 +14,13 @@ import (
 	"context"
 	"math/rand"
 
+	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/pebble"
+	"github.com/cockroachdb/pebble/vfs"
 )
 
 // NewInMem allocates and returns a new, opened in-memory engine. The caller
@@ -30,6 +33,36 @@ func NewInMem(
 	return newPebbleInMem(ctx, attrs, cacheSize, settings)
 }
 
+// NewInMemWithCache allocated and returns a new, opened in-memory engine, and
+// uses the provided cache. The caller must call the engine's Close method
+// when the engine is no longer needed.
+func NewInMemWithCache(
+	ctx context.Context, attrs roachpb.Attributes, cache *pebble.Cache, settings *cluster.Settings,
+) Engine {
+	log.Infof(ctx, "NewInMemWithCache")
+	opts := DefaultPebbleOptions()
+	opts.Cache = cache
+
+	opts.FS = vfs.NewMem()
+	db, err := NewPebble(
+		ctx,
+		PebbleConfig{
+			StorageConfig: base.StorageConfig{
+				Attrs: attrs,
+				// TODO(bdarnell): The hard-coded 512 MiB is wrong; see
+				// https://github.com/cockroachdb/cockroach/issues/16750
+				MaxSize:  512 << 20, /* 512 MiB */
+				Settings: settings,
+			},
+			Opts: opts,
+		})
+	if err != nil {
+		panic(err)
+	}
+	return db
+
+}
+
 // The ForTesting functions randomize the settings for separated intents. This
 // is a bit peculiar for tests outside the storage package, since they usually
 // have higher level cluster constructs, including creating a cluster.Settings

pkg/storage/intent_reader_writer.go

@@ -46,7 +46,7 @@ var SeparatedIntentsEnabled = settings.RegisterBoolSetting(
 	"storage.transaction.separated_intents.enabled",
 	"if enabled, intents will be written to a separate lock table, instead of being "+
 		"interleaved with MVCC values",
-	false,
+	true,
 )
 
 // This file defines wrappers for Reader and Writer, and functions to do the

pkg/storage/pebble.go

@@ -565,6 +565,7 @@ func NewPebble(ctx context.Context, cfg PebbleConfig) (*Pebble, error) {
 func newPebbleInMem(
 	ctx context.Context, attrs roachpb.Attributes, cacheSize int64, settings *cluster.Settings,
 ) *Pebble {
+	log.Infof(ctx, "newPebbleInMem: cacheSize %d", cacheSize)
 	opts := DefaultPebbleOptions()
 	opts.Cache = pebble.NewCache(cacheSize)
 	defer opts.Cache.Unref()