From 25d392cf2e9c46e22fd7057901228d27bc8c3c30 Mon Sep 17 00:00:00 2001 From: "James H. Linder" Date: Sat, 13 Aug 2022 15:29:34 -0400 Subject: [PATCH 1/3] CODEOWNERS: stop dev-inf PR review requests for gen file list changes Changes to most of the generated pkg/gen/*.bzl files happen for many PRs and they don't need to be reviewed by dev inf. Let's stop getting PR review requests for them. Release note: None --- .github/CODEOWNERS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 39cadd872b58..23fb0ef02b6b 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -188,6 +188,8 @@ /pkg/ccl/sqlproxyccl/ @cockroachdb/sqlproxy-prs @cockroachdb/server-prs /pkg/gen/ @cockroachdb/dev-inf +/pkg/gen/*.bzl @cockroachdb/dev-inf-noreview +/pkg/gen/gen.bzl @cockroachdb/dev-inf /pkg/acceptance/ @cockroachdb/sql-experience /pkg/base/ @cockroachdb/unowned @cockroachdb/kv-prs @cockroachdb/server-prs From 5eab50fba8a5b2c7d4805ba9b38aa317500c0534 Mon Sep 17 00:00:00 2001 From: Matthew Todd Date: Thu, 11 Aug 2022 16:59:08 -0400 Subject: [PATCH 2/3] insights: call ingester methods by reference Fixes the data race observed in #85988. Passing this data structure by value meant that the `fullHandler` was working with a different object than the external callers. Release justification: Category 3: Fixes for high-priority or high-severity bugs in existing functionality Release note: None --- pkg/sql/sqlstats/insights/ingester.go | 12 ++++++------ pkg/sql/sqlstats/insights/insights.go | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/sql/sqlstats/insights/ingester.go b/pkg/sql/sqlstats/insights/ingester.go index 46dca868de37..e57515896978 100644 --- a/pkg/sql/sqlstats/insights/ingester.go +++ b/pkg/sql/sqlstats/insights/ingester.go @@ -55,7 +55,7 @@ type event struct { statement *Statement } -func (i concurrentBufferIngester) Start(ctx context.Context, stopper *stop.Stopper) { +func (i *concurrentBufferIngester) Start(ctx context.Context, stopper *stop.Stopper) { // This task pulls buffers from the channel and forwards them along to the // underlying Registry. _ = stopper.RunAsyncTask(ctx, "insights-ingester", func(ctx context.Context) { @@ -86,7 +86,7 @@ func (i concurrentBufferIngester) Start(ctx context.Context, stopper *stop.Stopp }) } -func (i concurrentBufferIngester) ingest(events *eventBuffer) { +func (i *concurrentBufferIngester) ingest(events *eventBuffer) { for _, e := range events { // Because an eventBuffer is a fixed-size array, rather than a slice, // we do not know how full it is until we hit a nil entry. @@ -101,7 +101,7 @@ func (i concurrentBufferIngester) ingest(events *eventBuffer) { } } -func (i concurrentBufferIngester) ObserveStatement( +func (i *concurrentBufferIngester) ObserveStatement( sessionID clusterunique.ID, statement *Statement, ) { if !i.enabled() { @@ -115,7 +115,7 @@ func (i concurrentBufferIngester) ObserveStatement( }) } -func (i concurrentBufferIngester) ObserveTransaction( +func (i *concurrentBufferIngester) ObserveTransaction( sessionID clusterunique.ID, transaction *Transaction, ) { if !i.enabled() { @@ -129,13 +129,13 @@ func (i concurrentBufferIngester) ObserveTransaction( }) } -func (i concurrentBufferIngester) IterateInsights( +func (i *concurrentBufferIngester) IterateInsights( ctx context.Context, visitor func(context.Context, *Insight), ) { i.delegate.IterateInsights(ctx, visitor) } -func (i concurrentBufferIngester) enabled() bool { +func (i *concurrentBufferIngester) enabled() bool { return i.delegate.enabled() } diff --git a/pkg/sql/sqlstats/insights/insights.go b/pkg/sql/sqlstats/insights/insights.go index b5e049e3247c..45b02e372dd3 100644 --- a/pkg/sql/sqlstats/insights/insights.go +++ b/pkg/sql/sqlstats/insights/insights.go @@ -138,5 +138,5 @@ type Registry interface { // New builds a new Registry. func New(st *cluster.Settings, metrics Metrics) Registry { - return newRegistry(st, metrics) + return newConcurrentBufferIngester(newRegistry(st, metrics)) } From f49210b6296b83922a3a5b710b00e0d8253ae0e9 Mon Sep 17 00:00:00 2001 From: Andrew Werner Date: Mon, 15 Aug 2022 20:01:02 -0400 Subject: [PATCH 3/3] sql/schemachanger/sctest: run the Backup test over all stages Before this change, we'd only run them over the non-revertible stages. Release note: None --- pkg/sql/schemachanger/sctest/cumulative.go | 25 ++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/pkg/sql/schemachanger/sctest/cumulative.go b/pkg/sql/schemachanger/sctest/cumulative.go index c6b1bb0de250..ba5f8025813c 100644 --- a/pkg/sql/schemachanger/sctest/cumulative.go +++ b/pkg/sql/schemachanger/sctest/cumulative.go @@ -327,9 +327,11 @@ func Backup(t *testing.T, dir string, newCluster NewClusterFunc) { testFunc := func(t *testing.T, _ string, _ bool, setup, stmts []parser.Statement) { postCommit, nonRevertible := countStages(t, setup, stmts) n := postCommit + nonRevertible - - t.Logf("test case has %d revertible post-commit stages", n) - for i := postCommit; i <= n; i++ { + t.Logf( + "test case has %d revertible post-commit stages and %d non-revertible"+ + " post-commit stages", postCommit, nonRevertible, + ) + for i := 0; i <= n; i++ { if !t.Run( fmt.Sprintf("backup/restore stage %d of %d", i, n), func(t *testing.T) { testBackupRestoreCase(t, setup, stmts, i) }, @@ -405,7 +407,11 @@ func Backup(t *testing.T, dir string, newCluster NewClusterFunc) { var backups []backup var done bool var rollbackStage int - completedStages := make(map[int]struct{}) + type stageKey struct { + stage int + rollback bool + } + completedStages := make(map[stageKey]struct{}) for i := 0; !done; i++ { // We want to let the stages up to ord continue unscathed. Then, we'll // start taking backups at ord. If ord corresponds to a revertible @@ -423,10 +429,17 @@ func Backup(t *testing.T, dir string, newCluster NewClusterFunc) { s := <-stageChan // Move the index backwards if we see the same stage repeat due to a txn // retry error for example. - if _, ok := completedStages[s.stageIdx]; ok { + stage := stageKey{ + stage: s.stageIdx, + rollback: s.p.InRollback, + } + if _, ok := completedStages[stage]; ok { i-- + if stage.rollback { + rollbackStage-- + } } - completedStages[s.stageIdx] = struct{}{} + completedStages[stage] = struct{}{} shouldFail := ord == i && s.p.Stages[s.stageIdx].Phase != scop.PostCommitNonRevertiblePhase && !s.p.InRollback