sql: remove stale skip in TestTelemetry

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.

Release note: None

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/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",