From 1a90a9b127a0bd2a87bedaf2c914f591762128f2 Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Thu, 10 Oct 2024 16:09:28 -0700 Subject: [PATCH] opt: refactor used indexes I noticed the unnecessary usage of strings to track index usage in some CPU/heap profiles. I attempted to eliminate this entirely, but unfortunately these strings are persisted, making modification of the type difficult, and I had difficulty following the thread from their creation all the way to their consumption - I think someone with more expertise in the observability part of the code base would be required to untangle this. So for now I've settled for refactoring the creation of "used indexes" to avoid using strings until later on during instrumentation. This has the minor benefit possible de-duplicating the indexes before generating the strings, if an index is used multiple times in a query. I've also replaced the expensive usage of `fmt.Sprintf` for generating the strings to the faster `+` concatenation (benchmarks for this are at: https://gist.github.com/mgartner/28a9d52e54e91691d0a7617164aa3157). Release note: None --- pkg/sql/exec_log.go | 102 +++++++++++---------- pkg/sql/executor_statement_metrics.go | 6 +- pkg/sql/instrumentation.go | 3 +- pkg/sql/opt/exec/execbuilder/BUILD.bazel | 1 - pkg/sql/opt/exec/execbuilder/builder.go | 38 +++++++- pkg/sql/opt/exec/execbuilder/relational.go | 15 ++- 6 files changed, 101 insertions(+), 64 deletions(-) diff --git a/pkg/sql/exec_log.go b/pkg/sql/exec_log.go index 1ecb31dd94ab..86f167465613 100644 --- a/pkg/sql/exec_log.go +++ b/pkg/sql/exec_log.go @@ -359,56 +359,58 @@ func (p *planner) maybeLogStatementInternal( defer releaseSampledQuery(sampledQuery) *sampledQuery = eventpb.SampledQuery{ - CommonSQLExecDetails: execDetails, - SkippedQueries: skippedQueries, - CostEstimate: p.curPlan.instrumentation.costEstimate, - Distribution: p.curPlan.instrumentation.distribution.String(), - PlanGist: p.curPlan.instrumentation.planGist.String(), - SessionID: p.extendedEvalCtx.SessionID.String(), - Database: p.CurrentDatabase(), - StatementID: p.stmt.QueryID.String(), - TransactionID: txnID, - StatementFingerprintID: stmtFingerprintID.String(), - MaxFullScanRowsEstimate: p.curPlan.instrumentation.maxFullScanRows, - TotalScanRowsEstimate: p.curPlan.instrumentation.totalScanRows, - OutputRowsEstimate: p.curPlan.instrumentation.outputRows, - StatsAvailable: p.curPlan.instrumentation.statsAvailable, - NanosSinceStatsCollected: int64(p.curPlan.instrumentation.nanosSinceStatsCollected), - BytesRead: p.curPlan.instrumentation.topLevelStats.bytesRead, - RowsRead: p.curPlan.instrumentation.topLevelStats.rowsRead, - RowsWritten: p.curPlan.instrumentation.topLevelStats.rowsWritten, - InnerJoinCount: int64(p.curPlan.instrumentation.joinTypeCounts[descpb.InnerJoin]), - LeftOuterJoinCount: int64(p.curPlan.instrumentation.joinTypeCounts[descpb.LeftOuterJoin]), - FullOuterJoinCount: int64(p.curPlan.instrumentation.joinTypeCounts[descpb.FullOuterJoin]), - SemiJoinCount: int64(p.curPlan.instrumentation.joinTypeCounts[descpb.LeftSemiJoin]), - AntiJoinCount: int64(p.curPlan.instrumentation.joinTypeCounts[descpb.LeftAntiJoin]), - IntersectAllJoinCount: int64(p.curPlan.instrumentation.joinTypeCounts[descpb.IntersectAllJoin]), - ExceptAllJoinCount: int64(p.curPlan.instrumentation.joinTypeCounts[descpb.ExceptAllJoin]), - HashJoinCount: int64(p.curPlan.instrumentation.joinAlgorithmCounts[exec.HashJoin]), - CrossJoinCount: int64(p.curPlan.instrumentation.joinAlgorithmCounts[exec.CrossJoin]), - IndexJoinCount: int64(p.curPlan.instrumentation.joinAlgorithmCounts[exec.IndexJoin]), - LookupJoinCount: int64(p.curPlan.instrumentation.joinAlgorithmCounts[exec.LookupJoin]), - MergeJoinCount: int64(p.curPlan.instrumentation.joinAlgorithmCounts[exec.MergeJoin]), - InvertedJoinCount: int64(p.curPlan.instrumentation.joinAlgorithmCounts[exec.InvertedJoin]), - ApplyJoinCount: int64(p.curPlan.instrumentation.joinAlgorithmCounts[exec.ApplyJoin]), - ZigZagJoinCount: int64(p.curPlan.instrumentation.joinAlgorithmCounts[exec.ZigZagJoin]), - ContentionNanos: queryLevelStats.ContentionTime.Nanoseconds(), - Regions: queryLevelStats.Regions, - SQLInstanceIDs: queryLevelStats.SQLInstanceIDs, - KVNodeIDs: queryLevelStats.KVNodeIDs, - UsedFollowerRead: queryLevelStats.UsedFollowerRead, - NetworkBytesSent: queryLevelStats.NetworkBytesSent, - MaxMemUsage: queryLevelStats.MaxMemUsage, - MaxDiskUsage: queryLevelStats.MaxDiskUsage, - KVBytesRead: queryLevelStats.KVBytesRead, - KVPairsRead: queryLevelStats.KVPairsRead, - KVRowsRead: queryLevelStats.KVRowsRead, - KvTimeNanos: queryLevelStats.KVTime.Nanoseconds(), - KvGrpcCalls: queryLevelStats.KVBatchRequestsIssued, - NetworkMessages: queryLevelStats.NetworkMessages, - CpuTimeNanos: queryLevelStats.CPUTime.Nanoseconds(), - IndexRecommendations: indexRecs, - Indexes: p.curPlan.instrumentation.indexesUsed, + CommonSQLExecDetails: execDetails, + SkippedQueries: skippedQueries, + CostEstimate: p.curPlan.instrumentation.costEstimate, + Distribution: p.curPlan.instrumentation.distribution.String(), + PlanGist: p.curPlan.instrumentation.planGist.String(), + SessionID: p.extendedEvalCtx.SessionID.String(), + Database: p.CurrentDatabase(), + StatementID: p.stmt.QueryID.String(), + TransactionID: txnID, + StatementFingerprintID: stmtFingerprintID.String(), + MaxFullScanRowsEstimate: p.curPlan.instrumentation.maxFullScanRows, + TotalScanRowsEstimate: p.curPlan.instrumentation.totalScanRows, + OutputRowsEstimate: p.curPlan.instrumentation.outputRows, + StatsAvailable: p.curPlan.instrumentation.statsAvailable, + NanosSinceStatsCollected: int64(p.curPlan.instrumentation.nanosSinceStatsCollected), + BytesRead: p.curPlan.instrumentation.topLevelStats.bytesRead, + RowsRead: p.curPlan.instrumentation.topLevelStats.rowsRead, + RowsWritten: p.curPlan.instrumentation.topLevelStats.rowsWritten, + InnerJoinCount: int64(p.curPlan.instrumentation.joinTypeCounts[descpb.InnerJoin]), + LeftOuterJoinCount: int64(p.curPlan.instrumentation.joinTypeCounts[descpb.LeftOuterJoin]), + FullOuterJoinCount: int64(p.curPlan.instrumentation.joinTypeCounts[descpb.FullOuterJoin]), + SemiJoinCount: int64(p.curPlan.instrumentation.joinTypeCounts[descpb.LeftSemiJoin]), + AntiJoinCount: int64(p.curPlan.instrumentation.joinTypeCounts[descpb.LeftAntiJoin]), + IntersectAllJoinCount: int64(p.curPlan.instrumentation.joinTypeCounts[descpb.IntersectAllJoin]), + ExceptAllJoinCount: int64(p.curPlan.instrumentation.joinTypeCounts[descpb.ExceptAllJoin]), + HashJoinCount: int64(p.curPlan.instrumentation.joinAlgorithmCounts[exec.HashJoin]), + CrossJoinCount: int64(p.curPlan.instrumentation.joinAlgorithmCounts[exec.CrossJoin]), + IndexJoinCount: int64(p.curPlan.instrumentation.joinAlgorithmCounts[exec.IndexJoin]), + LookupJoinCount: int64(p.curPlan.instrumentation.joinAlgorithmCounts[exec.LookupJoin]), + MergeJoinCount: int64(p.curPlan.instrumentation.joinAlgorithmCounts[exec.MergeJoin]), + InvertedJoinCount: int64(p.curPlan.instrumentation.joinAlgorithmCounts[exec.InvertedJoin]), + ApplyJoinCount: int64(p.curPlan.instrumentation.joinAlgorithmCounts[exec.ApplyJoin]), + ZigZagJoinCount: int64(p.curPlan.instrumentation.joinAlgorithmCounts[exec.ZigZagJoin]), + ContentionNanos: queryLevelStats.ContentionTime.Nanoseconds(), + Regions: queryLevelStats.Regions, + SQLInstanceIDs: queryLevelStats.SQLInstanceIDs, + KVNodeIDs: queryLevelStats.KVNodeIDs, + UsedFollowerRead: queryLevelStats.UsedFollowerRead, + NetworkBytesSent: queryLevelStats.NetworkBytesSent, + MaxMemUsage: queryLevelStats.MaxMemUsage, + MaxDiskUsage: queryLevelStats.MaxDiskUsage, + KVBytesRead: queryLevelStats.KVBytesRead, + KVPairsRead: queryLevelStats.KVPairsRead, + KVRowsRead: queryLevelStats.KVRowsRead, + KvTimeNanos: queryLevelStats.KVTime.Nanoseconds(), + KvGrpcCalls: queryLevelStats.KVBatchRequestsIssued, + NetworkMessages: queryLevelStats.NetworkMessages, + CpuTimeNanos: queryLevelStats.CPUTime.Nanoseconds(), + IndexRecommendations: indexRecs, + // TODO(mgartner): Use a slice of struct{uint64, uint64} instead of + // converting to strings. + Indexes: p.curPlan.instrumentation.indexesUsed.Strings(), ScanCount: int64(p.curPlan.instrumentation.scanCounts[exec.ScanCount]), ScanWithStatsCount: int64(p.curPlan.instrumentation.scanCounts[exec.ScanWithStatsCount]), ScanWithStatsForecastCount: int64(p.curPlan.instrumentation.scanCounts[exec.ScanWithStatsForecastCount]), diff --git a/pkg/sql/executor_statement_metrics.go b/pkg/sql/executor_statement_metrics.go index 5056ec6559be..d2d7d302ea81 100644 --- a/pkg/sql/executor_statement_metrics.go +++ b/pkg/sql/executor_statement_metrics.go @@ -208,8 +208,10 @@ func (ex *connExecutor) recordStatementSummary( EndTime: phaseTimes.GetSessionPhaseTime(sessionphase.PlannerStartExecStmt).Add(svcLatRaw), FullScan: fullScan, ExecStats: queryLevelStats, - Indexes: planner.instrumentation.indexesUsed, - Database: planner.SessionData().Database, + // TODO(mgartner): Use a slice of struct{uint64, uint64} instead of + // converting to strings. + Indexes: planner.instrumentation.indexesUsed.Strings(), + Database: planner.SessionData().Database, } stmtFingerprintID, err := diff --git a/pkg/sql/instrumentation.go b/pkg/sql/instrumentation.go index e6c76d858f1a..4f8eaca6af36 100644 --- a/pkg/sql/instrumentation.go +++ b/pkg/sql/instrumentation.go @@ -26,6 +26,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/idxrecommendations" "github.com/cockroachdb/cockroach/pkg/sql/isql" "github.com/cockroachdb/cockroach/pkg/sql/opt/exec" + "github.com/cockroachdb/cockroach/pkg/sql/opt/exec/execbuilder" "github.com/cockroachdb/cockroach/pkg/sql/opt/exec/explain" "github.com/cockroachdb/cockroach/pkg/sql/opt/indexrec" "github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder" @@ -225,7 +226,7 @@ type instrumentationHelper struct { scanCounts [exec.NumScanCountTypes]int // indexesUsed list the indexes used in the query with format tableID@indexID. - indexesUsed []string + indexesUsed execbuilder.IndexesUsed // schemachangerMode indicates which schema changer mode was used to execute // the query. diff --git a/pkg/sql/opt/exec/execbuilder/BUILD.bazel b/pkg/sql/opt/exec/execbuilder/BUILD.bazel index ad8b1921d6ed..b06cb525c606 100644 --- a/pkg/sql/opt/exec/execbuilder/BUILD.bazel +++ b/pkg/sql/opt/exec/execbuilder/BUILD.bazel @@ -47,7 +47,6 @@ go_library( "//pkg/sql/sqlerrors", "//pkg/sql/sqltelemetry", "//pkg/sql/types", - "//pkg/util", "//pkg/util/buildutil", "//pkg/util/encoding", "//pkg/util/errorutil", diff --git a/pkg/sql/opt/exec/execbuilder/builder.go b/pkg/sql/opt/exec/execbuilder/builder.go index 34830973e177..c529e9ea3f81 100644 --- a/pkg/sql/opt/exec/execbuilder/builder.go +++ b/pkg/sql/opt/exec/execbuilder/builder.go @@ -7,6 +7,8 @@ package execbuilder import ( "context" + "slices" + "strconv" "time" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" @@ -168,7 +170,41 @@ type Builder struct { IsANSIDML bool // IndexesUsed list the indexes used in query with the format tableID@indexID. - IndexesUsed []string + IndexesUsed +} + +// IndexesUsed is a list of indexes used in a query. +type IndexesUsed struct { + indexes []struct { + tableID cat.StableID + indexID cat.StableID + } +} + +// add adds the given index to the list, if it is not already present. +func (iu *IndexesUsed) add(tableID, indexID cat.StableID) { + s := struct { + tableID cat.StableID + indexID cat.StableID + }{tableID, indexID} + if !slices.Contains(iu.indexes, s) { + iu.indexes = append(iu.indexes, s) + } +} + +// Strings returns a slice of strings with the format tableID@indexID for each +// index in the list. +// +// TODO(mgartner): Use a slice of struct{uint64, uint64} instead of converting +// to strings. +func (iu *IndexesUsed) Strings() []string { + res := make([]string, len(iu.indexes)) + const base = 10 + for i, u := range iu.indexes { + res[i] = strconv.FormatUint(uint64(u.tableID), base) + "@" + + strconv.FormatUint(uint64(u.indexID), base) + } + return res } // New constructs an instance of the execution node builder using the diff --git a/pkg/sql/opt/exec/execbuilder/relational.go b/pkg/sql/opt/exec/execbuilder/relational.go index dd494134d8aa..af66e039ee9c 100644 --- a/pkg/sql/opt/exec/execbuilder/relational.go +++ b/pkg/sql/opt/exec/execbuilder/relational.go @@ -38,7 +38,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sqlerrors" "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" "github.com/cockroachdb/cockroach/pkg/sql/types" - "github.com/cockroachdb/cockroach/pkg/util" "github.com/cockroachdb/cockroach/pkg/util/buildutil" "github.com/cockroachdb/cockroach/pkg/util/encoding" "github.com/cockroachdb/cockroach/pkg/util/errorutil" @@ -753,7 +752,7 @@ func (b *Builder) buildScan(scan *memo.ScanExpr) (_ execPlan, outputCols colOrdM return execPlan{}, colOrdMap{}, errors.AssertionFailedf("expected inverted index scan to have a constraint") } - b.IndexesUsed = util.CombineUnique(b.IndexesUsed, []string{fmt.Sprintf("%d@%d", tab.ID(), idx.ID())}) + b.IndexesUsed.add(tab.ID(), idx.ID()) // Save if we planned a full (large) table/index scan on the builder so that // the planner can be made aware later. We only do this for non-virtual @@ -2294,7 +2293,7 @@ func (b *Builder) buildIndexJoin( // TODO(radu): the distsql implementation of index join assumes that the input // starts with the PK columns in order (#40749). pri := tab.Index(cat.PrimaryIndex) - b.IndexesUsed = util.CombineUnique(b.IndexesUsed, []string{fmt.Sprintf("%d@%d", tab.ID(), pri.ID())}) + b.IndexesUsed.add(tab.ID(), pri.ID()) keyCols := make([]exec.NodeColumnOrdinal, pri.KeyColumnCount()) for i := range keyCols { keyCols[i], err = getNodeColumnOrdinal(inputCols, join.Table.ColumnID(pri.Column(i).Ordinal())) @@ -2672,7 +2671,7 @@ func (b *Builder) buildLookupJoin( tab := md.Table(join.Table) idx := tab.Index(join.Index) - b.IndexesUsed = util.CombineUnique(b.IndexesUsed, []string{fmt.Sprintf("%d@%d", tab.ID(), idx.ID())}) + b.IndexesUsed.add(tab.ID(), idx.ID()) locking, err := b.buildLocking(join.Table, join.Locking) if err != nil { @@ -2852,7 +2851,7 @@ func (b *Builder) buildInvertedJoin( md := b.mem.Metadata() tab := md.Table(join.Table) idx := tab.Index(join.Index) - b.IndexesUsed = util.CombineUnique(b.IndexesUsed, []string{fmt.Sprintf("%d@%d", tab.ID(), idx.ID())}) + b.IndexesUsed.add(tab.ID(), idx.ID()) prefixEqCols := make([]exec.NodeColumnOrdinal, len(join.PrefixKeyCols)) for i, c := range join.PrefixKeyCols { @@ -2994,10 +2993,8 @@ func (b *Builder) buildZigzagJoin( rightTable := md.Table(join.RightTable) leftIndex := leftTable.Index(join.LeftIndex) rightIndex := rightTable.Index(join.RightIndex) - b.IndexesUsed = util.CombineUnique(b.IndexesUsed, - []string{fmt.Sprintf("%d@%d", leftTable.ID(), leftIndex.ID())}) - b.IndexesUsed = util.CombineUnique(b.IndexesUsed, - []string{fmt.Sprintf("%d@%d", rightTable.ID(), rightIndex.ID())}) + b.IndexesUsed.add(leftTable.ID(), leftIndex.ID()) + b.IndexesUsed.add(rightTable.ID(), rightIndex.ID()) leftEqCols := make([]exec.TableColumnOrdinal, len(join.LeftEqCols)) rightEqCols := make([]exec.TableColumnOrdinal, len(join.RightEqCols))