Merge cockroachdb#153738 cockroachdb#154002 cockroachdb#154038

153738: storage: raise MemTableStopWritesThreshold when memory permits r=sumeerbhola a=jbowens

When there's sufficient memory available, raise the MemTableStopWritesThreshold up to as high as 16. We've observed instances of memtable write stalls in practice, especially in the presence of high latencies in cloud networked block devices. If the higher levels are not writing to the storage engine too fast, but flushes are unable to keep up because of disk slowness, we'd rather accumulate additional memtables that forcibly stall writes.

Epic: none
Fixes: cockroachdb#153673.
Release note: None

154002: execbuilder: only run geospatial in local config r=yuzefovich a=yuzefovich

We generally run execbuilder tests in local or 5node configs since the tests are about planning and tracing of expected KV requests, so there is no benefit to running the tests with more diverse configs. Due to some historical reasons, `geospatial` test file has been running in almost all configs, but I don't see a reason why that should be the case. This commit updates the test to run only in `local` config.

Epic: None
Release note: None

154038: workflows: run `update_releases` on `release-25.4` r=dodeca12 a=dodeca12

Epic: None
Release note: None

Co-authored-by: Jackson Owens <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Swapneeth Gorantla <[email protected]>

Author: 4 people

.github/workflows/update_releases.yaml

@@ -32,6 +32,7 @@ jobs:
           - "release-24.3"
           - "release-25.2"
           - "release-25.3"
+          - "release-25.4"
     name: Update pkg/testutils/release/cockroach_releases.yaml on ${{ matrix.branch }}
     runs-on: ubuntu-latest
     steps:

pkg/BUILD.bazel

@@ -538,14 +538,6 @@ ALL_TESTS = [
     "//pkg/sql/opt/cycle:cycle_test",
     "//pkg/sql/opt/distribution:distribution_test",
     "//pkg/sql/opt/exec/execbuilder/tests/5node:5node_test",
-    "//pkg/sql/opt/exec/execbuilder/tests/fakedist-disk:fakedist-disk_test",
-    "//pkg/sql/opt/exec/execbuilder/tests/fakedist-vec-off:fakedist-vec-off_test",
-    "//pkg/sql/opt/exec/execbuilder/tests/fakedist:fakedist_test",
-    "//pkg/sql/opt/exec/execbuilder/tests/local-legacy-schema-changer:local-legacy-schema-changer_test",
-    "//pkg/sql/opt/exec/execbuilder/tests/local-mixed-25.2:local-mixed-25_2_test",
-    "//pkg/sql/opt/exec/execbuilder/tests/local-mixed-25.3:local-mixed-25_3_test",
-    "//pkg/sql/opt/exec/execbuilder/tests/local-prepared:local-prepared_test",
-    "//pkg/sql/opt/exec/execbuilder/tests/local-vec-off:local-vec-off_test",
     "//pkg/sql/opt/exec/execbuilder/tests/local:local_test",
     "//pkg/sql/opt/exec/execbuilder:execbuilder_test",
     "//pkg/sql/opt/exec/explain:explain_test",
@@ -2120,14 +2112,6 @@ GO_TARGETS = [
     "//pkg/sql/opt/distribution:distribution",
     "//pkg/sql/opt/distribution:distribution_test",
     "//pkg/sql/opt/exec/execbuilder/tests/5node:5node_test",
-    "//pkg/sql/opt/exec/execbuilder/tests/fakedist-disk:fakedist-disk_test",
-    "//pkg/sql/opt/exec/execbuilder/tests/fakedist-vec-off:fakedist-vec-off_test",
-    "//pkg/sql/opt/exec/execbuilder/tests/fakedist:fakedist_test",
-    "//pkg/sql/opt/exec/execbuilder/tests/local-legacy-schema-changer:local-legacy-schema-changer_test",
-    "//pkg/sql/opt/exec/execbuilder/tests/local-mixed-25.2:local-mixed-25_2_test",
-    "//pkg/sql/opt/exec/execbuilder/tests/local-mixed-25.3:local-mixed-25_3_test",
-    "//pkg/sql/opt/exec/execbuilder/tests/local-prepared:local-prepared_test",
-    "//pkg/sql/opt/exec/execbuilder/tests/local-vec-off:local-vec-off_test",
     "//pkg/sql/opt/exec/execbuilder/tests/local:local_test",
     "//pkg/sql/opt/exec/execbuilder:execbuilder",
     "//pkg/sql/opt/exec/execbuilder:execbuilder_test",

pkg/ccl/logictestccl/tests/local-read-committed/generated_test.go

@@ -8,7 +8,6 @@ go_test(
         "//c-deps:libgeos",  # keep
         "//pkg/ccl/logictestccl:testdata",  # keep
         "//pkg/sql/logictest:testdata",  # keep
-        "//pkg/sql/opt/exec/execbuilder:testdata",  # keep
     ],
     exec_properties = {"test.Pool": "large"},
     shard_count = 48,
@@ -20,7 +19,6 @@ go_test(
         "//pkg/security/securityassets",
         "//pkg/security/securitytest",
         "//pkg/server",
-        "//pkg/sql",
         "//pkg/sql/logictest",
         "//pkg/testutils/serverutils",
         "//pkg/testutils/skip",

pkg/ccl/logictestccl/tests/local-repeatable-read/BUILD.bazel

@@ -613,13 +613,15 @@ go_test(
         "//pkg/util/tracing",
         "//pkg/util/tracing/tracingpb",
         "//pkg/util/uuid",
+        "@com_github_cockroachdb_crlib//crstrings",
         "@com_github_cockroachdb_datadriven//:datadriven",
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_cockroachdb_logtags//:logtags",
         "@com_github_cockroachdb_pebble//:pebble",
         "@com_github_cockroachdb_pebble//vfs",
         "@com_github_cockroachdb_redact//:redact",
         "@com_github_dustin_go_humanize//:go-humanize",
+        "@com_github_ghemawat_stream//:stream",
         "@com_github_gogo_protobuf//jsonpb",
         "@com_github_gogo_protobuf//proto",
         "@com_github_gorilla_mux//:mux",

pkg/ccl/logictestccl/tests/local-repeatable-read/generated_test.go

@@ -739,9 +739,26 @@ func (cfg *Config) CreateEngines(ctx context.Context) (Engines, error) {
 	if err != nil {
 		return Engines{}, err
 	}
-
 	log.Event(ctx, "initializing engines")
 
+	// The (pebble.Options).MemTableStopWritesThreshold configures the number of
+	// memtables that may be queued before Pebble induces a write stall.
+	// Queueing memtables consume memory from the block cache, evicting resident
+	// blocks. If flushes are not keeping up and the count of queued memtables
+	// grows too large, read performance will degrade severely:
+	//
+	// - Every read needs to seek in every queued memtable.
+	// - Memtables take memory from the block cache, meaning that block
+	//   cache effectiveness decreases the more memtables that are queued.
+	//
+	// We constrain the count of queued memtables to be between 4 and 16. Within
+	// those bounds, we'll grow it to use up to half of the block cache. If
+	// there are multiple stores, we need to divide that half by the count of
+	// stores.
+	stopWritesThreshold := int(cfg.CacheSize/2/storage.DefaultMemtableSize) / len(cfg.Stores.Specs)
+	stopWritesThreshold = max(stopWritesThreshold, 4)
+	stopWritesThreshold = min(stopWritesThreshold, 16)
+
 	var fileCache *pebble.FileCache
 	// TODO(radu): use the fileCache for in-memory stores as well.
 	if physicalStores > 0 {
@@ -779,6 +796,7 @@ func (cfg *Config) CreateEngines(ctx context.Context) (Engines, error) {
 			storage.Attributes(roachpb.Attributes{Attrs: spec.Attributes}),
 			storage.If(storeKnobs.SmallEngineBlocks, storage.BlockSize(1)),
 			storage.BlockConcurrencyLimitDivisor(len(cfg.Stores.Specs)),
+			storage.MemTableStopWritesThreshold(stopWritesThreshold),
 		}
 		if len(storeKnobs.EngineKnobs) > 0 {
 			storageConfigOpts = append(storageConfigOpts, storeKnobs.EngineKnobs...)

pkg/server/BUILD.bazel

@@ -6,10 +6,14 @@
 package server
 
 import (
+	"bytes"
 	"context"
+	"fmt"
+	"io"
 	"net"
 	"os"
 	"reflect"
+	"strings"
 	"sync/atomic"
 	"testing"
 	"time"
@@ -27,7 +31,10 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/netutil"
 	"github.com/cockroachdb/cockroach/pkg/util/netutil/addr"
+	"github.com/cockroachdb/crlib/crstrings"
+	"github.com/cockroachdb/datadriven"
 	"github.com/cockroachdb/pebble/vfs"
+	"github.com/ghemawat/stream"
 	"github.com/kr/pretty"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -368,3 +375,76 @@ func TestIdProviderServerIdentityString(t *testing.T) {
 		})
 	}
 }
+
+func TestCreateEngines(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	specs := map[string]base.StoreSpec{}
+
+	datadriven.RunTest(t, "testdata/create_engines", func(t *testing.T, d *datadriven.TestData) string {
+		switch d.Cmd {
+		case "store-spec":
+			var name string
+			d.ScanArgs(t, "name", &name)
+			spec := base.StoreSpec{}
+			spec.Path = name
+			for _, line := range crstrings.Lines(d.Input) {
+				parts := strings.SplitN(line, "=", 2)
+				switch parts[0] {
+				case "in-memory":
+					spec.InMemory = true
+				case "attrs":
+					spec.Attributes = strings.Split(parts[1], ":")
+				default:
+					return fmt.Sprintf("unknown field: %q", parts[0])
+				}
+			}
+			specs[name] = spec
+			return ""
+		case "create-engines":
+			var cfg Config
+			cfg.Settings = cluster.MakeTestingClusterSettings()
+
+			var pattern string
+			d.ScanArgs(t, "pattern", &pattern)
+			var specNames []string
+			d.ScanArgs(t, "specs", &specNames)
+			for _, specName := range specNames {
+				spec, ok := specs[specName]
+				if !ok {
+					return fmt.Sprintf("unknown store: %q", specName)
+				}
+				cfg.Stores.Specs = append(cfg.Stores.Specs, spec)
+			}
+			d.MaybeScanArgs(t, "CacheSize", &cfg.CacheSize)
+
+			engines, err := cfg.CreateEngines(context.Background())
+			if err != nil {
+				return fmt.Sprintf("failed to create engines: %v", err)
+			}
+			defer engines.Close()
+
+			var buf bytes.Buffer
+			for _, e := range engines {
+				buf.WriteString(strings.TrimSpace(e.GetPebbleOptions().String()))
+				buf.WriteString("\n")
+			}
+			return grepStr(&buf, pattern)
+		default:
+			return fmt.Sprintf("unknown command: %q", d.Cmd)
+		}
+	})
+}
+
+func grepStr(r io.Reader, pattern string) string {
+	var buf bytes.Buffer
+	if err := stream.Run(stream.Sequence(
+		stream.ReadLines(r),
+		stream.Grep(pattern),
+		stream.WriteLines(&buf),
+	)); err != nil {
+		return err.Error()
+	}
+	return buf.String()
+}

pkg/server/config.go

@@ -0,0 +1,40 @@
+store-spec name=a
+in-memory
+----
+
+store-spec name=b
+in-memory
+----
+
+# 64 MiB cache; should result in 4 memtable stop writes threshold.
+
+create-engines specs=(a,b) pattern=mem_table_stop_writes_threshold CacheSize=67108864
+----
+  mem_table_stop_writes_threshold=4
+  mem_table_stop_writes_threshold=4
+
+create-engines specs=(a) pattern=mem_table_stop_writes_threshold CacheSize=67108864
+----
+  mem_table_stop_writes_threshold=4
+
+# 1 GiB cache; should result in 8 memtable stop writes threshold with a single
+# store (with 64 MiB memtables).
+
+create-engines specs=(b) pattern=mem_table_stop_writes_threshold CacheSize=1073741824
+----
+  mem_table_stop_writes_threshold=8
+
+# But if we add another store, the memtable stop writes threshold should be
+# halved back to the minimum of 4.
+
+create-engines specs=(a, b) pattern=mem_table_stop_writes_threshold CacheSize=1073741824
+----
+  mem_table_stop_writes_threshold=4
+  mem_table_stop_writes_threshold=4
+
+# 64 GiB cache; should result in 16 memtable stop writes threshold.
+
+create-engines specs=(a,b) pattern=mem_table_stop_writes_threshold CacheSize=68719476736
+----
+  mem_table_stop_writes_threshold=16
+  mem_table_stop_writes_threshold=16

pkg/server/config_test.go

@@ -1,7 +1,4 @@
-# This logic test fails in the 3node-tenant configuration because the keys are
-# prefixed with the tenant ID if run by a tenant:
-# https://github.com/cockroachdb/cockroach/issues/49582
-# LogicTest: !3node-tenant-default-configs
+# LogicTest: local
 
 statement ok
 CREATE TABLE b(