Merge #157784 #157852 #157919 #157920

157784: tree: speed up two tests r=yuzefovich a=yuzefovich

We just saw a package-level test timeout of 1 minute being hit because `TestRandomInjectHints` took 29s and `TestParseArrayRandomParseArray` took 16s before interruption. This commit reduces the number of iterations by 5x and 10x, respectively.

Fixes: #157256.
Release note: None

157852: mixedversion: remove special case for skipping current version r=williamchoe3 a=DarrylWong

In the past, we used to bump CRDB's minSupportedVersion _before_ bumping CRDB's current version.

Consider the old version bump process:
  1. Bump minimum supported version (e.g. 25.2→25.3)
  2. Bump current version (e.g. 25.4→26.1)
  3. Bump minimum supported version again (e.g. 25.3→25.4)

After the first step, we would be in a state where the mixed version framework would see v25.3 as a skippable version (.1 and .3 ordinal versions are skippable), but since the minimum supported version is 25.3, 25.2→25.4 is (temporarily) not a legal upgrade.

This required a special case to disallow skip upgrades in the case of this edge case. However, we now bump the current version _first_. This means we no longer need to protect against the above and we actually hit a different bug.

Since we now bump the current version first, the special case will always view the current version as having multiple supported predecessors and force a skip upgrade even if not applicable.

Release note: none
Epic: none

157919: roachprod: fix service registry unit test name collision r=golgeek a=DarrylWong

We were previously generating cluster names with Uint32, which is not sufficient enough to prevent naming collisions after 1000 iterations. Instead, use a RandString of length 10.

Fixes: #157884
Release note: none

157920: storage,server: add debug separated value retrieval profile endpoint r=RaduBerinde a=jbowens

Add a new debug endpoint that returns a text-formatted representation of all the stack traces that performed a retrieval of a separated value while the profile was being collected. This provides better observability into what code paths are suffering more expensive value retrievals.

Epic: none
Informs: #146575
Release note: none

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

Author: 4 people

pkg/cmd/roachtest/roachtestutil/mixedversion/BUILD.bazel

@@ -14,7 +14,6 @@ go_library(
     importpath = "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/roachtestutil/mixedversion",
     visibility = ["//visibility:public"],
     deps = [
-        "//pkg/clusterversion",
         "//pkg/cmd/roachprod/grafana",
         "//pkg/cmd/roachtest/cluster",
         "//pkg/cmd/roachtest/option",
@@ -39,7 +38,6 @@ go_library(
         "//pkg/util/syncutil",
         "//pkg/util/timeutil",
         "@com_github_cockroachdb_errors//:errors",
-        "@com_github_cockroachdb_version//:version",
         "@org_golang_x_exp//maps",
     ],
 )
@@ -59,7 +57,6 @@ go_test(
     embed = [":mixedversion"],
     embedsrcs = ["testdata/test_releases.yaml"],
     deps = [
-        "//pkg/clusterversion",
         "//pkg/cmd/roachtest/option",
         "//pkg/cmd/roachtest/registry",
         "//pkg/cmd/roachtest/roachtestutil",

pkg/cmd/roachtest/roachtestutil/mixedversion/mixedversion.go

@@ -79,7 +79,6 @@ import (
 	"sync"
 	"time"
 
-	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/cluster"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/option"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry"
@@ -94,7 +93,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/release"
 	"github.com/cockroachdb/cockroach/pkg/util/randutil"
 	"github.com/cockroachdb/errors"
-	"github.com/cockroachdb/version"
 )
 
 const (
@@ -613,14 +611,6 @@ func (t *Test) supportsSkipUpgradeTo(pred, v *clusterupgrade.Version) bool {
 	if t.options.minimumSupportedVersion.Series() == pred.Series() {
 		return false
 	}
-	// Special case for the current release series. This is useful to keep the
-	// test correct when we bump the minimum supported version separately from
-	// the current version.
-	r := clusterversion.Latest.ReleaseSeries()
-	currentMajor := version.MajorVersion{Year: int(r.Major), Ordinal: int(r.Minor)}
-	if currentMajor.Equals(v.Version.Major()) {
-		return len(clusterversion.SupportedPreviousReleases()) > 1
-	}
 
 	series := v.Version.Major()
 	switch {

pkg/cmd/roachtest/roachtestutil/mixedversion/mixedversion_test.go

@@ -11,7 +11,6 @@ import (
 	"math/rand"
 	"testing"
 
-	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/option"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/roachtestutil/clusterupgrade"
@@ -468,13 +467,6 @@ func TestSupportsSkipUpgradeTo(t *testing.T) {
 	expect := func(verStr string, expected bool) {
 		t.Helper()
 		v := clusterupgrade.MustParseVersion(verStr)
-		r := clusterversion.Latest.ReleaseSeries()
-		currentMajor := version.MajorVersion{Year: int(r.Major), Ordinal: int(r.Minor)}
-		if currentMajor.Equals(v.Version.Major()) {
-			// We have to special case the current series, to allow for bumping the
-			// min supported version separately from the current version.
-			expected = len(clusterversion.SupportedPreviousReleases()) > 1
-		}
 		require.Equal(t, expected, mvt.supportsSkipUpgradeTo(prev, v))
 	}
 	for _, v := range []string{"v24.3.0", "v24.3.0-beta.1", "v25.2.1", "v25.2.0-rc.1"} {

pkg/cmd/roachtest/roachtestutil/mixedversion/testdata/planner/separate_process

@@ -44,8 +44,8 @@ Plan:
 ├── delete all-tenants override for the `version` key (9) [stage=system:tenant-setup;tenant:tenant-setup]
 ├── run startup hooks concurrently
 │   ├── run "create tables" hookId="planner_test.go:141", after 3m0s delay (10) [stage=system:on-startup;tenant:on-startup]
-│   └── run "initialize bank workload" hookId="mixedversion.go:884", after 100ms delay (11) [stage=system:on-startup;tenant:on-startup]
-├── run "bank workload" hookId="mixedversion.go:892" (12) [stage=system:background;tenant:background]
+│   └── run "initialize bank workload" hookId="mixedversion.go:874", after 100ms delay (11) [stage=system:on-startup;tenant:on-startup]
+├── run "bank workload" hookId="mixedversion.go:882" (12) [stage=system:background;tenant:background]
 ├── upgrade cluster from "v22.2.3" to "v23.1.4"
 │   ├── upgrade storage cluster
 │   │   ├── prevent auto-upgrades on system tenant by setting `preserve_downgrade_option` (13) [stage=system:init;tenant:upgrading-system]

pkg/cmd/roachtest/roachtestutil/mixedversion/testdata/planner/shared_process

@@ -53,8 +53,8 @@ Plan:
 ├── delete all-tenants override for the `version` key (16) [stage=system:tenant-setup;tenant:tenant-setup]
 ├── run startup hooks concurrently
 │   ├── run "create tables" hookId="planner_test.go:141", after 30s delay (17) [stage=system:on-startup;tenant:on-startup]
-│   └── run "initialize bank workload" hookId="mixedversion.go:884", after 3m0s delay (18) [stage=system:on-startup;tenant:on-startup]
-├── run "bank workload" hookId="mixedversion.go:892" (19) [stage=system:background;tenant:background]
+│   └── run "initialize bank workload" hookId="mixedversion.go:874", after 3m0s delay (18) [stage=system:on-startup;tenant:on-startup]
+├── run "bank workload" hookId="mixedversion.go:882" (19) [stage=system:background;tenant:background]
 ├── upgrade cluster from "v23.1.12" to "v23.2.0"
 │   ├── prevent auto-upgrades on system tenant by setting `preserve_downgrade_option` (20) [stage=system:init;tenant:init]
 │   ├── prevent auto-upgrades on mixed-version-tenant-cyvju tenant by setting `preserve_downgrade_option` (21) [stage=system:init;tenant:init]

pkg/cmd/roachtest/roachtestutil/mixedversion/testdata/planner/step_stages

@@ -35,10 +35,10 @@ Plan:
 ├── install fixtures for version "v22.2.3" (1) [stage=system-setup]
 ├── start cluster at version "v22.2.3" (2) [stage=system-setup]
 ├── wait for all nodes (:1-4) to acknowledge cluster version '22.2' on system tenant (3) [stage=system-setup]
-├── run "initialize bank workload" hookId="mixedversion.go:884" (4) [stage=on-startup]
+├── run "initialize bank workload" hookId="mixedversion.go:874" (4) [stage=on-startup]
 ├── start background hooks concurrently
-│   ├── run "bank workload" hookId="mixedversion.go:892", after 0s delay (5) [stage=background]
-│   └── run "csv server" hookId="mixedversion.go:861", after 0s delay (6) [stage=background]
+│   ├── run "bank workload" hookId="mixedversion.go:882", after 0s delay (5) [stage=background]
+│   └── run "csv server" hookId="mixedversion.go:851", after 0s delay (6) [stage=background]
 ├── upgrade cluster from "v22.2.3" to "v23.1.4"
 │   ├── prevent auto-upgrades on system tenant by setting `preserve_downgrade_option` (7) [stage=init]
 │   ├── upgrade nodes :1-4 from "v22.2.3" to "v23.1.4"

pkg/roachprod/install/services_test.go

@@ -138,7 +138,7 @@ func (t *serviceRegistryTest) makeVM(clusterName string, i int) vm.VM {
 }
 
 func (t *serviceRegistryTest) newCluster(nodeCount int) *SyncedCluster {
-	clusterName := fmt.Sprintf("cluster-%d", t.rng.Uint32())
+	clusterName := fmt.Sprintf("cluster-%s", randutil.RandString(t.rng, 10, randutil.PrintableKeyAlphabet))
 	c := &SyncedCluster{
 		Cluster: cloud.Cluster{
 			Name: clusterName,

pkg/server/debug/server.go

@@ -6,13 +6,15 @@
 package debug
 
 import (
+	"context"
 	"expvar"
 	"fmt"
 	"io"
 	"net/http"
 	"net/http/pprof"
 	"strconv"
 	"strings"
+	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/base/serverident"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver"
@@ -277,6 +279,29 @@ func (ds *Server) RegisterEngines(engines []storage.Engine) error {
 					fmt.Fprintf(w, "error analyzing LSM at %s: %v", dir, err)
 				}
 			})
+		ds.mux.HandleFunc(fmt.Sprintf("/debug/storage/%d/profiles/separated-value-retrievals", storeID),
+			func(w http.ResponseWriter, req *http.Request) {
+				dur := 30 * time.Second
+				if secsStr := req.Header.Get("seconds"); secsStr != "" {
+					secs, err := strconv.ParseInt(secsStr, 10, 64)
+					if err != nil {
+						http.Error(w, "error parsing seconds", http.StatusBadRequest)
+						return
+					}
+					dur = time.Duration(secs) * time.Second
+				}
+				ctx := req.Context()
+				ctx, cancel := context.WithTimeout(ctx, dur)
+				defer cancel()
+				profile, err := eng.ProfileSeparatedValueRetrievals(ctx)
+				if err != nil {
+					http.Error(w, "error profiling separated value retrievals", http.StatusInternalServerError)
+					return
+				}
+				w.Header().Set("Content-Type", "text/plain")
+				w.WriteHeader(http.StatusOK)
+				fmt.Fprint(w, profile.String())
+			})
 	}
 	return nil
 }

pkg/sql/sem/tree/inject_hints_test.go

@@ -241,7 +241,7 @@ func TestRandomInjectHints(t *testing.T) {
 	skip.UnderDeadlock(t, "the test is too slow")
 	skip.UnderRace(t, "the test is too slow")
 
-	const numStatements = 500
+	const numStatements = 100
 
 	ctx := context.Background()
 	rng, seed := randutil.NewTestRand()

pkg/sql/sem/tree/parse_array_test.go

@@ -8,12 +8,12 @@ package tree
 import (
 	"bytes"
 	"context"
-	"math/rand"
 	"testing"
 
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/randutil"
 )
 
 var tupleOfTwoInts = types.MakeTuple([]*types.T{types.Int, types.Int})
@@ -144,21 +144,22 @@ type noopUnwrapCompareContext struct {
 
 func (noopUnwrapCompareContext) UnwrapDatum(ctx context.Context, d Datum) Datum { return d }
 
-const randomArrayIterations = 1000
+const randomArrayIterations = 100
 const randomArrayMaxLength = 10
 const randomStringMaxLength = 1000
 
 func TestParseArrayRandomParseArray(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
+	rng, _ := randutil.NewTestRand()
 	for i := 0; i < randomArrayIterations; i++ {
-		numElems := rand.Intn(randomArrayMaxLength)
+		numElems := rng.Intn(randomArrayMaxLength)
 		ary := make([][]byte, numElems)
 		for aryIdx := range ary {
-			len := rand.Intn(randomStringMaxLength)
+			len := rng.Intn(randomStringMaxLength)
 			str := make([]byte, len)
 			for strIdx := 0; strIdx < len; strIdx++ {
-				str[strIdx] = byte(rand.Intn(256))
+				str[strIdx] = byte(rng.Intn(256))
 			}
 			ary[aryIdx] = str
 		}
@@ -175,7 +176,7 @@ func TestParseArrayRandomParseArray(t *testing.T) {
 			// means that there's no printable way to encode non-printing characters,
 			// users must use `e` prefixed strings).
 			for _, c := range b {
-				if c == '"' || c == '\\' || rand.Intn(10) == 0 {
+				if c == '"' || c == '\\' || rng.Intn(10) == 0 {
 					buf.WriteByte('\\')
 				}
 				buf.WriteByte(c)