Merge #107493 #107555 #107597

107493: ui,build: push cluster-ui assets into external folder during watch mode r=nathanstilwell a=sjbarag

Previously, watch mode builds of cluster-ui (e.g. 'dev ui watch' or 'pnpm build:watch') would emit files only to
pkg/ui/workspaces/cluster-ui/dist. Using that output in a watch task of a private repo required setting up symlinks via a 'make' task[1]. Unfortunately, that symlink made it far too easy for the node module resolution algorithm in that private repo to follow the symlink back to cockroach.git, which gave that project access to the modules in pkg/ui/node_modules/ and pkg/ui/workspaces/cluster-ui/node_modules. This resulted in webpack finding multiple copies of react-router (which expects to be a singleton), typescript finding multiple incompatible versions of react, etc.

Unfortunately, webpack doesn't support multiple output directories natively. Add a custom webpack plugin that copies emitted files to an arbitrary number of output directories.

[1] pnpm link doesn't work due to some package-name aliasing we've got
    going on there.

Release note: None
Epic: none

107555: sql: remove stale skip in TestTelemetry r=yuzefovich a=yuzefovich

This commit unskips multiple telemetry tests that were skipped for no good reason (they were referencing an unrelated issue). This uncovered some bugs in the new schema changer telemetry reporting where we duplicated `_index` twice in the feature counter for inverted indexes. Also, `index` telemetry test contained an invalid statement which is now removed.

The only file that is still skipped is `sql-stats` where the output doesn't match the expectations, and I'm not sure whether the test is stale or something is broken, so a separate issue was filed.

Addresses: #47893.
Epic: None.

Release note: None

107597: builtins: force production values in TestSerialNormalizationWithUniqueUnorderedID r=yuzefovich a=yuzefovich

We've observed that if `batch-bytes-limit` value is set too low, then the "key counts" query in this test takes much longer (on my laptop it was 60s for a particular random seed vs 2.4s with production values), so this commit forces some production values.

Fixes: #106829.

Release note: None

Co-authored-by: Sean Barag <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>

pkg/cmd/dev/testdata/datadriven/ui

@@ -81,6 +81,23 @@ cp -r sandbox/pkg/ui/workspaces/cluster-ui/dist crdb-checkout/pkg/ui/workspaces/
 pnpm --dir crdb-checkout/pkg/ui/workspaces/cluster-ui run build:watch
 pnpm --dir crdb-checkout/pkg/ui/workspaces/db-console exec webpack-dev-server --config webpack.config.js --mode development --env.WEBPACK_SERVE --env.dist=ccl --env.target=http://localhost:8080 --port 12345
 
+exec
+dev ui watch --cluster-ui-dst /path/to/foo --cluster-ui-dst /path/to/bar
+----
+bazel info workspace --color=no
+pnpm --dir crdb-checkout/pkg/ui install
+bazel build //pkg/ui/workspaces/cluster-ui:cluster-ui-lib //pkg/ui/workspaces/db-console/ccl/src/js:crdb-protobuf-client-ccl-lib
+bazel info bazel-bin --color=no
+bazel info workspace --color=no
+cp sandbox/pkg/ui/workspaces/db-console/src/js/protos.js crdb-checkout/pkg/ui/workspaces/db-console/src/js/protos.js
+cp sandbox/pkg/ui/workspaces/db-console/ccl/src/js/protos.js crdb-checkout/pkg/ui/workspaces/db-console/ccl/src/js/protos.js
+cp sandbox/pkg/ui/workspaces/db-console/src/js/protos.d.ts crdb-checkout/pkg/ui/workspaces/db-console/src/js/protos.d.ts
+cp sandbox/pkg/ui/workspaces/db-console/ccl/src/js/protos.d.ts crdb-checkout/pkg/ui/workspaces/db-console/ccl/src/js/protos.d.ts
+rm -rf crdb-checkout/pkg/ui/workspaces/cluster-ui/dist
+cp -r sandbox/pkg/ui/workspaces/cluster-ui/dist crdb-checkout/pkg/ui/workspaces/cluster-ui/dist
+pnpm --dir crdb-checkout/pkg/ui/workspaces/cluster-ui run build:watch --env.copy-to=/path/to/foo --env.copy-to=/path/to/bar
+pnpm --dir crdb-checkout/pkg/ui/workspaces/db-console exec webpack-dev-server --config webpack.config.js --mode development --env.WEBPACK_SERVE --env.dist=ccl --env.target=http://localhost:8080 --port 3000
+
 exec
 dev ui lint
 ----

pkg/cmd/dev/ui.go

@@ -269,6 +269,9 @@ func makeUIWatchCmd(d *dev) *cobra.Command {
 		// secureFlag is the name of the boolean long (GNU-style) flag that makes
 		// webpack's dev server use HTTPS.
 		secureFlag = "secure"
+		// clusterUiDestinationsFlag is the name of the long (GNU-style) flag that
+		// tells webpack where to copy emitted files during watch mode.
+		clusterUiDestinationsFlag = "cluster-ui-dst"
 	)
 
 	watchCmd := &cobra.Command{
@@ -334,23 +337,26 @@ Replaces 'make ui-watch'.`,
 				return err
 			}
 			port := fmt.Sprint(portNumber)
-			dbTarget, err := cmd.Flags().GetString(dbTargetFlag)
-			if err != nil {
-				log.Fatalf("unexpected error: %v", err)
-				return err
-			}
+			dbTarget := mustGetFlagString(cmd, dbTargetFlag)
 			_, err = url.Parse(dbTarget)
 			if err != nil {
 				log.Fatalf("invalid format for --%s argument: %v", dbTargetFlag, err)
 				return err
 			}
 			secure := mustGetFlagBool(cmd, secureFlag)
+			clusterUiDestinations := mustGetFlagStringArray(cmd, clusterUiDestinationsFlag)
 
-			// Start the cluster-ui watch task
+			// Start the cluster-ui watch tasks
 			nbExec := d.exec.AsNonBlocking()
 			argv := []string{
 				"--dir", dirs.clusterUI, "run", "build:watch",
 			}
+
+			// Add additional webpack args to copy cluster-ui output to external directories.
+			for _, dst := range clusterUiDestinations {
+				argv = append(argv, "--env.copy-to="+dst)
+			}
+
 			err = nbExec.CommandContextInheritingStdStreams(ctx, "pnpm", argv...)
 			if err != nil {
 				log.Fatalf("Unable to watch cluster-ui for changes: %v", err)
@@ -401,6 +407,11 @@ Replaces 'make ui-watch'.`,
 	watchCmd.Flags().String(dbTargetFlag, "http://localhost:8080", "url to proxy DB requests to")
 	watchCmd.Flags().Bool(secureFlag, false, "serve via HTTPS")
 	watchCmd.Flags().Bool(ossFlag, false, "build only the open-source parts of the UI")
+	watchCmd.Flags().StringArray(
+		clusterUiDestinationsFlag,
+		[]string{},
+		"directory to copy emitted cluster-ui files to. Can be set multiple times.",
+	)
 
 	return watchCmd
 }

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_index.go

@@ -79,9 +79,9 @@ func CreateIndex(b BuildCtx, n *tree.CreateIndex) {
 		if n.Unique {
 			panic(pgerror.New(pgcode.InvalidSQLStatementName, "inverted indexes can't be unique"))
 		}
-		b.IncrementSchemaChangeIndexCounter("inverted_index")
-		if len(n.Columns) > 0 {
-			b.IncrementSchemaChangeIndexCounter("multi_column_inverted_index")
+		b.IncrementSchemaChangeIndexCounter("inverted")
+		if len(n.Columns) > 1 {
+			b.IncrementSchemaChangeIndexCounter("multi_column_inverted")
 		}
 	}
 	activeVersion := b.EvalCtx().Settings.Version.ActiveVersion(context.TODO())

pkg/sql/sem/builtins/builtins_test.go

@@ -31,6 +31,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/duration"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/retry"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/errors"
@@ -100,6 +101,7 @@ func TestMapToUniqueUnorderedID(t *testing.T) {
 // by insertions guarantees a (somewhat) uniform distribution of the data.
 func TestSerialNormalizationWithUniqueUnorderedID(t *testing.T) {
 	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
 
 	skip.UnderRace(t, "the test is too slow and the goodness of fit test "+
 		"assumes large N")
@@ -109,7 +111,15 @@ func TestSerialNormalizationWithUniqueUnorderedID(t *testing.T) {
 	// This test can flake when the random data is not distributed evenly.
 	err := retry.WithMaxAttempts(ctx, retry.Options{}, attempts, func() error {
 
-		params := base.TestServerArgs{}
+		params := base.TestServerArgs{
+			Knobs: base.TestingKnobs{
+				SQLEvalContext: &eval.TestingKnobs{
+					// We disable the randomization of some batch sizes because
+					// with some low values the test takes much longer.
+					ForceProductionValues: true,
+				},
+			},
+		}
 		s, db, _ := serverutils.StartServer(t, params)
 		defer s.Stopper().Stop(ctx)
 

pkg/sql/sqltestutils/BUILD.bazel

@@ -28,6 +28,7 @@ go_library(
         "//pkg/sql/pgwire/pgcode",
         "//pkg/sql/pgwire/pgerror",
         "//pkg/sql/sem/catconstants",
+        "//pkg/sql/sqlstats/persistedsqlstats",
         "//pkg/testutils",
         "//pkg/testutils/diagutils",
         "//pkg/testutils/serverutils",

pkg/sql/sqltestutils/telemetry.go

@@ -28,6 +28,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/appstatspb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants"
+	"github.com/cockroachdb/cockroach/pkg/sql/sqlstats/persistedsqlstats"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/diagutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
@@ -90,40 +91,26 @@ func TelemetryTest(t *testing.T, serverArgs []base.TestServerArgs, testTenant bo
 		test.Start(t, serverArgs)
 		defer test.Close()
 
-		if testTenant || test.cluster.StartedDefaultTestTenant() {
-			// TODO(andyk): Re-enable these tests once tenant clusters fully
-			// support the features they're using.
-			switch path {
-			case "testdata/telemetry/execution",
-				// Index & multiregion are disabled because it requires
-				// multi-region syntax to be enabled for secondary tenants.
-				"testdata/telemetry/multiregion",
-				"testdata/telemetry/multiregion_db",
-				"testdata/telemetry/multiregion_table",
-				"testdata/telemetry/multiregion_row",
-				"testdata/telemetry/index",
-				"testdata/telemetry/planning",
-				"testdata/telemetry/sql-stats":
-				skip.WithIssue(t, 47893, "tenant clusters do not support SQL features used by this test")
-			}
+		if path == "testdata/telemetry/sql-stats" {
+			skip.WithIssue(t, 107593)
 		}
 
 		// Run test against physical CRDB cluster.
 		t.Run("server", func(t *testing.T) {
 			datadriven.RunTest(t, path, func(t *testing.T, td *datadriven.TestData) string {
-				sqlServer := test.server.SQLServer().(*sql.Server)
 				reporter := test.server.DiagnosticsReporter().(*diagnostics.Reporter)
-				return test.RunTest(td, test.serverDB, reporter.ReportDiagnostics, sqlServer)
+				statsController := test.server.SQLServer().(*sql.Server).GetSQLStatsController()
+				return test.RunTest(td, test.serverDB, reporter.ReportDiagnostics, statsController)
 			})
 		})
 
 		if testTenant {
 			// Run test against logical tenant cluster.
 			t.Run("tenant", func(t *testing.T) {
 				datadriven.RunTest(t, path, func(t *testing.T, td *datadriven.TestData) string {
-					sqlServer := test.server.SQLServer().(*sql.Server)
 					reporter := test.tenant.DiagnosticsReporter().(*diagnostics.Reporter)
-					return test.RunTest(td, test.tenantDB, reporter.ReportDiagnostics, sqlServer)
+					statsController := test.tenant.SQLServer().(*sql.Server).GetSQLStatsController()
+					return test.RunTest(td, test.tenantDB, reporter.ReportDiagnostics, statsController)
 				})
 			})
 		}
@@ -160,7 +147,7 @@ func (tt *telemetryTest) Start(t *testing.T, serverArgs []base.TestServerArgs) {
 	diagSrvURL := tt.diagSrv.URL()
 	mapServerArgs := make(map[int]base.TestServerArgs, len(serverArgs))
 	for i, v := range serverArgs {
-		v.DefaultTestTenant = base.TODOTestTenantDisabled
+		v.DefaultTestTenant = base.TestControlsTenantsExplicitly
 		v.Knobs.Server = &server.TestingKnobs{
 			DiagnosticsTestingKnobs: diagnostics.TestingKnobs{
 				OverrideReportingURL: &diagSrvURL,
@@ -195,7 +182,7 @@ func (tt *telemetryTest) RunTest(
 	td *datadriven.TestData,
 	db *gosql.DB,
 	reportDiags func(ctx context.Context),
-	sqlServer *sql.Server,
+	statsController *persistedsqlstats.Controller,
 ) (out string) {
 	defer func() {
 		if out == "" {
@@ -274,15 +261,15 @@ func (tt *telemetryTest) RunTest(
 
 	case "sql-stats":
 		// Report diagnostics once to reset the stats.
-		sqlServer.GetSQLStatsController().ResetLocalSQLStats(ctx)
+		statsController.ResetLocalSQLStats(ctx)
 		reportDiags(ctx)
 
 		_, err := db.Exec(td.Input)
 		var buf bytes.Buffer
 		if err != nil {
 			fmt.Fprintf(&buf, "error: %v\n", err)
 		}
-		sqlServer.GetSQLStatsController().ResetLocalSQLStats(ctx)
+		statsController.ResetLocalSQLStats(ctx)
 		reportDiags(ctx)
 		last := tt.diagSrv.LastRequestData()
 		buf.WriteString(formatSQLStats(last.SqlStats))

pkg/sql/telemetry_test.go

@@ -17,6 +17,7 @@ import (
 	"testing"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/cockroach/pkg/ccl"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqltestutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
@@ -30,6 +31,7 @@ import (
 func TestTelemetry(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
+	defer ccl.TestingEnableEnterprise()()
 	skip.UnderRace(t, "takes >1min under race")
 	skip.UnderDeadlock(t, "takes >1min under deadlock")
 

pkg/sql/testdata/telemetry/index

@@ -66,7 +66,7 @@ feature-usage
 CREATE TABLE err (i INT, j JSON, INVERTED INDEX (i, j) WHERE i);
 CREATE TABLE err (i INT, geom GEOMETRY, INVERTED INDEX (i, geom) WHERE i);
 ----
-error: pq: expected index predicate expression to have type bool, but 'i' has type int
+error: pq: expected INDEX PREDICATE expression to have type bool, but 'i' has type int
 
 feature-usage
 CREATE TABLE e (i INT, INDEX ((i + 10)))
@@ -132,7 +132,7 @@ feature-usage
 CREATE INVERTED INDEX err ON d (i, j) WHERE i;
 CREATE INVERTED INDEX err ON g4 (i, geom) WHERE i
 ----
-error: pq: expected index predicate expression to have type bool, but 'i' has type int
+error: pq: expected INDEX PREDICATE expression to have type bool, but 'i' has type int
 
 feature-usage
 CREATE INDEX i4 ON d ((i + 10))
@@ -142,20 +142,14 @@ sql.schema.expression_index
 feature-usage
 CREATE TABLE trigrams (t TEXT, u TEXT, INVERTED INDEX(t gin_trgm_ops))
 ----
-sql.schema.trigram_inverted_index
 sql.schema.inverted_index
+sql.schema.trigram_inverted_index
 
-exec
+feature-usage
 CREATE INDEX ON trigrams USING GIN(u gin_trgm_ops)
 ----
-sql.schema.trigram_inverted_index
 sql.schema.inverted_index
-
-exec
-CREATE INVERTED INDEX ON trigrams USING GIN(u gin_trgm_ops)
-----
 sql.schema.trigram_inverted_index
-sql.schema.inverted_index
 
 #### ALTER TABLE tests.
 

pkg/testutils/lint/lint_test.go

@@ -2242,7 +2242,6 @@ func TestLint(t *testing.T) {
 			":!sql/importer/read_import_mysql_test.go",
 			":!sql/schemachanger/sctest/test_server_factory.go",
 			":!sql/sqlinstance/instancestorage/instancecache_test.go",
-			":!sql/sqltestutils/telemetry.go",
 			":!sql/tests/server_params.go",
 			":!sql/ttl/ttljob/ttljob_test.go",
 			":!testutils/lint/lint_test.go",

pkg/ui/workspaces/cluster-ui/BUILD.bazel

@@ -11,7 +11,10 @@ load("@npm//pkg/ui/workspaces/cluster-ui:webpack-cli/package_json.bzl", webpack_
 npm_link_all_packages(name = "node_modules")
 
 WEBPACK_SRCS = glob(
-    ["src/**"],
+    [
+        "src/**",
+        "build/webpack/**",
+    ],
     exclude = [
         "src/**/*.stories.tsx",
         "src/**/*.spec.tsx",