Merge #115539 #115649 #115654

115539: roachtest: allocate new cluster when c.Extend fails r=srosenberg a=DarrylWong

Previously, failure to extend the lifetime of a cluster would propagate to the worker and end up aborting the entire run. Now, we destroy the cluster and let it allocate a new one.

Fixes: #112509
Release note: none
Epic: none

115649: cluster-ui: remove leading `/` in settings request r=xinhaoz a=xinhaoz

Recently we required that all requests paths from cluster-ui do not start with `/` so that they may be used with any given subpath when constructing proxy requests.

We missed removing the prefix from the `settings` api which requests all cluster settings.

Release note: None

Epic: none


----------------------
Previously, this auto stats enabled label would show disabled even though auto stats collection is enabled. This is because this value comes from the settings request which was not being issued correctly.
https://www.loom.com/share/42847f2c63484a508a128c5808df7bdd

115654: opt: fix insert fast path uniqueness check bug r=mgartner a=mgartner

This commit fixes a bug caused by creating a closure over an
incrementing integer in a loop.

Fixes #115377
Fixes #115378

Release note (bug fix): A bug has been fixed that caused node crashes
and panics when running `INSERT` queries on `REGIONAL BY ROW` tables
with `UNIQUE` constraints or indexes. The bug is only present in
version 23.2.0-beta.1.


Co-authored-by: DarrylWong <[email protected]>
Co-authored-by: Xin Hao Zhang <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_insert_fast_path

@@ -201,3 +201,18 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION]
 WHERE message LIKE '%batch%' AND message LIKE '%Scan%'
 ----
 r67: sending batch 4 Scan to (n1,s1):1
+
+# Regression test for #115377.
+statement ok
+CREATE TABLE t115377 (
+  k INT PRIMARY KEY,
+  a INT,
+  b INT,
+  UNIQUE (a, b)
+) LOCALITY REGIONAL BY ROW
+
+statement ok
+INSERT INTO t115377 VALUES (2, 0, 0)
+
+statement error pgcode 23505 duplicate key value violates unique constraint \"t115377_pkey\"
+INSERT INTO t115377 VALUES (2, 1, 1)

pkg/cmd/roachtest/cluster.go

@@ -2694,6 +2694,25 @@ func (c *clusterImpl) WipeForReuse(
 	return nil
 }
 
+// MaybeExtendCluster checks if the cluster has enough life left for the
+// test plus enough headroom after the test finishes so that the next test
+// can be selected. If it doesn't, extend it.
+func (c *clusterImpl) MaybeExtendCluster(
+	ctx context.Context, l *logger.Logger, testSpec registry.TestSpec,
+) error {
+	timeout := testTimeout(&testSpec)
+	minExp := timeutil.Now().Add(timeout + time.Hour)
+	if c.expiration.Before(minExp) {
+		extend := minExp.Sub(c.expiration)
+		l.PrintfCtx(ctx, "cluster needs to survive until %s, but has expiration: %s. Extending.",
+			minExp, c.expiration)
+		if err := c.Extend(ctx, extend, l); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 // archForTest determines the CPU architecture to use for a test. If the test
 // doesn't specify it, one is chosen randomly depending on flags.
 func archForTest(ctx context.Context, l *logger.Logger, testSpec registry.TestSpec) vm.CPUArch {

pkg/cmd/roachtest/test_runner.go

@@ -573,14 +573,18 @@ func (r *testRunner) runWorker(
 			// Try to reuse cluster.
 			testToRun = work.selectTestForCluster(ctx, c.spec, r.cr)
 			if !testToRun.noWork {
+				var err error
 				// We found a test to run on this cluster. Wipe the cluster.
-				if err := c.WipeForReuse(ctx, l, testToRun.spec.Cluster); err != nil {
+				if err = c.WipeForReuse(ctx, l, testToRun.spec.Cluster); err == nil {
+					// Extend the lifetime of the cluster if needed.
+					err = c.MaybeExtendCluster(ctx, l, testToRun.spec)
+				}
+				// We do not count reuse attempt error toward clusterCreateErr. If
+				// either the Wipe or Extend failed, then destroy the cluster and attempt
+				// to create a fresh cluster for the selected test.
+				if err != nil {
 					shout(ctx, l, stdout, "Unable to reuse cluster: %s due to: %s. Will attempt to create a fresh one",
 						c.Name(), err)
-					// We do not count reuse attempt error toward clusterCreateErr. Let's
-					// destroy the cluster and attempt to create a fresh cluster for the
-					// selected test.
-					//
 					// We don't release the quota allocation - the new cluster will be
 					// identical.
 					testToRun.canReuseCluster = false
@@ -1077,23 +1081,6 @@ func (r *testRunner) runTest(
 
 	t.start = timeutil.Now()
 
-	timeout := 3 * time.Hour
-	if d := s.Timeout; d != 0 {
-		timeout = d
-	}
-	// Make sure the cluster has enough life left for the test plus enough headroom
-	// after the test finishes so that the next test can be selected. If it
-	// doesn't, extend it.
-	minExp := timeutil.Now().Add(timeout + time.Hour)
-	if c.expiration.Before(minExp) {
-		extend := minExp.Sub(c.expiration)
-		l.PrintfCtx(ctx, "cluster needs to survive until %s, but has expiration: %s. Extending.",
-			minExp, c.expiration)
-		if err := c.Extend(ctx, extend, l); err != nil {
-			return errors.Wrapf(err, "failed to extend cluster: %s", c.name)
-		}
-	}
-
 	runCtx, cancel := context.WithCancel(ctx)
 	defer cancel()
 	t.mu.Lock()
@@ -1124,6 +1111,7 @@ func (r *testRunner) runTest(
 	}()
 
 	var timedOut bool
+	timeout := testTimeout(t.spec)
 
 	if grafanaAvailable {
 		// Shout this to the log and stdout to make it available to anyone watching the test via CI or locally.
@@ -1709,3 +1697,13 @@ func zipArtifacts(t *testImpl) error {
 	}
 	return moveToZipArchive("artifacts.zip", t.ArtifactsDir(), list...)
 }
+
+// testTimeout returns the timeout of a test. The default is set
+// to 3 hours but tests may specify their own timeouts.
+func testTimeout(spec *registry.TestSpec) time.Duration {
+	timeout := 3 * time.Hour
+	if d := spec.Timeout; d != 0 {
+		timeout = d
+	}
+	return timeout
+}

pkg/sql/logictest/testdata/logic_test/unique

@@ -1026,3 +1026,25 @@ ALTER TABLE multiple_uniq ADD CONSTRAINT uniq_fk FOREIGN KEY (d) REFERENCES uniq
 statement error pq: insert on table "multiple_uniq" violates foreign key constraint "uniq_fk"\nDETAIL: Key \(d\)=\(14\) is not present in table "uniq"\.
 INSERT INTO multiple_uniq (a,b,c,d)
 VALUES (11,12,13,14), (15,16,17,18)
+
+# Regression test for #115377.
+statement ok
+CREATE TABLE t115377 (
+  k INT,
+  a INT,
+  b INT,
+  s TEXT NOT NULL,
+  PRIMARY KEY (s, k),
+  UNIQUE WITHOUT INDEX (k),
+  UNIQUE (s, a, b),
+  UNIQUE WITHOUT INDEX (a, b),
+  CHECK (s IN ('east', 'west'))
+)
+
+statement ok
+INSERT INTO t115377 VALUES (2, 0, 0, 'east')
+
+statement error pgcode 23505 duplicate key value violates unique constraint \"unique_k\"
+INSERT INTO t115377 VALUES (2, 1, 1, 'east')
+
+subtest end

pkg/sql/opt/exec/execbuilder/mutation.go

@@ -197,8 +197,9 @@ func (b *Builder) tryBuildFastPathInsert(ins *memo.InsertExpr) (_ execPlan, ok b
 				execFastPathCheck.DatumsFromConstraint[j][execFastPathCheck.InsertCols[k]] = constExpr.Value
 			}
 		}
+		uniqCheck := &ins.UniqueChecks[i]
 		execFastPathCheck.MkErr = func(values tree.Datums) error {
-			return mkFastPathUniqueCheckErr(md, &ins.UniqueChecks[i], values, execFastPathCheck.ReferencedIndex)
+			return mkFastPathUniqueCheckErr(md, uniqCheck, values, execFastPathCheck.ReferencedIndex)
 		}
 	}
 

pkg/ui/workspaces/cluster-ui/src/api/clusterSettingsApi.ts

@@ -25,7 +25,7 @@ export function getClusterSettings(
 ): Promise<SettingsResponseMessage> {
   return fetchData(
     cockroach.server.serverpb.SettingsResponse,
-    `/${ADMIN_API_PREFIX}/settings?unredacted_values=true`,
+    `${ADMIN_API_PREFIX}/settings?unredacted_values=true`,
     cockroach.server.serverpb.SettingsRequest,
     req,
     timeout,