From 7c6ba91c26ae31c124904aada3cd1ee87a734145 Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Tue, 25 Jul 2023 10:59:18 -0700 Subject: [PATCH] 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 --- .../internal/scbuildstmt/create_index.go | 6 ++-- pkg/sql/sqltestutils/BUILD.bazel | 1 + pkg/sql/sqltestutils/telemetry.go | 35 ++++++------------- pkg/sql/telemetry_test.go | 2 ++ pkg/sql/testdata/telemetry/index | 14 +++----- pkg/testutils/lint/lint_test.go | 1 - 6 files changed, 21 insertions(+), 38 deletions(-) diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_index.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_index.go index f47f35d86adc..d9342090cf1d 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_index.go +++ b/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()) diff --git a/pkg/sql/sqltestutils/BUILD.bazel b/pkg/sql/sqltestutils/BUILD.bazel index 28ba198e627f..6cc5066e30ee 100644 --- a/pkg/sql/sqltestutils/BUILD.bazel +++ b/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", diff --git a/pkg/sql/sqltestutils/telemetry.go b/pkg/sql/sqltestutils/telemetry.go index 73cbcb66bb2c..1a7fb7c27741 100644 --- a/pkg/sql/sqltestutils/telemetry.go +++ b/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,30 +91,16 @@ 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) }) }) @@ -121,9 +108,9 @@ func TelemetryTest(t *testing.T, serverArgs []base.TestServerArgs, testTenant bo // 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,7 +261,7 @@ 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) @@ -282,7 +269,7 @@ func (tt *telemetryTest) RunTest( 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)) diff --git a/pkg/sql/telemetry_test.go b/pkg/sql/telemetry_test.go index 488ceb30192e..642423f87347 100644 --- a/pkg/sql/telemetry_test.go +++ b/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") diff --git a/pkg/sql/testdata/telemetry/index b/pkg/sql/testdata/telemetry/index index 87c48a318b2a..7de127f6a5c4 100644 --- a/pkg/sql/testdata/telemetry/index +++ b/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. diff --git a/pkg/testutils/lint/lint_test.go b/pkg/testutils/lint/lint_test.go index 660245fa4b2b..1d4b925e5249 100644 --- a/pkg/testutils/lint/lint_test.go +++ b/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",