Merge #73773 #73790 #73812

73773: roachtest: add knex test r=rafiss a=otan

These tests *will* fail on master as knex does not handle the PK/FK name
rename correctly. However, this commit alone makes it backportable to
v21.2, so this is being presented as is.

Release note: None

73790: bench/rttanalysis: stop printing text trace, write jaeger r=ajwerner a=ajwerner

This commit changes the behavior on failure of the rttanalysis tests.
Before it would print a textual trace. That was pretty much useless and
it was very loud. Now it'll write the jaeger trace json to a file in the
logging directory. Hopefully that'll be more useful to inspect the difference
between the expectation and the observed result.

Release note: None

73812: sql: fix some help texts r=otan a=knz

(first 3 commits from #73802.) 
a hiccup was introduced in #73802, but I don't want to cancel the CI / bors build to put the fix in, hence a new PR.

Co-authored-by: Oliver Tan <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>

pkg/bench/rttanalysis/rtt_analysis_bench.go

@@ -12,13 +12,16 @@ package rttanalysis
 
 import (
 	"context"
+	"io/ioutil"
+	"path/filepath"
 	"strings"
 	"sync"
 	"testing"
 
 	"github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord"
 	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
+	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/quotapool"
 	"github.com/cockroachdb/cockroach/pkg/util/tracing"
 	"github.com/cockroachdb/cockroach/pkg/util/tracing/tracingpb"
@@ -50,6 +53,7 @@ func runRoundTripBenchmark(b testingB, tests []RoundTripBenchTestCase, cc Cluste
 // concurrency.
 func runRoundTripBenchmarkTest(
 	t *testing.T,
+	scope *log.TestLogScope,
 	results *resultSet,
 	tests []RoundTripBenchTestCase,
 	cc ClusterConstructor,
@@ -63,7 +67,7 @@ func runRoundTripBenchmarkTest(
 		go func(tc RoundTripBenchTestCase) {
 			defer wg.Done()
 			t.Run(tc.Name, func(t *testing.T) {
-				runRoundTripBenchmarkTestCase(t, results, tc, cc, numRuns, limit)
+				runRoundTripBenchmarkTestCase(t, scope, results, tc, cc, numRuns, limit)
 			})
 		}(tc)
 	}
@@ -72,6 +76,7 @@ func runRoundTripBenchmarkTest(
 
 func runRoundTripBenchmarkTestCase(
 	t *testing.T,
+	scope *log.TestLogScope,
 	results *resultSet,
 	tc RoundTripBenchTestCase,
 	cc ClusterConstructor,
@@ -86,15 +91,18 @@ func runRoundTripBenchmarkTestCase(
 		go func() {
 			defer wg.Done()
 			defer alloc.Release()
-			executeRoundTripTest(tShim{T: t, results: results}, tc, cc)
+			executeRoundTripTest(tShim{
+				T: t, results: results, scope: scope,
+			}, tc, cc)
 		}()
 	}
 	wg.Wait()
 }
 
 // executeRoundTripTest executes a RoundTripBenchCase on with the provided SQL runner
 func executeRoundTripTest(b testingB, tc RoundTripBenchTestCase, cc ClusterConstructor) {
-	defer b.logScope()()
+	getDir, cleanup := b.logScope()
+	defer cleanup()
 
 	cluster := cc(b)
 	defer cluster.close()
@@ -139,11 +147,15 @@ func executeRoundTripTest(b testingB, tc RoundTripBenchTestCase, cc ClusterConst
 	}
 
 	res := float64(roundTrips) / float64(b.N())
+
 	if haveExp && !exp.matches(int(res)) && !*rewriteFlag {
-		b.Errorf(`got %v, expected %v. trace:
-%v
-(above trace from test %s. got %v, expected %v)
-`, res, exp, r, b.Name(), res, exp)
+		b.Errorf(`%s: got %v, expected %v`, b.Name(), res, exp)
+		dir := getDir()
+		jaegerJSON, err := r.ToJaegerJSON(tc.Stmt, "", "n0")
+		require.NoError(b, err)
+		path := filepath.Join(dir, strings.Replace(b.Name(), "/", "_", -1)) + ".jaeger.json"
+		require.NoError(b, ioutil.WriteFile(path, []byte(jaegerJSON), 0666))
+		b.Errorf("wrote jaeger trace to %s", path)
 	}
 	b.ReportMetric(res, roundTripsMetric)
 }

pkg/bench/rttanalysis/testing_shims.go

@@ -28,7 +28,7 @@ type testingB interface {
 
 	// logScope is used to wrap log.Scope and make it available only in
 	// appropriate contexts.
-	logScope() func()
+	logScope() (getDirectory func() string, close func())
 }
 
 type bShim struct {
@@ -37,9 +37,9 @@ type bShim struct {
 
 var _ testingB = bShim{}
 
-func (b bShim) logScope() func() {
+func (b bShim) logScope() (getDirectory func() string, close func()) {
 	sc := log.Scope(b)
-	return func() { sc.Close(b) }
+	return sc.GetDirectory, func() { sc.Close(b) }
 }
 func (b bShim) N() int { return b.B.N }
 func (b bShim) Run(name string, f func(b testingB)) {
@@ -54,13 +54,17 @@ func (b bShim) Run(name string, f func(b testingB)) {
 // execution.
 type tShim struct {
 	*testing.T
+	scope   *log.TestLogScope
 	results *resultSet
 }
 
 var _ testingB = tShim{}
 
-func (ts tShim) logScope() func() {
-	return func() {}
+func (ts tShim) logScope() (getDirectory func() string, close func()) {
+	return ts.scope.GetDirectory, func() {}
+}
+func (ts tShim) GetDirectory() string {
+	return ts.scope.GetDirectory()
 }
 func (ts tShim) N() int      { return 1 }
 func (ts tShim) ResetTimer() {}
@@ -81,6 +85,6 @@ func (ts tShim) Name() string {
 }
 func (ts tShim) Run(s string, f func(testingB)) {
 	ts.T.Run(s, func(t *testing.T) {
-		f(tShim{results: ts.results, T: t})
+		f(tShim{results: ts.results, T: t, scope: ts.scope})
 	})
 }

pkg/bench/rttanalysis/validate_benchmark_data.go

@@ -66,7 +66,8 @@ func runBenchmarkExpectationTests(t *testing.T, r *Registry) {
 	}
 
 	// Only create the scope after we've checked if we need to exec the subprocess.
-	defer log.Scope(t).Close(t)
+	scope := log.Scope(t)
+	defer scope.Close(t)
 
 	defer func() {
 		if t.Failed() {
@@ -88,7 +89,7 @@ func runBenchmarkExpectationTests(t *testing.T, r *Registry) {
 				if isRewrite {
 					runs = *rewriteIterations
 				}
-				runRoundTripBenchmarkTest(t, &results, cases, r.cc, runs, limiter)
+				runRoundTripBenchmarkTest(t, scope, &results, cases, r.cc, runs, limiter)
 			})
 		}(b, cases)
 	}

pkg/cmd/roachtest/tests/BUILD.bazel

@@ -69,6 +69,7 @@ go_library(
         "java_helpers.go",
         "jepsen.go",
         "jobs.go",
+        "knex.go",
         "kv.go",
         "kvbench.go",
         "ledger.go",

pkg/cmd/roachtest/tests/knex.go

@@ -0,0 +1,129 @@
+// Copyright 2021 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package tests
+
+import (
+	"context"
+
+	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/cluster"
+	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry"
+	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test"
+	"github.com/stretchr/testify/require"
+)
+
+const supportedKnexTag = "0.95.14"
+
+// This test runs one of knex's test suite against a single cockroach
+// node.
+
+func registerKnex(r registry.Registry) {
+	runKnex := func(
+		ctx context.Context,
+		t test.Test,
+		c cluster.Cluster,
+	) {
+		if c.IsLocal() {
+			t.Fatal("cannot be run in local mode")
+		}
+		node := c.Node(1)
+		t.Status("setting up cockroach")
+		c.Put(ctx, t.Cockroach(), "./cockroach", c.All())
+		if err := c.PutLibraries(ctx, "./lib"); err != nil {
+			t.Fatal(err)
+		}
+		c.Start(ctx, c.All())
+
+		version, err := fetchCockroachVersion(ctx, c, node[0])
+		require.NoError(t, err)
+
+		err = alterZoneConfigAndClusterSettings(ctx, version, c, node[0])
+		require.NoError(t, err)
+
+		err = repeatRunE(
+			ctx,
+			t,
+			c,
+			node,
+			"create sql database",
+			`./cockroach sql --insecure -e "CREATE DATABASE test"`,
+		)
+		require.NoError(t, err)
+
+		err = repeatRunE(
+			ctx,
+			t,
+			c,
+			node,
+			"add nodesource repository",
+			`sudo apt install ca-certificates && curl -sL https://deb.nodesource.com/setup_12.x | sudo -E bash -`,
+		)
+		require.NoError(t, err)
+
+		err = repeatRunE(
+			ctx, t, c, node, "install nodejs and npm", `sudo apt-get -qq install nodejs`,
+		)
+		require.NoError(t, err)
+
+		err = repeatRunE(
+			ctx, t, c, node, "update npm", `sudo npm i -g npm`,
+		)
+		require.NoError(t, err)
+
+		err = repeatRunE(
+			ctx, t, c, node, "install mocha", `sudo npm i -g mocha`,
+		)
+		require.NoError(t, err)
+
+		err = repeatRunE(
+			ctx, t, c, node, "remove old knex", `sudo rm -rf /mnt/data1/knex`,
+		)
+		require.NoError(t, err)
+
+		err = repeatGitCloneE(
+			ctx,
+			t,
+			c,
+			"https://github.com/knex/knex.git",
+			"/mnt/data1/knex",
+			supportedKnexTag,
+			node,
+		)
+		require.NoError(t, err)
+
+		err = repeatRunE(
+			ctx, t, c, node, "install knex npm dependencies", `cd /mnt/data1/knex/ && sudo npm i`,
+		)
+		require.NoError(t, err)
+
+		t.Status("running knex tests")
+		rawResults, err := c.RunWithBuffer(
+			ctx,
+			t.L(),
+			node,
+			`cd /mnt/data1/knex/ && DB='cockroachdb' npm test`,
+		)
+		rawResultsStr := string(rawResults)
+		t.L().Printf("Test Results: %s", rawResultsStr)
+		if err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	r.Add(registry.TestSpec{
+		Name:    "knex",
+		Owner:   registry.OwnerSQLExperience,
+		Cluster: r.MakeClusterSpec(1),
+		Tags:    []string{`default`, `orm`},
+		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
+			runKnex(ctx, t, c)
+		},
+	})
+}

pkg/cmd/roachtest/tests/registry.go

@@ -55,6 +55,7 @@ func RegisterTests(r registry.Registry) {
 	registerJasyncSQL(r)
 	RegisterJepsen(r)
 	registerJobsMixedVersions(r)
+	registerKnex(r)
 	registerKV(r)
 	registerKVContention(r)
 	registerKVQuiescenceDead(r)

pkg/sql/parser/sql.y

@@ -1554,7 +1554,7 @@ alter_ddl_stmt:
 //   ALTER TABLE ... UNSPLIT ALL
 //   ALTER TABLE ... SCATTER [ FROM ( <exprs...> ) TO ( <exprs...> ) ]
 //   ALTER TABLE ... INJECT STATISTICS ...  (experimental)
-//   ALTER TABLE ... RELOCATE [ LEASE | VOTERS | NONVOTERS ] FOR <selectclause>  (experimental)
+//   ALTER TABLE ... RELOCATE [ LEASE | VOTERS | NONVOTERS ] <selectclause>  (experimental)
 //   ALTER TABLE ... PARTITION BY RANGE ( <name...> ) ( <rangespec> )
 //   ALTER TABLE ... PARTITION BY LIST ( <name...> ) ( <listspec> )
 //   ALTER TABLE ... PARTITION BY NOTHING
@@ -1810,7 +1810,7 @@ alter_range_stmt:
 //   ALTER INDEX ... UNSPLIT AT <selectclause>
 //   ALTER INDEX ... UNSPLIT ALL
 //   ALTER INDEX ... SCATTER [ FROM ( <exprs...> ) TO ( <exprs...> ) ]
-//   ALTER INDEX ... RELOCATE [ LEASE | VOTERS | NONVOTERS ] FOR <selectclause>
+//   ALTER INDEX ... RELOCATE [ LEASE | VOTERS | NONVOTERS ] <selectclause>
 //
 // Zone configurations:
 //   DISCARD