Merge cockroachdb#83711 cockroachdb#83733 cockroachdb#83809

83711: bazel: delete bespoke `string_test` target r=mari-crl a=rickystewart

This is an artifact of when `rules_go` had worse support for "external"
tests, but this works out-of-the-box now.

Release note: None

83733: roachtest: unskip large decommissionBench test r=AlexTalks a=AlexTalks

This extends the timeout of the large, 3000 warehouse decommission
benchmark roachtest to 3 hours, since it can take up to an hour for the
test to import data, achieve range count balance, and ramp up its
workload. This was skipped in cockroachdb#83445 due to frequent timeouts at the 1hr
mark.

It also adds a `--max-rate` parameter to the workload generator in order
to ensure the cluster avoids overload.

Closes cockroachdb#82870

Release note: None

83809: sql/sem/builtins: deflake TestSerialNormalizationWithUniqueUnorderedID r=ajwerner a=ajwerner

The test had two problems leading to flakes:
1) The statistic value was chosen with the wrong number of degrees of freedom.
   As such, it represented a 0.62% p value as opposed to a 0.001% p value.
2) The test is only meaningful when collected from a large N, which we did
   not use for the race test.

This should deflake the test.

Fixes cockroachdb#78075.

Release note: None

Co-authored-by: Ricky Stewart <[email protected]>
Co-authored-by: Alex Sarkesian <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>

pkg/BUILD.bazel

@@ -190,7 +190,6 @@ ALL_TESTS = [
     "//pkg/kv:kv_test",
     "//pkg/roachpb:roachpb_disallowed_imports_test",
     "//pkg/roachpb:roachpb_test",
-    "//pkg/roachpb:string_test",
     "//pkg/roachprod/cloud:cloud_test",
     "//pkg/roachprod/config:config_test",
     "//pkg/roachprod/install:install_test",

pkg/cmd/roachtest/tests/decommissionbench.go

@@ -44,6 +44,9 @@ type decommissionBenchSpec struct {
 	// When true, the test will attempt to stop the node prior to decommission.
 	whileDown bool
 
+	// An override for the default timeout, if needed.
+	timeout time.Duration
+
 	skip string
 }
 
@@ -86,7 +89,9 @@ func registerDecommissionBench(r registry.Registry) {
 			warehouses:       3000,
 			load:             true,
 			admissionControl: true,
-			skip:             "https://github.com/cockroachdb/cockroach/issues/82870",
+			// This test can take nearly an hour to import and achieve balance, so
+			// we extend the timeout to let it complete.
+			timeout: 3 * time.Hour,
 		},
 	} {
 		registerDecommissionBenchSpec(r, benchSpec)
@@ -96,6 +101,9 @@ func registerDecommissionBench(r registry.Registry) {
 // registerDecommissionBenchSpec adds a test using the specified configuration to the registry.
 func registerDecommissionBenchSpec(r registry.Registry, benchSpec decommissionBenchSpec) {
 	timeout := defaultTimeout
+	if benchSpec.timeout != time.Duration(0) {
+		timeout = benchSpec.timeout
+	}
 	extraNameParts := []string{""}
 
 	if benchSpec.snapshotRate != 0 {
@@ -166,12 +174,13 @@ func runDecommissionBench(
 		c.Start(ctx, t.L(), startOpts, install.MakeClusterSettings(), c.Node(i))
 	}
 
+	maxRate := tpccMaxRate(benchSpec.warehouses)
 	rampDuration := 3 * time.Minute
 	rampStarted := make(chan struct{}, 1)
 	importCmd := fmt.Sprintf(`./cockroach workload fixtures import tpcc --warehouses=%d`,
 		benchSpec.warehouses)
-	workloadCmd := fmt.Sprintf("./workload run tpcc --warehouses=%d --duration=%s "+
-		"--histograms=%s/stats.json --ramp=%s --tolerate-errors {pgurl:1-%d}", benchSpec.warehouses,
+	workloadCmd := fmt.Sprintf("./workload run tpcc --warehouses=%d --max-rate=%d --duration=%s "+
+		"--histograms=%s/stats.json --ramp=%s --tolerate-errors {pgurl:1-%d}", maxRate, benchSpec.warehouses,
 		testTimeout, t.PerfArtifactsDir(), rampDuration, benchSpec.nodes)
 	t.Status(fmt.Sprintf("initializing cluster with %d warehouses", benchSpec.warehouses))
 	c.Run(ctx, c.Node(pinnedNode), importCmd)
@@ -230,7 +239,7 @@ func runDecommissionBench(
 	// per-second "tick", we will simply tick at the start of the decommission
 	// and again at the completion. Roachperf will use the elapsed time between
 	// these ticks to plot the duration of the decommission.
-	tick, perfBuf := initBulkJobPerfArtifacts("decommission", defaultTimeout)
+	tick, perfBuf := initBulkJobPerfArtifacts("decommission", testTimeout)
 	recorder := &decommBenchTicker{pre: tick, post: tick}
 
 	m.Go(func(ctx context.Context) error {

pkg/cmd/roachtest/tests/tpcc.go

@@ -291,6 +291,14 @@ var tpccSupportedWarehouses = []struct {
 	{hardware: "gce-n5cpu16", v: version.MustParse(`v2.1.0-0`), warehouses: 1300},
 }
 
+// tpccMaxRate calculates the max rate of the workload given a number of warehouses.
+func tpccMaxRate(warehouses int) int {
+	const txnsPerWarehousePerSecond = 12.8 * (23.0 / 10.0) * (1.0 / 60.0) // max_tpmC/warehouse * all_txns/new_order_txns * minutes/seconds
+	rateAtExpected := txnsPerWarehousePerSecond * float64(warehouses)
+	maxRate := int(rateAtExpected / 2)
+	return maxRate
+}
+
 func maxSupportedTPCCWarehouses(
 	buildVersion version.Version, cloud string, nodes spec.ClusterSpec,
 ) int {
@@ -1016,9 +1024,7 @@ func loadTPCCBench(
 	// the desired distribution. This should allow for load-based rebalancing to
 	// help distribute load. Optionally pass some load configuration-specific
 	// flags.
-	const txnsPerWarehousePerSecond = 12.8 * (23.0 / 10.0) * (1.0 / 60.0) // max_tpmC/warehouse * all_txns/new_order_txns * minutes/seconds
-	rateAtExpected := txnsPerWarehousePerSecond * float64(b.EstimatedMax)
-	maxRate := int(rateAtExpected / 2)
+	maxRate := tpccMaxRate(b.EstimatedMax)
 	rampTime := (1 * rebalanceWait) / 4
 	loadTime := (3 * rebalanceWait) / 4
 	cmd = fmt.Sprintf("./cockroach workload run tpcc --warehouses=%d --workers=%d --max-rate=%d "+

pkg/roachpb/BUILD.bazel

@@ -1,5 +1,3 @@
-# gazelle:exclude string_test.go
-
 load("@rules_proto//proto:defs.bzl", "proto_library")
 load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library")
 load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
@@ -80,6 +78,7 @@ go_test(
         "metadata_test.go",
         "replica_unavailable_error_test.go",
         "span_group_test.go",
+        "string_test.go",
         "tenant_test.go",
         "version_test.go",
     ],
@@ -88,8 +87,10 @@ go_test(
     tags = ["no-remote"],
     deps = [
         "//pkg/cli/exit",
+        "//pkg/keys",
         "//pkg/kv/kvserver/concurrency/lock",
         "//pkg/storage/enginepb",
+        "//pkg/testutils/echotest",
         "//pkg/testutils/zerofields",
         "//pkg/util",
         "//pkg/util/bitarray",
@@ -120,27 +121,6 @@ go_test(
     ],
 )
 
-# keep
-go_test(
-    name = "string_test",
-    size = "small",
-    srcs = ["string_test.go"],
-    data = glob(["testdata/**"]),
-    deps = [
-        ":roachpb",  # keep
-        "//pkg/cli/exit",
-        "//pkg/keys",
-        "//pkg/kv/kvserver/concurrency/lock",
-        "//pkg/storage/enginepb",
-        "//pkg/testutils/echotest",
-        "//pkg/util/hlc",
-        "//pkg/util/uuid",
-        "@com_github_cockroachdb_errors//:errors",
-        "@com_github_cockroachdb_redact//:redact",
-        "@com_github_stretchr_testify//require",
-    ],
-)
-
 proto_library(
     name = "roachpb_proto",
     srcs = [

pkg/sql/sem/builtins/BUILD.bazel

@@ -161,6 +161,7 @@ go_test(
         "//pkg/sql/types",
         "//pkg/testutils",
         "//pkg/testutils/serverutils",
+        "//pkg/testutils/skip",
         "//pkg/testutils/sqlutils",
         "//pkg/testutils/testcluster",
         "//pkg/util",

pkg/sql/sem/builtins/builtins_test.go

@@ -26,8 +26,8 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
-	"github.com/cockroachdb/cockroach/pkg/util"
 	"github.com/cockroachdb/cockroach/pkg/util/duration"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
@@ -94,6 +94,8 @@ func TestMapToUniqueUnorderedID(t *testing.T) {
 func TestSerialNormalizationWithUniqueUnorderedID(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	ctx := context.Background()
+	skip.UnderRace(t, "the test is too slow and the goodness of fit test "+
+		"assumes large N")
 	params := base.TestServerArgs{}
 	s, db, _ := serverutils.StartServer(t, params)
 	defer s.Stopper().Stop(ctx)
@@ -110,10 +112,6 @@ CREATE TABLE t (
 )`)
 
 	numberOfRows := 10000
-	if util.RaceEnabled {
-		// We use a small number of rows because inserting rows under race is slow.
-		numberOfRows = 100
-	}
 
 	// Enforce 3 bits worth of range splits in the high order to collect range
 	// statistics after row insertions.
@@ -138,13 +136,13 @@ INSERT INTO t(j) SELECT * FROM generate_series(1, %d);
 	// chi-square goodness of fit statistic. We'll set our null hypothesis as
 	// 'each range in the distribution should have the same probability of getting
 	// a row inserted' and we'll check if we can reject the null hypothesis if
-	// chi-square is greater than the critical value we currently set as 19.5114,
+	// chi-square is greater than the critical value we currently set as 35.2585,
 	// a deliberate choice that gives us a p-value of 0.00001 according to
 	// https://www.fourmilab.ch/rpkp/experiments/analysis/chiCalc.html. If we are
 	// able to reject the null hypothesis, then the distribution is not uniform,
-	// and we raise an error.
+	// and we raise an error. This test has 7 degrees of freedom (categories - 1).
 	chiSquared := discreteUniformChiSquared(keyCounts)
-	criticalValue := 19.5114
+	criticalValue := 35.2585
 	require.Lessf(t, chiSquared, criticalValue, "chiSquared value of %f must be"+
 		" less than criticalVal %f to guarantee distribution is relatively uniform",
 		chiSquared, criticalValue)