server: refactor database index recommendations for DatabaseDetails API

Resolves: #93909

Previously, the DatabaseDetails API would fetch the index
recommendations of the database by executing an expensive query for
**each table of the database**, then coalesce the results of each table
to get the database-level result. This was needlessly expensive and
impractical, particularly so for large schemas. This change ensures that
only a single query is executed **per database** to fetch its index
recommendations.

Release note (performance improvement): Refactored the query logic when
fetching database index recommendations for the DatabaseDetails API
endpoint, greatly reducing the query time and cost, particularly for
large schemas.

pkg/server/admin.go

@@ -642,12 +642,12 @@ func (s *adminServer) databaseDetailsHelper(
 		if err != nil {
 			return nil, err
 		}
-		resp.Stats.NumIndexRecommendations, err = s.getNumDatabaseIndexRecommendations(ctx, req.Database, resp.TableNames)
+		dbIndexRecommendations, err := getDatabaseIndexRecommendations(ctx, req.Database, s.ie, s.st, s.sqlServer.execCfg)
 		if err != nil {
 			return nil, err
 		}
+		resp.Stats.NumIndexRecommendations = int32(len(dbIndexRecommendations))
 	}
-
 	return &resp, nil
 }
 
@@ -760,25 +760,6 @@ func (s *adminServer) getDatabaseStats(
 	return &stats, nil
 }
 
-func (s *adminServer) getNumDatabaseIndexRecommendations(
-	ctx context.Context, databaseName string, tableNames []string,
-) (int32, error) {
-	var numDatabaseIndexRecommendations int
-	idxUsageStatsProvider := s.sqlServer.pgServer.SQLServer.GetLocalIndexStatistics()
-	for _, tableName := range tableNames {
-		tableIndexStatsRequest := &serverpb.TableIndexStatsRequest{
-			Database: databaseName,
-			Table:    tableName,
-		}
-		tableIndexStatsResponse, err := getTableIndexUsageStats(ctx, tableIndexStatsRequest, idxUsageStatsProvider, s.ie, s.st, s.sqlServer.execCfg)
-		if err != nil {
-			return 0, err
-		}
-		numDatabaseIndexRecommendations += len(tableIndexStatsResponse.IndexRecommendations)
-	}
-	return int32(numDatabaseIndexRecommendations), nil
-}
-
 // getFullyQualifiedTableName, given a database name and a tableName that either
 // is a unqualified name or a schema-qualified name, returns a maximally
 // qualified name: either database.table if the input wasn't schema qualified,

pkg/server/index_usage_stats.go

@@ -12,6 +12,7 @@ package server
 
 import (
 	"context"
+	"fmt"
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
@@ -20,6 +21,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/idxusage"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
@@ -256,9 +258,6 @@ func getTableIndexUsageStats(
  		WHERE ti.descriptor_id = $::REGCLASS`,
 		tableID,
 	)
-
-	const expectedNumDatums = 7
-
 	it, err := ie.QueryIteratorEx(ctx, "index-usage-stats", nil,
 		sessiondata.InternalExecutorOverride{
 			User:     userName,
@@ -278,14 +277,7 @@ func getTableIndexUsageStats(
 	defer func() { err = errors.CombineErrors(err, it.Close()) }()
 
 	for ok, err = it.Next(ctx); ok; ok, err = it.Next(ctx) {
-		var row tree.Datums
-		if row = it.Cur(); row == nil {
-			return nil, errors.New("unexpected null row")
-		}
-
-		if row.Len() != expectedNumDatums {
-			return nil, errors.Newf("expected %d columns, received %d", expectedNumDatums, row.Len())
-		}
+		row := it.Cur()
 
 		indexID := tree.MustBeDInt(row[0])
 		indexName := tree.MustBeDString(row[1])
@@ -324,7 +316,11 @@ func getTableIndexUsageStats(
 		}
 
 		statsRow := idxusage.IndexStatsRow{
-			Row:              idxStatsRow,
+			TableID:          idxStatsRow.Statistics.Key.TableID,
+			IndexID:          idxStatsRow.Statistics.Key.IndexID,
+			CreatedAt:        idxStatsRow.CreatedAt,
+			LastRead:         idxStatsRow.Statistics.Stats.LastRead,
+			IndexType:        idxStatsRow.IndexType,
 			UnusedIndexKnobs: execConfig.UnusedIndexRecommendationsKnobs,
 		}
 		recommendations := statsRow.GetRecommendationsFromIndexStats(req.Database, st)
@@ -373,3 +369,83 @@ func getTableIDFromDatabaseAndTableName(
 	tableID := tree.MustBeDOid(row[0]).Oid
 	return int(tableID), nil
 }
+
+func getDatabaseIndexRecommendations(
+	ctx context.Context,
+	dbName string,
+	ie *sql.InternalExecutor,
+	st *cluster.Settings,
+	execConfig *sql.ExecutorConfig,
+) ([]*serverpb.IndexRecommendation, error) {
+
+	// Omit fetching index recommendations for the 'system' database.
+	if dbName == catconstants.SystemDatabaseName {
+		return []*serverpb.IndexRecommendation{}, nil
+	}
+
+	userName, err := userFromContext(ctx)
+	if err != nil {
+		return []*serverpb.IndexRecommendation{}, err
+	}
+
+	query := fmt.Sprintf(`
+		SELECT
+			ti.descriptor_id as table_id,
+			ti.index_id,
+			ti.index_type,
+			last_read,
+			ti.created_at
+		FROM %[1]s.crdb_internal.index_usage_statistics AS us
+		 JOIN %[1]s.crdb_internal.table_indexes AS ti ON (us.index_id = ti.index_id AND us.table_id = ti.descriptor_id AND index_type = 'secondary')
+		 JOIN %[1]s.crdb_internal.tables AS t ON (ti.descriptor_id = t.table_id AND t.database_name != 'system');`, dbName)
+
+	it, err := ie.QueryIteratorEx(ctx, "db-index-recommendations", nil,
+		sessiondata.InternalExecutorOverride{
+			User:     userName,
+			Database: dbName,
+		}, query)
+
+	if err != nil {
+		return []*serverpb.IndexRecommendation{}, err
+	}
+
+	// We have to make sure to close the iterator since we might return from the
+	// for loop early (before Next() returns false).
+	defer func() { err = errors.CombineErrors(err, it.Close()) }()
+
+	var ok bool
+	var idxRecommendations []*serverpb.IndexRecommendation
+
+	for ok, err = it.Next(ctx); ok; ok, err = it.Next(ctx) {
+		row := it.Cur()
+
+		tableID := tree.MustBeDInt(row[0])
+		indexID := tree.MustBeDInt(row[1])
+		indexType := tree.MustBeDString(row[2])
+		lastRead := time.Time{}
+		if row[3] != tree.DNull {
+			lastRead = tree.MustBeDTimestampTZ(row[3]).Time
+		}
+		var createdAt *time.Time
+		if row[4] != tree.DNull {
+			ts := tree.MustBeDTimestamp(row[4])
+			createdAt = &ts.Time
+		}
+
+		if err != nil {
+			return []*serverpb.IndexRecommendation{}, err
+		}
+
+		statsRow := idxusage.IndexStatsRow{
+			TableID:          roachpb.TableID(tableID),
+			IndexID:          roachpb.IndexID(indexID),
+			CreatedAt:        createdAt,
+			LastRead:         lastRead,
+			IndexType:        string(indexType),
+			UnusedIndexKnobs: execConfig.UnusedIndexRecommendationsKnobs,
+		}
+		recommendations := statsRow.GetRecommendationsFromIndexStats(dbName, st)
+		idxRecommendations = append(idxRecommendations, recommendations...)
+	}
+	return idxRecommendations, nil
+}

pkg/sql/idxusage/BUILD.bazel

@@ -16,6 +16,7 @@ go_library(
         "//pkg/server/serverpb",
         "//pkg/settings",
         "//pkg/settings/cluster",
+        "//pkg/sql/sem/catconstants",
         "//pkg/util/syncutil",
         "//pkg/util/timeutil",
         "@com_github_cockroachdb_errors//:errors",

pkg/sql/idxusage/index_usage_stats_rec.go

@@ -15,9 +15,11 @@ import (
 	"math"
 	"time"
 
+	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/server/serverpb"
 	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 )
 
@@ -26,7 +28,11 @@ import (
 // implements additional methods to support unused index recommendations and
 // hold testing knobs.
 type IndexStatsRow struct {
-	Row              *serverpb.TableIndexStatsResponse_ExtendedCollectedIndexUsageStatistics
+	TableID          roachpb.TableID
+	IndexID          roachpb.IndexID
+	CreatedAt        *time.Time
+	LastRead         time.Time
+	IndexType        string
 	UnusedIndexKnobs *UnusedIndexRecommendationTestingKnobs
 }
 
@@ -65,7 +71,8 @@ func (i IndexStatsRow) GetRecommendationsFromIndexStats(
 	dbName string, st *cluster.Settings,
 ) []*serverpb.IndexRecommendation {
 	var recommendations = []*serverpb.IndexRecommendation{}
-	if dbName == "system" || i.Row.IndexType == "primary" {
+	// Omit fetching index recommendations for the 'system' database.
+	if dbName == catconstants.SystemDatabaseName {
 		return recommendations
 	}
 	rec := i.maybeAddUnusedIndexRecommendation(DropUnusedIndexDuration.Get(&st.SV))
@@ -78,11 +85,15 @@ func (i IndexStatsRow) GetRecommendationsFromIndexStats(
 func (i IndexStatsRow) maybeAddUnusedIndexRecommendation(
 	unusedIndexDuration time.Duration,
 ) *serverpb.IndexRecommendation {
+	if i.IndexType == "primary" {
+		return nil
+	}
+
 	var rec *serverpb.IndexRecommendation
 
 	if i.UnusedIndexKnobs == nil {
-		rec = i.recommendDropUnusedIndex(timeutil.Now(), i.Row.CreatedAt,
-			i.Row.Statistics.Stats.LastRead, unusedIndexDuration)
+		rec = i.recommendDropUnusedIndex(timeutil.Now(), i.CreatedAt,
+			i.LastRead, unusedIndexDuration)
 	} else {
 		rec = i.recommendDropUnusedIndex(i.UnusedIndexKnobs.GetCurrentTime(),
 			i.UnusedIndexKnobs.GetCreatedAt(), i.UnusedIndexKnobs.GetLastRead(), unusedIndexDuration)
@@ -106,17 +117,17 @@ func (i IndexStatsRow) recommendDropUnusedIndex(
 	// dropping with a "never used" reason.
 	if lastActive.Equal(time.Time{}) {
 		return &serverpb.IndexRecommendation{
-			TableID: i.Row.Statistics.Key.TableID,
-			IndexID: i.Row.Statistics.Key.IndexID,
+			TableID: i.TableID,
+			IndexID: i.IndexID,
 			Type:    serverpb.IndexRecommendation_DROP_UNUSED,
 			Reason:  indexNeverUsedReason,
 		}
 	}
 	// Last usage of the index exceeds the unused index duration.
 	if currentTime.Sub(lastActive) >= unusedIndexDuration {
 		return &serverpb.IndexRecommendation{
-			TableID: i.Row.Statistics.Key.TableID,
-			IndexID: i.Row.Statistics.Key.IndexID,
+			TableID: i.TableID,
+			IndexID: i.IndexID,
 			Type:    serverpb.IndexRecommendation_DROP_UNUSED,
 			Reason:  fmt.Sprintf(indexExceedUsageDurationReasonPlaceholder, formatDuration(unusedIndexDuration)),
 		}