From 62110c08d9307204b855431fa1e805377438a6a1 Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Wed, 25 Jan 2023 08:47:39 -0800 Subject: [PATCH 1/6] colinfo: add missing type families into CanHaveCompositeKeyEncoding This commit adds several missing type families into `CanHaveCompositeKeyEncoding` method. Some of these type families are internal and, probably, don't need to be handled, but we recently introduced TSVector and TSQuery types which were incorrectly marked as being composite since they were not mentioned explicitly in the type family switch. This commit also makes it so that we panic in this method if we forget to include a newly-introduced type into this switch. Release note: None --- pkg/sql/catalog/colinfo/column_type_properties.go | 11 ++++++++--- .../catalog/colinfo/column_type_properties_test.go | 7 +++++-- pkg/sql/logictest/testdata/logic_test/tsvector | 5 +++++ 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/pkg/sql/catalog/colinfo/column_type_properties.go b/pkg/sql/catalog/colinfo/column_type_properties.go index 0ddc87bafed5..7eeeddbb3cea 100644 --- a/pkg/sql/catalog/colinfo/column_type_properties.go +++ b/pkg/sql/catalog/colinfo/column_type_properties.go @@ -16,6 +16,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/types" + "github.com/cockroachdb/errors" ) // CheckDatumTypeFitsColumnType verifies that a given scalar value @@ -80,12 +81,16 @@ func CanHaveCompositeKeyEncoding(typ *types.T) bool { types.GeometryFamily, types.GeographyFamily, types.EnumFamily, - types.Box2DFamily: + types.Box2DFamily, + types.VoidFamily, + types.EncodedKeyFamily, + types.TSQueryFamily, + types.TSVectorFamily: return false case types.UnknownFamily, types.AnyFamily: - fallthrough - default: return true + default: + panic(errors.AssertionFailedf("unsupported column family for type %s", typ.SQLString())) } } diff --git a/pkg/sql/catalog/colinfo/column_type_properties_test.go b/pkg/sql/catalog/colinfo/column_type_properties_test.go index a063e49fd737..fb985eaee505 100644 --- a/pkg/sql/catalog/colinfo/column_type_properties_test.go +++ b/pkg/sql/catalog/colinfo/column_type_properties_test.go @@ -73,8 +73,6 @@ func TestCanHaveCompositeKeyEncoding(t *testing.T) { {types.VarChar, false}, {types.MakeTuple([]*types.T{types.Int, types.Date}), false}, {types.MakeTuple([]*types.T{types.Float, types.Date}), true}, - // Test that a made up type with a bogus family will return true. - {&types.T{InternalType: types.InternalType{Family: 1 << 29}}, true}, } { // Note that sprint is used here because the bogus type family will // panic when formatting to a string and sprint will catch that. @@ -82,4 +80,9 @@ func TestCanHaveCompositeKeyEncoding(t *testing.T) { require.Equal(t, tc.exp, CanHaveCompositeKeyEncoding(tc.typ)) }) } + // Test that a made up type with a bogus family will panic. + t.Run("bogus", func(t *testing.T) { + bogusType := &types.T{InternalType: types.InternalType{Family: 1 << 29}} + require.Panics(t, func() { CanHaveCompositeKeyEncoding(bogusType) }) + }) } diff --git a/pkg/sql/logictest/testdata/logic_test/tsvector b/pkg/sql/logictest/testdata/logic_test/tsvector index 65ffb7d1428c..d080df2d04aa 100644 --- a/pkg/sql/logictest/testdata/logic_test/tsvector +++ b/pkg/sql/logictest/testdata/logic_test/tsvector @@ -235,3 +235,8 @@ SELECT a FROM a@a_a_idx WHERE a @@ 'ba:* & foo' # columns. statement error index \"a_a_idx\" is inverted and cannot be used for this query EXPLAIN SELECT * FROM a@a_a_idx WHERE a @@ b + +# Regression test for incorrectly marking TSVector type as composite (#95680). +statement ok +CREATE TABLE t95680 (c1 FLOAT NOT NULL, c2 TSVECTOR NOT NULL, INVERTED INDEX (c1 ASC, c2 ASC)); +INSERT INTO t95680 VALUES (1.0::FLOAT, e'\'kCrLZNl\' \'sVDj\' \'yO\' \'z\':54C,440B,519C,794B':::TSVECTOR); From 63caa61478baba311941033ec91cb2bbe3ae8894 Mon Sep 17 00:00:00 2001 From: maryliag Date: Wed, 25 Jan 2023 18:45:30 -0500 Subject: [PATCH 2/6] ui: add check for cpu usage Add a check, for cases when the value might no be returned (cluster with mixed versions). Part Of #87213 Release note: None --- .../cluster-ui/src/statementsTable/statementsTable.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.tsx b/pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.tsx index f33416e31d74..efe5387fcfdb 100644 --- a/pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.tsx @@ -224,7 +224,7 @@ export function makeStatementsColumns( title: statisticsTableTitles.cpu(statType), cell: cpuBar, sort: (stmt: AggregateStatistics) => - FixLong(Number(stmt.stats.exec_stats.cpu_nanos.mean)), + FixLong(Number(stmt.stats.exec_stats.cpu_nanos?.mean)), }, { name: "maxMemUsage", From 1786abfe98ace0e13a473eb52942208e42b629a2 Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Thu, 26 Jan 2023 08:28:54 -0800 Subject: [PATCH 3/6] colfetcher: disable direct columnar scans for now This commit disables direct columnar scans which are now randomly enabled in tests due to this feature having a data race at the moment. Release note: None --- pkg/sql/colfetcher/cfetcher_wrapper.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/sql/colfetcher/cfetcher_wrapper.go b/pkg/sql/colfetcher/cfetcher_wrapper.go index 3176b836b7a0..fec5c8f883a0 100644 --- a/pkg/sql/colfetcher/cfetcher_wrapper.go +++ b/pkg/sql/colfetcher/cfetcher_wrapper.go @@ -24,7 +24,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/colmem" "github.com/cockroachdb/cockroach/pkg/sql/execinfra" "github.com/cockroachdb/cockroach/pkg/storage" - "github.com/cockroachdb/cockroach/pkg/util" "github.com/cockroachdb/cockroach/pkg/util/mon" ) @@ -34,16 +33,17 @@ var DirectScansEnabled = settings.RegisterBoolSetting( settings.TenantWritable, "sql.distsql.direct_columnar_scans.enabled", "set to true to enable the 'direct' columnar scans in the KV layer", - directScansEnabledDefault, -) - -var directScansEnabledDefault = util.ConstantWithMetamorphicTestBool( - "direct-scans-enabled", - // TODO(yuzefovich, 23.1): update the default to 'true' for multi-tenant - // setups. false, ) +// TODO(yuzefovich): uncomment this when #95937 is fixed. +//var directScansEnabledDefault = util.ConstantWithMetamorphicTestBool( +// "direct-scans-enabled", +// // TODO(yuzefovich, 23.1): update the default to 'true' for multi-tenant +// // setups. +// false, +//) + // cFetcherWrapper implements the storage.CFetcherWrapper interface. See a large // comment in storage/col_mvcc.go for more details. type cFetcherWrapper struct { From 2ac04f71bd4598f72b3b8ad2cbd04cc7d62afa44 Mon Sep 17 00:00:00 2001 From: Andy Yang Date: Thu, 26 Jan 2023 11:50:34 -0500 Subject: [PATCH 4/6] binfetcher: fix binary downloading for arm64 MacOS This patch updates the suffix used to fetch arm64 MacOS binaries to match the naming scheme used to build releases. Release note: None --- pkg/util/binfetcher/binfetcher.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/util/binfetcher/binfetcher.go b/pkg/util/binfetcher/binfetcher.go index cff4a81c665f..3af803437a2d 100644 --- a/pkg/util/binfetcher/binfetcher.go +++ b/pkg/util/binfetcher/binfetcher.go @@ -86,7 +86,11 @@ func (opts *Options) init() error { goos := opts.GOOS if opts.GOOS == "darwin" { - goos += "-10.9" + if opts.GOARCH == "arm64" { + goos += "-11.0" + } else { + goos += "-10.9" + } } else if opts.GOOS == "windows" { goos += "-6.2" } From 3ef8fbbd2bdc082039ceeaa2e548bd99ee0b2e68 Mon Sep 17 00:00:00 2001 From: Jeremy Yang Date: Thu, 26 Jan 2023 08:56:00 -0800 Subject: [PATCH 5/6] roachtest/awsdms: fix race condition that can cause panics In #95518, we added a new test case to check for a table error during the DMS process. However there is a race when the check happens depending on how quickly tasks have ran and the task ReplicationTaskStats can be nil and we tried to access field on the nil object that was returned from the API call. This commit checks that the ReplicationTaskStat is not nil before accessing the TablesErrored property. Fixes: #93305 Release note: None --- pkg/cmd/roachtest/tests/awsdms.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/roachtest/tests/awsdms.go b/pkg/cmd/roachtest/tests/awsdms.go index 6d526e5a0c8a..09eb08010e49 100644 --- a/pkg/cmd/roachtest/tests/awsdms.go +++ b/pkg/cmd/roachtest/tests/awsdms.go @@ -327,9 +327,11 @@ func checkDMSNoPKTableError(ctx context.Context, t test.Test, dmsCli *dms.Client } } for _, task := range dmsTasks.ReplicationTasks { - if task.ReplicationTaskStats.TablesErrored == 1 { - t.L().Printf("table error was found") - return nil + if task.ReplicationTaskStats != nil { + if task.ReplicationTaskStats.TablesErrored == 1 { + t.L().Printf("table error was found") + return nil + } } } return errors.New("no table error found yet") From dbc8bfd85e5065fe1b30dad008f91bc10f88eb43 Mon Sep 17 00:00:00 2001 From: Renato Costa Date: Thu, 26 Jan 2023 14:16:16 -0500 Subject: [PATCH 6/6] roachtest: use teardown log when creating GitHub issue This is a follow up of #95831. The logger passed to the `githubIssues` struct writes to the test runner logger, which is not ideal. This changes the logger passed to use the `teardown` logger, so log entries related to GitHub issue creation are in the same directory as the failing test itself. Release note: None --- pkg/cmd/roachtest/github.go | 13 ++++--------- pkg/cmd/roachtest/github_test.go | 11 ++--------- pkg/cmd/roachtest/test_runner.go | 6 +++--- 3 files changed, 9 insertions(+), 21 deletions(-) diff --git a/pkg/cmd/roachtest/github.go b/pkg/cmd/roachtest/github.go index 88b214d55f84..f520ff85573c 100644 --- a/pkg/cmd/roachtest/github.go +++ b/pkg/cmd/roachtest/github.go @@ -26,7 +26,6 @@ import ( type githubIssues struct { disable bool - l *logger.Logger cluster *clusterImpl vmCreateOpts *vm.CreateOpts issuePoster func(context.Context, *logger.Logger, issues.IssueFormatter, issues.PostRequest) error @@ -41,15 +40,11 @@ const ( sshErr ) -func newGithubIssues( - disable bool, c *clusterImpl, vmCreateOpts *vm.CreateOpts, l *logger.Logger, -) *githubIssues { - +func newGithubIssues(disable bool, c *clusterImpl, vmCreateOpts *vm.CreateOpts) *githubIssues { return &githubIssues{ disable: disable, vmCreateOpts: vmCreateOpts, cluster: c, - l: l, issuePoster: issues.Post, teamLoader: team.DefaultLoadTeams, } @@ -201,10 +196,10 @@ func (g *githubIssues) createPostRequest( } } -func (g *githubIssues) MaybePost(t *testImpl, message string) error { +func (g *githubIssues) MaybePost(t *testImpl, l *logger.Logger, message string) error { doPost, skipReason := g.shouldPost(t) if !doPost { - g.l.Printf("skipping GitHub issue posting (%s)", skipReason) + l.Printf("skipping GitHub issue posting (%s)", skipReason) return nil } @@ -220,7 +215,7 @@ func (g *githubIssues) MaybePost(t *testImpl, message string) error { return g.issuePoster( context.Background(), - g.l, + l, issues.UnitTestFormatter, g.createPostRequest(t, cat, message), ) diff --git a/pkg/cmd/roachtest/github_test.go b/pkg/cmd/roachtest/github_test.go index d68a46bd11a5..d9ccdb298b80 100644 --- a/pkg/cmd/roachtest/github_test.go +++ b/pkg/cmd/roachtest/github_test.go @@ -91,14 +91,8 @@ func TestShouldPost(t *testing.T) { Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {}, } - ti := &testImpl{ - spec: testSpec, - l: nilLogger(), - } - - github := &githubIssues{ - disable: c.disableIssues, - } + ti := &testImpl{spec: testSpec} + github := &githubIssues{disable: c.disableIssues} doPost, skipReason := github.shouldPost(ti) require.Equal(t, c.expectedPost, doPost) @@ -188,7 +182,6 @@ func TestCreatePostRequest(t *testing.T) { github := &githubIssues{ vmCreateOpts: vmOpts, cluster: testClusterImpl, - l: nilLogger(), teamLoader: teamLoadFn, } diff --git a/pkg/cmd/roachtest/test_runner.go b/pkg/cmd/roachtest/test_runner.go index fd1a8a5f9e45..202715b619ab 100644 --- a/pkg/cmd/roachtest/test_runner.go +++ b/pkg/cmd/roachtest/test_runner.go @@ -638,7 +638,7 @@ func (r *testRunner) runWorker( // Now run the test. l.PrintfCtx(ctx, "starting test: %s:%d", testToRun.spec.Name, testToRun.runNum) - github := newGithubIssues(r.config.disableIssue, c, vmCreateOpts, l) + github := newGithubIssues(r.config.disableIssue, c, vmCreateOpts) if clusterCreateErr != nil { // N.B. cluster creation must have failed... @@ -648,7 +648,7 @@ func (r *testRunner) runWorker( // Generate failure reason and mark the test failed to preclude fetching (cluster) artifacts. t.Error(clusterCreateErr) // N.B. issue title is of the form "roachtest: ${t.spec.Name} failed" (see UnitTestFormatter). - if err := github.MaybePost(t, t.failureMsg()); err != nil { + if err := github.MaybePost(t, l, t.failureMsg()); err != nil { shout(ctx, l, stdout, "failed to post issue: %s", err) } } else { @@ -841,7 +841,7 @@ func (r *testRunner) runTest( shout(ctx, l, stdout, "--- FAIL: %s (%s)\n%s", runID, durationStr, output) - if err := github.MaybePost(t, output); err != nil { + if err := github.MaybePost(t, l, output); err != nil { shout(ctx, l, stdout, "failed to post issue: %s", err) } } else {