From 7cfcce172d7e7e04329a760dff30eb8aeef7cfe5 Mon Sep 17 00:00:00 2001 From: Drew Kimball Date: Mon, 28 Nov 2022 15:17:53 -0800 Subject: [PATCH 1/6] opt: fix rare incorrect results due to sort between paired joins Previously, it was possible for paired joins to produce incorrect results in the case when an ordering was required of their output, and a sort was added between the paired joins to enforce the ordering. This patch prevents a sort from being added to the output of the first join in a set of paired joins. This is necessary because the continuation column that is used to indicate false positives matched by the first join relies on the ordering being maintained between the joins. Fixes #89603 Release note: None --- pkg/sql/opt/ordering/ordering.go | 24 ++++++ pkg/sql/opt/xform/optimizer.go | 2 +- pkg/sql/opt/xform/testdata/rules/join | 114 ++++++++++++++++++++++++++ 3 files changed, 139 insertions(+), 1 deletion(-) diff --git a/pkg/sql/opt/ordering/ordering.go b/pkg/sql/opt/ordering/ordering.go index ed314f0d88f1..edc911e46a4d 100644 --- a/pkg/sql/opt/ordering/ordering.go +++ b/pkg/sql/opt/ordering/ordering.go @@ -31,6 +31,30 @@ func CanProvide(expr memo.RelExpr, required *props.OrderingChoice) bool { return funcMap[expr.Op()].canProvideOrdering(expr, required) } +// CanEnforce returns true if the output of the given operator can be sorted +// in order to satisfy the given required ordering. +func CanEnforce(expr memo.RelExpr, required *props.OrderingChoice) bool { + if required.Any() { + return false + } + if buildutil.CrdbTestBuild { + checkRequired(expr, required) + } + switch t := expr.(type) { + case *memo.ExplainExpr: + return false + case *memo.LookupJoinExpr: + // For paired joins we use a boolean continuation column to handle false + // positive matches in the first join. This relies on the ordering being + // unchanged between the first and second joins, so adding a sort on top + // of this expression could lead to incorrect results. + return !t.IsFirstJoinInPairedJoiner + case *memo.InvertedJoinExpr: + return !t.IsFirstJoinInPairedJoiner + } + return true +} + // BuildChildRequired returns the ordering that must be required of its // given child in order to satisfy a required ordering. Can only be called if // CanProvide is true for the required ordering. diff --git a/pkg/sql/opt/xform/optimizer.go b/pkg/sql/opt/xform/optimizer.go index 2f25a27a6a9b..23d89535d251 100644 --- a/pkg/sql/opt/xform/optimizer.go +++ b/pkg/sql/opt/xform/optimizer.go @@ -655,7 +655,7 @@ func (o *Optimizer) enforceProps( return o.optimizeEnforcer(state, getEnforcer, required, member) } - if !required.Ordering.Any() && member.Op() != opt.ExplainOp { + if ordering.CanEnforce(member, &required.Ordering) { // Try Sort enforcer that requires no ordering from its input. getEnforcer := func() memo.RelExpr { enforcer := o.getScratchSort() diff --git a/pkg/sql/opt/xform/testdata/rules/join b/pkg/sql/opt/xform/testdata/rules/join index b9e7a1389dd4..7f14d5a74981 100644 --- a/pkg/sql/opt/xform/testdata/rules/join +++ b/pkg/sql/opt/xform/testdata/rules/join @@ -13208,3 +13208,117 @@ project └── filters (true) ### END regression tests for issue #69617 + +# Regression test for #89603 +exec-ddl +CREATE TABLE table11 ( + col1_1 FLOAT8 NOT NULL, + col1_2 TIMESTAMP NOT NULL, + col1_3 BYTES NOT NULL, + col1_4 NAME NOT NULL, + col1_5 OID NULL, + col1_6 JSONB NULL, + col1_7 REGPROCEDURE NULL, + INDEX table1_col1_3_col1_1_expr_idx (col1_3 ASC, col1_1 DESC) + ) +---- + +# The sort must be done as the last step instead of in between the first and +# second joins of the paired join. +opt set=testing_optimizer_random_seed=4057832385546395198 set=testing_optimizer_cost_perturbation=1.0 +SELECT + tab_17534.col1_3 AS col_52140, + 0:::OID AS col_52141, + tab_17533.col1_7 AS col_52142, + '\x58':::BYTES AS col_52143, + 0:::OID AS col_52144 +FROM + table11@[0] AS tab_17533 + RIGHT JOIN table11@[0] AS tab_17534 + JOIN table11@[0] AS tab_17535 ON + (tab_17534.crdb_internal_mvcc_timestamp) = (tab_17535.crdb_internal_mvcc_timestamp) + AND (tab_17534.col1_4) = (tab_17535.col1_4) + JOIN table11@[0] AS tab_17536 ON + (tab_17535.col1_1) = (tab_17536.col1_1) + AND (tab_17535.col1_3) = (tab_17536.col1_3) + AND (tab_17534.col1_6) = (tab_17536.col1_6) + JOIN table11@[0] AS tab_17537 + JOIN table11@[0] AS tab_17538 ON + (tab_17537.crdb_internal_mvcc_timestamp) = (tab_17538.crdb_internal_mvcc_timestamp) ON + (tab_17536.col1_2) = (tab_17538.col1_2) AND (tab_17535.col1_4) = (tab_17537.col1_4) ON + (tab_17533.col1_3) = (tab_17537.col1_3) AND (tab_17533.col1_6) = (tab_17536.col1_6) +ORDER BY + tab_17535.col1_4, tab_17535.col1_3 DESC +---- +project + ├── columns: col_52140:13!null col_52141:61!null col_52142:7 col_52143:62!null col_52144:61!null [hidden: tab_17535.col1_3:23!null tab_17535.col1_4:24!null] + ├── immutable + ├── fd: ()-->(61,62) + ├── ordering: +24,-23 opt(61,62) [actual: +24,-23] + ├── sort + │ ├── columns: tab_17533.col1_3:3 tab_17533.col1_6:6 tab_17533.col1_7:7 tab_17534.col1_3:13!null tab_17534.col1_4:14!null tab_17534.col1_6:16!null tab_17534.crdb_internal_mvcc_timestamp:19!null tab_17535.col1_1:21!null tab_17535.col1_3:23!null tab_17535.col1_4:24!null tab_17535.crdb_internal_mvcc_timestamp:29!null tab_17536.col1_1:31!null tab_17536.col1_2:32!null tab_17536.col1_3:33!null tab_17536.col1_6:36!null tab_17537.col1_3:43!null tab_17537.col1_4:44!null tab_17537.crdb_internal_mvcc_timestamp:49!null tab_17538.col1_2:52!null tab_17538.crdb_internal_mvcc_timestamp:59!null + │ ├── immutable + │ ├── fd: (19)==(29), (29)==(19), (14)==(24,44), (24)==(14,44), (21)==(31), (31)==(21), (23)==(33), (33)==(23), (16)==(36), (36)==(16), (49)==(59), (59)==(49), (32)==(52), (52)==(32), (44)==(14,24) + │ ├── ordering: +(14|24|44),-(23|33) [actual: +14,-23] + │ └── left-join (lookup table11 [as=tab_17533]) + │ ├── columns: tab_17533.col1_3:3 tab_17533.col1_6:6 tab_17533.col1_7:7 tab_17534.col1_3:13!null tab_17534.col1_4:14!null tab_17534.col1_6:16!null tab_17534.crdb_internal_mvcc_timestamp:19!null tab_17535.col1_1:21!null tab_17535.col1_3:23!null tab_17535.col1_4:24!null tab_17535.crdb_internal_mvcc_timestamp:29!null tab_17536.col1_1:31!null tab_17536.col1_2:32!null tab_17536.col1_3:33!null tab_17536.col1_6:36!null tab_17537.col1_3:43!null tab_17537.col1_4:44!null tab_17537.crdb_internal_mvcc_timestamp:49!null tab_17538.col1_2:52!null tab_17538.crdb_internal_mvcc_timestamp:59!null + │ ├── key columns: [70] = [8] + │ ├── lookup columns are key + │ ├── second join in paired joiner + │ ├── immutable + │ ├── fd: (19)==(29), (29)==(19), (14)==(24,44), (24)==(14,44), (21)==(31), (31)==(21), (23)==(33), (33)==(23), (16)==(36), (36)==(16), (49)==(59), (59)==(49), (32)==(52), (52)==(32), (44)==(14,24) + │ ├── left-join (lookup table11@table1_col1_3_col1_1_expr_idx [as=tab_17533]) + │ │ ├── columns: tab_17534.col1_3:13!null tab_17534.col1_4:14!null tab_17534.col1_6:16!null tab_17534.crdb_internal_mvcc_timestamp:19!null tab_17535.col1_1:21!null tab_17535.col1_3:23!null tab_17535.col1_4:24!null tab_17535.crdb_internal_mvcc_timestamp:29!null tab_17536.col1_1:31!null tab_17536.col1_2:32!null tab_17536.col1_3:33!null tab_17536.col1_6:36!null tab_17537.col1_3:43!null tab_17537.col1_4:44!null tab_17537.crdb_internal_mvcc_timestamp:49!null tab_17538.col1_2:52!null tab_17538.crdb_internal_mvcc_timestamp:59!null tab_17533.col1_1:63 tab_17533.col1_3:65 tab_17533.rowid:70 continuation:73 + │ │ ├── key columns: [43] = [65] + │ │ ├── first join in paired joiner; continuation column: continuation:73 + │ │ ├── immutable + │ │ ├── fd: (19)==(29), (29)==(19), (14)==(24,44), (24)==(14,44), (21)==(31), (31)==(21), (23)==(33), (33)==(23), (16)==(36), (36)==(16), (49)==(59), (59)==(49), (32)==(52), (52)==(32), (44)==(14,24), (70)-->(63,65,73) + │ │ ├── inner-join (hash) + │ │ │ ├── columns: tab_17534.col1_3:13!null tab_17534.col1_4:14!null tab_17534.col1_6:16!null tab_17534.crdb_internal_mvcc_timestamp:19!null tab_17535.col1_1:21!null tab_17535.col1_3:23!null tab_17535.col1_4:24!null tab_17535.crdb_internal_mvcc_timestamp:29!null tab_17536.col1_1:31!null tab_17536.col1_2:32!null tab_17536.col1_3:33!null tab_17536.col1_6:36!null tab_17537.col1_3:43!null tab_17537.col1_4:44!null tab_17537.crdb_internal_mvcc_timestamp:49!null tab_17538.col1_2:52!null tab_17538.crdb_internal_mvcc_timestamp:59!null + │ │ │ ├── immutable + │ │ │ ├── fd: (19)==(29), (29)==(19), (14)==(24,44), (24)==(14,44), (21)==(31), (31)==(21), (23)==(33), (33)==(23), (16)==(36), (36)==(16), (49)==(59), (59)==(49), (32)==(52), (52)==(32), (44)==(14,24) + │ │ │ ├── inner-join (hash) + │ │ │ │ ├── columns: tab_17534.col1_3:13!null tab_17534.col1_4:14!null tab_17534.col1_6:16!null tab_17534.crdb_internal_mvcc_timestamp:19!null tab_17535.col1_1:21!null tab_17535.col1_3:23!null tab_17535.col1_4:24!null tab_17535.crdb_internal_mvcc_timestamp:29!null tab_17536.col1_1:31!null tab_17536.col1_2:32!null tab_17536.col1_3:33!null tab_17536.col1_6:36!null tab_17537.col1_3:43!null tab_17537.col1_4:44!null tab_17537.crdb_internal_mvcc_timestamp:49 + │ │ │ │ ├── multiplicity: left-rows(zero-or-more), right-rows(one-or-more) + │ │ │ │ ├── immutable + │ │ │ │ ├── fd: (24)==(14,44), (44)==(14,24), (21)==(31), (31)==(21), (23)==(33), (33)==(23), (19)==(29), (29)==(19), (14)==(24,44), (16)==(36), (36)==(16) + │ │ │ │ ├── scan table11 [as=tab_17537] + │ │ │ │ │ └── columns: tab_17537.col1_3:43!null tab_17537.col1_4:44!null tab_17537.crdb_internal_mvcc_timestamp:49 + │ │ │ │ ├── inner-join (lookup table11 [as=tab_17536]) + │ │ │ │ │ ├── columns: tab_17534.col1_3:13!null tab_17534.col1_4:14!null tab_17534.col1_6:16!null tab_17534.crdb_internal_mvcc_timestamp:19!null tab_17535.col1_1:21!null tab_17535.col1_3:23!null tab_17535.col1_4:24!null tab_17535.crdb_internal_mvcc_timestamp:29!null tab_17536.col1_1:31!null tab_17536.col1_2:32!null tab_17536.col1_3:33!null tab_17536.col1_6:36!null + │ │ │ │ │ ├── key columns: [38] = [38] + │ │ │ │ │ ├── lookup columns are key + │ │ │ │ │ ├── immutable + │ │ │ │ │ ├── fd: (19)==(29), (29)==(19), (14)==(24), (24)==(14), (21)==(31), (31)==(21), (23)==(33), (33)==(23), (16)==(36), (36)==(16) + │ │ │ │ │ ├── inner-join (lookup table11@table1_col1_3_col1_1_expr_idx [as=tab_17536]) + │ │ │ │ │ │ ├── columns: tab_17534.col1_3:13!null tab_17534.col1_4:14!null tab_17534.col1_6:16 tab_17534.crdb_internal_mvcc_timestamp:19!null tab_17535.col1_1:21!null tab_17535.col1_3:23!null tab_17535.col1_4:24!null tab_17535.crdb_internal_mvcc_timestamp:29!null tab_17536.col1_1:31!null tab_17536.col1_3:33!null tab_17536.rowid:38!null + │ │ │ │ │ │ ├── key columns: [23 21] = [33 31] + │ │ │ │ │ │ ├── immutable + │ │ │ │ │ │ ├── fd: (19)==(29), (29)==(19), (14)==(24), (24)==(14), (38)-->(31,33), (23)==(33), (33)==(23), (21)==(31), (31)==(21) + │ │ │ │ │ │ ├── inner-join (hash) + │ │ │ │ │ │ │ ├── columns: tab_17534.col1_3:13!null tab_17534.col1_4:14!null tab_17534.col1_6:16 tab_17534.crdb_internal_mvcc_timestamp:19!null tab_17535.col1_1:21!null tab_17535.col1_3:23!null tab_17535.col1_4:24!null tab_17535.crdb_internal_mvcc_timestamp:29!null + │ │ │ │ │ │ │ ├── immutable + │ │ │ │ │ │ │ ├── fd: (19)==(29), (29)==(19), (14)==(24), (24)==(14) + │ │ │ │ │ │ │ ├── scan table11 [as=tab_17535] + │ │ │ │ │ │ │ │ └── columns: tab_17535.col1_1:21!null tab_17535.col1_3:23!null tab_17535.col1_4:24!null tab_17535.crdb_internal_mvcc_timestamp:29 + │ │ │ │ │ │ │ ├── scan table11 [as=tab_17534] + │ │ │ │ │ │ │ │ └── columns: tab_17534.col1_3:13!null tab_17534.col1_4:14!null tab_17534.col1_6:16 tab_17534.crdb_internal_mvcc_timestamp:19 + │ │ │ │ │ │ │ └── filters + │ │ │ │ │ │ │ ├── tab_17534.crdb_internal_mvcc_timestamp:19 = tab_17535.crdb_internal_mvcc_timestamp:29 [outer=(19,29), immutable, constraints=(/19: (/NULL - ]; /29: (/NULL - ]), fd=(19)==(29), (29)==(19)] + │ │ │ │ │ │ │ └── tab_17534.col1_4:14 = tab_17535.col1_4:24 [outer=(14,24), constraints=(/14: (/NULL - ]; /24: (/NULL - ]), fd=(14)==(24), (24)==(14)] + │ │ │ │ │ │ └── filters (true) + │ │ │ │ │ └── filters + │ │ │ │ │ └── tab_17534.col1_6:16 = tab_17536.col1_6:36 [outer=(16,36), immutable, constraints=(/16: (/NULL - ]; /36: (/NULL - ]), fd=(16)==(36), (36)==(16)] + │ │ │ │ └── filters + │ │ │ │ └── tab_17535.col1_4:24 = tab_17537.col1_4:44 [outer=(24,44), constraints=(/24: (/NULL - ]; /44: (/NULL - ]), fd=(24)==(44), (44)==(24)] + │ │ │ ├── scan table11 [as=tab_17538] + │ │ │ │ └── columns: tab_17538.col1_2:52!null tab_17538.crdb_internal_mvcc_timestamp:59 + │ │ │ └── filters + │ │ │ ├── tab_17537.crdb_internal_mvcc_timestamp:49 = tab_17538.crdb_internal_mvcc_timestamp:59 [outer=(49,59), immutable, constraints=(/49: (/NULL - ]; /59: (/NULL - ]), fd=(49)==(59), (59)==(49)] + │ │ │ └── tab_17536.col1_2:32 = tab_17538.col1_2:52 [outer=(32,52), constraints=(/32: (/NULL - ]; /52: (/NULL - ]), fd=(32)==(52), (52)==(32)] + │ │ └── filters (true) + │ └── filters + │ └── tab_17533.col1_6:6 = tab_17536.col1_6:36 [outer=(6,36), immutable, constraints=(/6: (/NULL - ]; /36: (/NULL - ]), fd=(6)==(36), (36)==(6)] + └── projections + ├── 0 [as=col_52141:61] + └── '\x58' [as=col_52143:62] From 756c6715e290bfc1955a8a0c70aae45fcdd6c04c Mon Sep 17 00:00:00 2001 From: Matthew Todd Date: Tue, 22 Nov 2022 10:33:05 -0500 Subject: [PATCH 2/6] sqlstats: record idle latency for transactions Part of #86667 Follows #91098 Release note (sql change): A new NumericStat, idleLat, was introduced to the statistics column of crdb_internal.transaction_statistics, reporting the time spent waiting for the client to send statements while holding a transaction open. --- pkg/roachpb/app_stats.go | 1 + pkg/roachpb/app_stats.proto | 4 + pkg/sql/conn_executor.go | 5 + pkg/sql/conn_executor_exec.go | 6 + pkg/sql/executor_statement_metrics.go | 1 + pkg/sql/sessionphase/session_phase.go | 17 ++- .../sqlstatsutil/json_encoding.go | 2 + .../sqlstatsutil/json_encoding_test.go | 4 + .../sqlstatsutil/json_impl.go | 1 + pkg/sql/sqlstats/sslocal/sql_stats_test.go | 103 +++++++++++------- .../sqlstats/ssmemstorage/ss_mem_writer.go | 1 + pkg/sql/sqlstats/ssprovider.go | 1 + .../cluster-ui/src/transactionsPage/utils.ts | 1 + 13 files changed, 105 insertions(+), 42 deletions(-) diff --git a/pkg/roachpb/app_stats.go b/pkg/roachpb/app_stats.go index e038e18b213b..96006e92f102 100644 --- a/pkg/roachpb/app_stats.go +++ b/pkg/roachpb/app_stats.go @@ -124,6 +124,7 @@ func (t *TransactionStatistics) Add(other *TransactionStatistics) { t.MaxRetries = other.MaxRetries } + t.IdleLat.Add(other.IdleLat, t.Count, other.Count) t.CommitLat.Add(other.CommitLat, t.Count, other.Count) t.RetryLat.Add(other.RetryLat, t.Count, other.Count) t.ServiceLat.Add(other.ServiceLat, t.Count, other.Count) diff --git a/pkg/roachpb/app_stats.proto b/pkg/roachpb/app_stats.proto index c0ffe36577ec..03ac2aa10534 100644 --- a/pkg/roachpb/app_stats.proto +++ b/pkg/roachpb/app_stats.proto @@ -149,6 +149,10 @@ message TransactionStatistics { // all statement operations have been applied. optional NumericStat commit_lat = 6 [(gogoproto.nullable) = false]; + // IdleLat is the cumulative amount of time spent in seconds waiting for + // the client to send statements while holding the transaction open. + optional NumericStat idle_lat = 11 [(gogoproto.nullable) = false]; + // BytesRead collects the number of bytes read from disk. optional NumericStat bytes_read = 7 [(gogoproto.nullable) = false]; diff --git a/pkg/sql/conn_executor.go b/pkg/sql/conn_executor.go index c3acee83d0a7..d2739d75b7d2 100644 --- a/pkg/sql/conn_executor.go +++ b/pkg/sql/conn_executor.go @@ -1398,6 +1398,11 @@ type connExecutor struct { shouldCollectTxnExecutionStats bool // accumulatedStats are the accumulated stats of all statements. accumulatedStats execstats.QueryLevelStats + + // idleLatency is the cumulative amount of time spent waiting for the + // client to send statements while holding the transaction open. + idleLatency time.Duration + // rowsRead and bytesRead are separate from QueryLevelStats because they are // accumulated independently since they are always collected, as opposed to // QueryLevelStats which are sampled. diff --git a/pkg/sql/conn_executor_exec.go b/pkg/sql/conn_executor_exec.go index 13118bc111e0..74e60b158b25 100644 --- a/pkg/sql/conn_executor_exec.go +++ b/pkg/sql/conn_executor_exec.go @@ -914,6 +914,9 @@ func (ex *connExecutor) resetTransactionOnSchemaChangeRetry(ctx context.Context) func (ex *connExecutor) commitSQLTransaction( ctx context.Context, ast tree.Statement, commitFn func(context.Context) error, ) (fsm.Event, fsm.EventPayload) { + ex.extraTxnState.idleLatency += ex.statsCollector.PhaseTimes(). + GetIdleLatency(ex.statsCollector.PreviousPhaseTimes()) + ex.phaseTimes.SetSessionPhaseTime(sessionphase.SessionStartTransactionCommit, timeutil.Now()) if err := commitFn(ctx); err != nil { if descs.IsTwoVersionInvariantViolationError(err) { @@ -2285,6 +2288,7 @@ func (ex *connExecutor) onTxnRestart(ctx context.Context) { // accumulatedStats are cleared, but shouldCollectTxnExecutionStats is // unchanged. ex.extraTxnState.accumulatedStats = execstats.QueryLevelStats{} + ex.extraTxnState.idleLatency = 0 ex.extraTxnState.rowsRead = 0 ex.extraTxnState.bytesRead = 0 ex.extraTxnState.rowsWritten = 0 @@ -2317,6 +2321,7 @@ func (ex *connExecutor) recordTransactionStart(txnID uuid.UUID) { ex.extraTxnState.numRows = 0 ex.extraTxnState.shouldCollectTxnExecutionStats = false ex.extraTxnState.accumulatedStats = execstats.QueryLevelStats{} + ex.extraTxnState.idleLatency = 0 ex.extraTxnState.rowsRead = 0 ex.extraTxnState.bytesRead = 0 ex.extraTxnState.rowsWritten = 0 @@ -2396,6 +2401,7 @@ func (ex *connExecutor) recordTransactionFinish( ServiceLatency: txnServiceLat, RetryLatency: txnRetryLat, CommitLatency: commitLat, + IdleLatency: ex.extraTxnState.idleLatency, RowsAffected: ex.extraTxnState.numRows, CollectedExecStats: ex.planner.instrumentation.collectExecStats, ExecStats: ex.extraTxnState.accumulatedStats, diff --git a/pkg/sql/executor_statement_metrics.go b/pkg/sql/executor_statement_metrics.go index 3cbcb182e3f6..4b36fe2c6ed2 100644 --- a/pkg/sql/executor_statement_metrics.go +++ b/pkg/sql/executor_statement_metrics.go @@ -252,6 +252,7 @@ func (ex *connExecutor) recordStatementSummary( ex.extraTxnState.transactionStatementsHash.Add(uint64(stmtFingerprintID)) } ex.extraTxnState.numRows += rowsAffected + ex.extraTxnState.idleLatency += idleLatRaw if log.V(2) { // ages since significant epochs diff --git a/pkg/sql/sessionphase/session_phase.go b/pkg/sql/sessionphase/session_phase.go index 9216383fbe0a..ae8337ddc015 100644 --- a/pkg/sql/sessionphase/session_phase.go +++ b/pkg/sql/sessionphase/session_phase.go @@ -216,16 +216,23 @@ func (t *Times) GetSessionAge() time.Duration { func (t *Times) GetIdleLatency(previous *Times) time.Duration { queryReceived := t.times[SessionQueryReceived] + previousQueryReceived := time.Time{} + previousQueryServiced := time.Time{} + if previous != nil { + previousQueryReceived = previous.times[SessionQueryReceived] + previousQueryServiced = previous.times[SessionQueryServiced] + } + // If we were received at the same time as the previous execution // (i.e., as part of a compound statement), we didn't have to wait // for the client at all. - if queryReceived == previous.times[SessionQueryReceived] { + if queryReceived == previousQueryReceived { return 0 } // In general, we have been waiting for the client since the end // of the previous execution. - waitingSince := previous.times[SessionQueryServiced] + waitingSince := previousQueryServiced // Although we really only want to measure idle latency *within* // an open transaction. So if we're in a new transaction, measure @@ -234,5 +241,11 @@ func (t *Times) GetIdleLatency(previous *Times) time.Duration { waitingSince = transactionStarted } + // And if we were received before we started waiting for the client, + // then there is no idle latency at all. + if waitingSince.After(queryReceived) { + return 0 + } + return queryReceived.Sub(waitingSince) } diff --git a/pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_encoding.go b/pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_encoding.go index 9e43708f675d..b9eeae6625c2 100644 --- a/pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_encoding.go +++ b/pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_encoding.go @@ -218,6 +218,7 @@ func BuildTxnMetadataJSON(statistics *roachpb.CollectedTransactionStatistics) (j // "svcLat": { "$ref": "#/definitions/numeric_stats" }, // "retryLat": { "$ref": "#/definitions/numeric_stats" }, // "commitLat": { "$ref": "#/definitions/numeric_stats" }, +// "idleLat": { "$ref": "#/definitions/numeric_stats" }, // "bytesRead": { "$ref": "#/definitions/numeric_stats" }, // "rowsRead": { "$ref": "#/definitions/numeric_stats" } // }, @@ -227,6 +228,7 @@ func BuildTxnMetadataJSON(statistics *roachpb.CollectedTransactionStatistics) (j // "svcLat", // "retryLat", // "commitLat", +// "idleLat", // "bytesRead", // "rowsRead", // ] diff --git a/pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_encoding_test.go b/pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_encoding_test.go index 0d10a9ad6ba0..3d4330061aa9 100644 --- a/pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_encoding_test.go +++ b/pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_encoding_test.go @@ -318,6 +318,10 @@ func TestSQLStatsJsonEncoding(t *testing.T) { "mean": {{.Float}}, "sqDiff": {{.Float}} }, + "idleLat": { + "mean": {{.Float}}, + "sqDiff": {{.Float}} + }, "bytesRead": { "mean": {{.Float}}, "sqDiff": {{.Float}} diff --git a/pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_impl.go b/pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_impl.go index 1a71a3749b87..5c10011b3ca7 100644 --- a/pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_impl.go +++ b/pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_impl.go @@ -303,6 +303,7 @@ func (t *innerTxnStats) jsonFields() jsonFields { {"svcLat", (*numericStats)(&t.ServiceLat)}, {"retryLat", (*numericStats)(&t.RetryLat)}, {"commitLat", (*numericStats)(&t.CommitLat)}, + {"idleLat", (*numericStats)(&t.IdleLat)}, {"bytesRead", (*numericStats)(&t.BytesRead)}, {"rowsRead", (*numericStats)(&t.RowsRead)}, {"rowsWritten", (*numericStats)(&t.RowsWritten)}, diff --git a/pkg/sql/sqlstats/sslocal/sql_stats_test.go b/pkg/sql/sqlstats/sslocal/sql_stats_test.go index 4ccdd49f15b3..bece50da04cb 100644 --- a/pkg/sql/sqlstats/sslocal/sql_stats_test.go +++ b/pkg/sql/sqlstats/sslocal/sql_stats_test.go @@ -1121,13 +1121,15 @@ func TestSQLStatsIdleLatencies(t *testing.T) { defer s.Stopper().Stop(ctx) testCases := []struct { - name string - lats map[string]float64 - ops func(*testing.T, *gosql.DB) + name string + stmtLats map[string]float64 + txnLat float64 + ops func(*testing.T, *gosql.DB) }{ { - name: "no latency", - lats: map[string]float64{"SELECT _": 0}, + name: "no latency", + stmtLats: map[string]float64{"SELECT _": 0}, + txnLat: 0, ops: func(t *testing.T, db *gosql.DB) { tx, err := db.Begin() require.NoError(t, err) @@ -1138,8 +1140,9 @@ func TestSQLStatsIdleLatencies(t *testing.T) { }, }, { - name: "no latency (implicit txn)", - lats: map[string]float64{"SELECT _": 0}, + name: "no latency (implicit txn)", + stmtLats: map[string]float64{"SELECT _": 0}, + txnLat: 0, ops: func(t *testing.T, db *gosql.DB) { // These 100ms don't count because we're not in an explicit transaction. time.Sleep(100 * time.Millisecond) @@ -1148,8 +1151,9 @@ func TestSQLStatsIdleLatencies(t *testing.T) { }, }, { - name: "no latency - prepared statement (implicit txn)", - lats: map[string]float64{"SELECT $1::INT8": 0}, + name: "no latency - prepared statement (implicit txn)", + stmtLats: map[string]float64{"SELECT $1::INT8": 0}, + txnLat: 0, ops: func(t *testing.T, db *gosql.DB) { stmt, err := db.Prepare("SELECT $1::INT") require.NoError(t, err) @@ -1159,8 +1163,9 @@ func TestSQLStatsIdleLatencies(t *testing.T) { }, }, { - name: "simple statement", - lats: map[string]float64{"SELECT _": 0.1}, + name: "simple statement", + stmtLats: map[string]float64{"SELECT _": 0.1}, + txnLat: 0.2, ops: func(t *testing.T, db *gosql.DB) { tx, err := db.Begin() require.NoError(t, err) @@ -1173,8 +1178,9 @@ func TestSQLStatsIdleLatencies(t *testing.T) { }, }, { - name: "compound statement", - lats: map[string]float64{"SELECT _": 0.1, "SELECT count(*) FROM crdb_internal.statement_statistics": 0}, + name: "compound statement", + stmtLats: map[string]float64{"SELECT _": 0.1, "SELECT count(*) FROM crdb_internal.statement_statistics": 0}, + txnLat: 0.2, ops: func(t *testing.T, db *gosql.DB) { tx, err := db.Begin() require.NoError(t, err) @@ -1187,8 +1193,9 @@ func TestSQLStatsIdleLatencies(t *testing.T) { }, }, { - name: "multiple statements - slow generation", - lats: map[string]float64{"SELECT pg_sleep(_)": 0, "SELECT _": 0}, + name: "multiple statements - slow generation", + stmtLats: map[string]float64{"SELECT pg_sleep(_)": 0, "SELECT _": 0}, + txnLat: 0, ops: func(t *testing.T, db *gosql.DB) { tx, err := db.Begin() require.NoError(t, err) @@ -1201,8 +1208,9 @@ func TestSQLStatsIdleLatencies(t *testing.T) { }, }, { - name: "prepared statement", - lats: map[string]float64{"SELECT $1::INT8": 0.1}, + name: "prepared statement", + stmtLats: map[string]float64{"SELECT $1::INT8": 0.1}, + txnLat: 0.2, ops: func(t *testing.T, db *gosql.DB) { stmt, err := db.Prepare("SELECT $1::INT") require.NoError(t, err) @@ -1217,8 +1225,9 @@ func TestSQLStatsIdleLatencies(t *testing.T) { }, }, { - name: "prepared statement inside transaction", - lats: map[string]float64{"SELECT $1::INT8": 0.1}, + name: "prepared statement inside transaction", + stmtLats: map[string]float64{"SELECT $1::INT8": 0.1}, + txnLat: 0.2, ops: func(t *testing.T, db *gosql.DB) { tx, err := db.Begin() require.NoError(t, err) @@ -1233,8 +1242,9 @@ func TestSQLStatsIdleLatencies(t *testing.T) { }, }, { - name: "multiple transactions", - lats: map[string]float64{"SELECT _": 0.1}, + name: "multiple transactions", + stmtLats: map[string]float64{"SELECT _": 0.1}, + txnLat: 0.2, ops: func(t *testing.T, db *gosql.DB) { for i := 0; i < 3; i++ { tx, err := db.Begin() @@ -1274,34 +1284,47 @@ func TestSQLStatsIdleLatencies(t *testing.T) { // Run the test operations. tc.ops(t, opsDB) - // Look for the latencies we expect. - actual := make(map[string]float64) - rows, err := db.Query(` - SELECT metadata->>'query', statistics->'statistics'->'idleLat'->'mean' - FROM crdb_internal.statement_statistics - WHERE app_name = $1`, appName) - require.NoError(t, err) - for rows.Next() { - var query string - var latency float64 - err = rows.Scan(&query, &latency) - require.NoError(t, err) - actual[query] = latency - } - require.NoError(t, rows.Err()) - // Make looser timing assertions in CI, since we've seen // more variability there. // - Bazel test runs also use this looser delta. // - Goland and `go test` use the tighter delta unless // the crdb_test build tag has been set. - delta := 0.002 + delta := 0.003 if buildutil.CrdbTestBuild { delta = 0.05 } - require.InDeltaMapValues(t, tc.lats, actual, delta, - "expected: %v\nactual: %v", tc.lats, actual) + // Look for the latencies we expect. + t.Run("stmt", func(t *testing.T) { + actual := make(map[string]float64) + rows, err := db.Query(` + SELECT metadata->>'query', statistics->'statistics'->'idleLat'->'mean' + FROM crdb_internal.statement_statistics + WHERE app_name = $1`, appName) + require.NoError(t, err) + for rows.Next() { + var query string + var latency float64 + err = rows.Scan(&query, &latency) + require.NoError(t, err) + actual[query] = latency + } + require.NoError(t, rows.Err()) + require.InDeltaMapValues(t, tc.stmtLats, actual, delta, + "expected: %v\nactual: %v", tc.stmtLats, actual) + }) + + t.Run("txn", func(t *testing.T) { + var actual float64 + row := db.QueryRow(` + SELECT statistics->'statistics'->'idleLat'->'mean' + FROM crdb_internal.transaction_statistics + WHERE app_name = $1`, appName) + err := row.Scan(&actual) + require.NoError(t, err) + require.GreaterOrEqual(t, actual, float64(0)) + require.InDelta(t, tc.txnLat, actual, delta) + }) }) } } diff --git a/pkg/sql/sqlstats/ssmemstorage/ss_mem_writer.go b/pkg/sql/sqlstats/ssmemstorage/ss_mem_writer.go index cea08471db70..7d27396c0a27 100644 --- a/pkg/sql/sqlstats/ssmemstorage/ss_mem_writer.go +++ b/pkg/sql/sqlstats/ssmemstorage/ss_mem_writer.go @@ -301,6 +301,7 @@ func (s *Container) RecordTransaction( stats.mu.data.ServiceLat.Record(stats.mu.data.Count, value.ServiceLatency.Seconds()) stats.mu.data.RetryLat.Record(stats.mu.data.Count, value.RetryLatency.Seconds()) stats.mu.data.CommitLat.Record(stats.mu.data.Count, value.CommitLatency.Seconds()) + stats.mu.data.IdleLat.Record(stats.mu.data.Count, value.IdleLatency.Seconds()) if value.RetryCount > stats.mu.data.MaxRetries { stats.mu.data.MaxRetries = value.RetryCount } diff --git a/pkg/sql/sqlstats/ssprovider.go b/pkg/sql/sqlstats/ssprovider.go index 66ddfd972abc..c00182debae4 100644 --- a/pkg/sql/sqlstats/ssprovider.go +++ b/pkg/sql/sqlstats/ssprovider.go @@ -234,6 +234,7 @@ type RecordedTxnStats struct { ServiceLatency time.Duration RetryLatency time.Duration CommitLatency time.Duration + IdleLatency time.Duration RowsAffected int CollectedExecStats bool ExecStats execstats.QueryLevelStats diff --git a/pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.ts b/pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.ts index daf1b5f8a1fc..2278c235e04c 100644 --- a/pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.ts +++ b/pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.ts @@ -340,6 +340,7 @@ function addTransactionStats( countA, countB, ), + idle_lat: aggregateNumericStats(a.idle_lat, b.idle_lat, countA, countB), rows_read: aggregateNumericStats(a.rows_read, b.rows_read, countA, countB), rows_written: aggregateNumericStats( a.rows_written, From 64624faab3540cce387b0b6cadf509364f94572f Mon Sep 17 00:00:00 2001 From: rharding6373 Date: Tue, 29 Nov 2022 13:29:47 -0800 Subject: [PATCH 3/6] dev: add rewritable paths for ccl execbuilder tests There are some ccl tests that use test files in `/pkg/sql/opt/exec/execbuilder`. This commit adds this as a rewritable path so that we can use the `--rewrite` flag with `dev`. Release note: None Epic: None --- pkg/cmd/dev/test.go | 1 + pkg/cmd/dev/testdata/datadriven/test | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/dev/test.go b/pkg/cmd/dev/test.go index be756015c0a6..348dea9b81d4 100644 --- a/pkg/cmd/dev/test.go +++ b/pkg/cmd/dev/test.go @@ -134,6 +134,7 @@ func (d *dev) test(cmd *cobra.Command, commandLine []string) error { // recursive directories ending in /... extraRewritablePaths = []struct{ pkg, path string }{ {"pkg/ccl/logictestccl", "pkg/sql/logictest"}, + {"pkg/ccl/logictestccl", "pkg/sql/opt/exec/execbuilder"}, {"pkg/sql/opt/memo", "pkg/sql/opt/testutils/opttester/testfixtures"}, {"pkg/sql/opt/norm", "pkg/sql/opt/testutils/opttester/testfixtures"}, {"pkg/sql/opt/xform", "pkg/sql/opt/testutils/opttester/testfixtures"}, diff --git a/pkg/cmd/dev/testdata/datadriven/test b/pkg/cmd/dev/testdata/datadriven/test index 122991b58cb3..e8d69c52b5e2 100644 --- a/pkg/cmd/dev/testdata/datadriven/test +++ b/pkg/cmd/dev/testdata/datadriven/test @@ -84,7 +84,7 @@ exec dev test pkg/ccl/logictestccl -f=TestTenantLogic/3node-tenant/system -v --rewrite ---- bazel info workspace --color=no -bazel test pkg/ccl/logictestccl:all --nocache_test_results --test_env=GOTRACEBACK=all --test_env=COCKROACH_WORKSPACE=crdb-checkout --test_arg -rewrite --sandbox_writable_path=crdb-checkout/pkg/ccl/logictestccl --sandbox_writable_path=crdb-checkout/pkg/sql/logictest --test_filter=TestTenantLogic/3node-tenant/system --test_arg -test.v --test_sharding_strategy=disabled --test_output all +bazel test pkg/ccl/logictestccl:all --nocache_test_results --test_env=GOTRACEBACK=all --test_env=COCKROACH_WORKSPACE=crdb-checkout --test_arg -rewrite --sandbox_writable_path=crdb-checkout/pkg/ccl/logictestccl --sandbox_writable_path=crdb-checkout/pkg/sql/logictest --sandbox_writable_path=crdb-checkout/pkg/sql/opt/exec/execbuilder --test_filter=TestTenantLogic/3node-tenant/system --test_arg -test.v --test_sharding_strategy=disabled --test_output all exec dev test pkg/spanconfig/spanconfigkvsubscriber -f=TestDecodeSpanTargets -v --stream-output From ee0fa072bbc60969fedb5bbad455d4803a399045 Mon Sep 17 00:00:00 2001 From: Jayant Shrivastava Date: Wed, 16 Nov 2022 17:05:42 -0500 Subject: [PATCH 4/6] roachtest/cdc: export stats for initial scan test to roachperf This change updates the cdc/initial_scan_only test to produce a `stats.json` artifact to be consumed by roachprod. This file contains stats for p99 foreground latency, changefeed throughput, and CPU usage. Release note: None --- pkg/cmd/roachtest/clusterstats/exporter.go | 4 ++ pkg/cmd/roachtest/tests/BUILD.bazel | 1 + pkg/cmd/roachtest/tests/cdc.go | 46 +++++++++++++++++++++- pkg/cmd/roachtest/tests/cdc_stats.go | 42 ++++++++++++++++++++ 4 files changed, 91 insertions(+), 2 deletions(-) create mode 100644 pkg/cmd/roachtest/tests/cdc_stats.go diff --git a/pkg/cmd/roachtest/clusterstats/exporter.go b/pkg/cmd/roachtest/clusterstats/exporter.go index 15fb7be1ba97..756c54ac13c6 100644 --- a/pkg/cmd/roachtest/clusterstats/exporter.go +++ b/pkg/cmd/roachtest/clusterstats/exporter.go @@ -48,6 +48,7 @@ type AggQuery struct { Query string AggFn AggregateFn Interval Interval + Tag string } // StatExporter defines an interface to export statistics to roachperf. @@ -278,6 +279,9 @@ func (cs *clusterStatCollector) getStatSummary( } ret.AggTag = summaryQuery.Query + if summaryQuery.Tag != "" { + ret.AggTag = summaryQuery.Tag + } // If there is more than one label name associated with the summary, we // cannot be sure which is the correct label. if len(taggedSummarySeries) != 1 { diff --git a/pkg/cmd/roachtest/tests/BUILD.bazel b/pkg/cmd/roachtest/tests/BUILD.bazel index 73f5dd821520..2b4b65ccf738 100644 --- a/pkg/cmd/roachtest/tests/BUILD.bazel +++ b/pkg/cmd/roachtest/tests/BUILD.bazel @@ -26,6 +26,7 @@ go_library( "canary.go", "cancel.go", "cdc.go", + "cdc_stats.go", "chaos.go", "clearrange.go", "cli.go", diff --git a/pkg/cmd/roachtest/tests/cdc.go b/pkg/cmd/roachtest/tests/cdc.go index 0a37a0e3fbed..0ee575dac861 100644 --- a/pkg/cmd/roachtest/tests/cdc.go +++ b/pkg/cmd/roachtest/tests/cdc.go @@ -38,6 +38,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/ccl/changefeedccl/cdctest" "github.com/cockroachdb/cockroach/pkg/ccl/changefeedccl/changefeedbase" "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/cluster" + "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/clusterstats" "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/option" "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry" "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/spec" @@ -77,6 +78,7 @@ type cdcTester struct { crdbNodes option.NodeListOption workloadNode option.NodeListOption logger *logger.Logger + promCfg *prometheus.Config // sinkType -> sinkURI sinkCache map[sinkType]string @@ -85,6 +87,43 @@ type cdcTester struct { doneCh chan struct{} } +// startStatsCollection sets the start point of the stats collection window +// and returns a function which should be called at the end of the test to dump a +// stats.json file to the artifacts directory. +func (ct *cdcTester) startStatsCollection() func() { + if ct.promCfg == nil { + ct.t.Error("prometheus configuration is nil") + } + promClient, err := clusterstats.SetupCollectorPromClient(ct.ctx, ct.cluster, ct.t.L(), ct.promCfg) + if err != nil { + ct.t.Errorf("error creating prometheus client for stats collector: %s", err) + } + + statsCollector := clusterstats.NewStatsCollector(ct.ctx, promClient) + startTime := timeutil.Now() + return func() { + endTime := timeutil.Now() + err := statsCollector.Exporter().Export(ct.ctx, ct.cluster, ct.t, + startTime, + endTime, + []clusterstats.AggQuery{sqlServiceLatencyAgg, changefeedThroughputAgg, cpuUsageAgg}, + func(stats map[string]clusterstats.StatSummary) (string, float64) { + // TODO(jayant): update this metric to be more accurate. + // It may be worth plugging in real latency values from the latency + // verifier here in the future for more accuracy. However, it may not be + // worth the added complexity. Since latency verifier failures will show + // up as roachtest failures, we don't need to make them very apparent in + // roachperf. Note that other roachperf stats, such as the aggregate stats + // above, will be accurate. + return "Total Run Time (mins)", endTime.Sub(startTime).Minutes() + }, + ) + if err != nil { + ct.t.Errorf("error exporting stats file: %s", err) + } + } +} + func (ct *cdcTester) startCRDBChaos() { chaosStopper := make(chan time.Time) ct.mon.Go(func(ctx context.Context) error { @@ -468,18 +507,19 @@ func newCDCTester(ctx context.Context, t test.Test, c cluster.Cluster) cdcTester if !t.SkipInit() { tester.startGrafana() } - return tester } func (ct *cdcTester) startGrafana() { - // Setup the prometheus instance on the workload node cfg := (&prometheus.Config{}). WithPrometheusNode(ct.workloadNode.InstallNodes()[0]). WithCluster(ct.crdbNodes.InstallNodes()). WithNodeExporter(ct.crdbNodes.InstallNodes()). WithGrafanaDashboard("https://go.crdb.dev/p/changefeed-roachtest-grafana-dashboard") cfg.Grafana.Enabled = true + + ct.promCfg = cfg + err := ct.cluster.StartGrafana(ct.ctx, ct.t.L(), cfg) if err != nil { ct.t.Errorf("error starting prometheus/grafana: %s", err) @@ -846,6 +886,7 @@ func registerCDC(r registry.Registry) { ct.runTPCCWorkload(tpccArgs{warehouses: 100}) + exportStatsFile := ct.startStatsCollection() feed := ct.newChangefeed(feedArgs{ sinkType: kafkaSink, targets: allTpccTargets, @@ -855,6 +896,7 @@ func registerCDC(r registry.Registry) { initialScanLatency: 30 * time.Minute, }) feed.waitForCompletion() + exportStatsFile() }, }) r.Add(registry.TestSpec{ diff --git a/pkg/cmd/roachtest/tests/cdc_stats.go b/pkg/cmd/roachtest/tests/cdc_stats.go new file mode 100644 index 000000000000..107f62f64df1 --- /dev/null +++ b/pkg/cmd/roachtest/tests/cdc_stats.go @@ -0,0 +1,42 @@ +// Copyright 2022 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package tests + +import "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/clusterstats" + +var ( + // sqlServiceLatency is the sql_service_latency_bucket prometheus metric. + sqlServiceLatency = clusterstats.ClusterStat{LabelName: "node", Query: "sql_service_latency_bucket"} + // sqlServiceLatencyAgg is the P99 latency of foreground SQL traffic across all nodes measured in ms. + sqlServiceLatencyAgg = clusterstats.AggQuery{ + Stat: sqlServiceLatency, + Query: "histogram_quantile(0.99, sum by(le) (rate(sql_service_latency_bucket[2m]))) / (1000*1000)", + Tag: "P99 Foreground Latency (ms)", + } + + // changefeedThroughput is the changefeed_emitted_bytes prometheus metric. + changefeedThroughput = clusterstats.ClusterStat{LabelName: "node", Query: "changefeed_emitted_bytes"} + // changefeedThroughputAgg is the total rate of bytes being emitted by a cluster measured in MB/s. + changefeedThroughputAgg = clusterstats.AggQuery{ + Stat: changefeedThroughput, + Query: "sum(rate(changefeed_emitted_bytes[1m]) / (1000 * 1000))", + Tag: "Throughput (MBps)", + } + + // cpuUsage is the sys_cpu_combined_percent_normalized prometheus metric per mode. + cpuUsage = clusterstats.ClusterStat{LabelName: "node", Query: "sys_cpu_combined_percent_normalized"} + // cpuUsageAgg is the average CPU usage across all nodes. + cpuUsageAgg = clusterstats.AggQuery{ + Stat: cpuUsage, + Query: "avg(sys_cpu_combined_percent_normalized) * 100", + Tag: "CPU Utilization (%)", + } +) From c0a9d9cbb845b3b741b88092dd76730306b0a476 Mon Sep 17 00:00:00 2001 From: Andy Yang Date: Wed, 30 Nov 2022 13:52:00 -0500 Subject: [PATCH 5/6] streamclient: replace usage of deprecated ioutil.ReadFile function This patch fixes a lint error resulting from a usage of the deprecated ioutil.ReadFile function. Release note: None --- .../streamclient/partitioned_stream_client_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/ccl/streamingccl/streamclient/partitioned_stream_client_test.go b/pkg/ccl/streamingccl/streamclient/partitioned_stream_client_test.go index 597246675182..e074b1e8e21f 100644 --- a/pkg/ccl/streamingccl/streamclient/partitioned_stream_client_test.go +++ b/pkg/ccl/streamingccl/streamclient/partitioned_stream_client_test.go @@ -11,8 +11,8 @@ package streamclient import ( "context" "fmt" - "io/ioutil" "net/url" + "os" "strings" "testing" "time" @@ -107,7 +107,7 @@ INSERT INTO d.t2 VALUES (2); v := ret.Query() for _, opt := range []string{"sslcert", "sslkey", "sslrootcert"} { path := v.Get(opt) - content, err := ioutil.ReadFile(path) + content, err := os.ReadFile(path) require.NoError(t, err) v.Set(opt, string(content)) From f1210ea2e5eeb7b3deabd01269554ebc7730dc30 Mon Sep 17 00:00:00 2001 From: Andrew Werner Date: Wed, 30 Nov 2022 14:21:05 -0500 Subject: [PATCH 6/6] jobsprotectedtsccl: unskip TestJobsProtectedTimestamp It was fixed by #92692. Fixes #91865. Release note: None --- pkg/ccl/jobsccl/jobsprotectedtsccl/BUILD.bazel | 1 - pkg/ccl/jobsccl/jobsprotectedtsccl/jobs_protected_ts_test.go | 3 --- 2 files changed, 4 deletions(-) diff --git a/pkg/ccl/jobsccl/jobsprotectedtsccl/BUILD.bazel b/pkg/ccl/jobsccl/jobsprotectedtsccl/BUILD.bazel index 15621beb4e6c..d2dc0c69cf32 100644 --- a/pkg/ccl/jobsccl/jobsprotectedtsccl/BUILD.bazel +++ b/pkg/ccl/jobsccl/jobsprotectedtsccl/BUILD.bazel @@ -29,7 +29,6 @@ go_test( "//pkg/sql/catalog/descpb", "//pkg/testutils", "//pkg/testutils/serverutils", - "//pkg/testutils/skip", "//pkg/testutils/sqlutils", "//pkg/testutils/testcluster", "//pkg/util/hlc", diff --git a/pkg/ccl/jobsccl/jobsprotectedtsccl/jobs_protected_ts_test.go b/pkg/ccl/jobsccl/jobsprotectedtsccl/jobs_protected_ts_test.go index 8dd2ad96af7e..6ef2fdce55ff 100644 --- a/pkg/ccl/jobsccl/jobsprotectedtsccl/jobs_protected_ts_test.go +++ b/pkg/ccl/jobsccl/jobsprotectedtsccl/jobs_protected_ts_test.go @@ -29,7 +29,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/testutils" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" - "github.com/cockroachdb/cockroach/pkg/testutils/skip" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" "github.com/cockroachdb/cockroach/pkg/testutils/testcluster" "github.com/cockroachdb/cockroach/pkg/util/hlc" @@ -133,8 +132,6 @@ WHERE func TestJobsProtectedTimestamp(t *testing.T) { defer leaktest.AfterTest(t)() - skip.WithIssue(t, 91865) // flaky test - ctx := context.Background() tc := testcluster.StartTestCluster(t, 1, base.TestClusterArgs{ ServerArgs: base.TestServerArgs{