Merge #104270

104270: server, ui: don't recommend drop index to unique indexes r=maryliag a=maryliag Previously, when an index was not being used we were recommending it to be dropped, and only if the index was primary, we would not recommend. We should not be recommending to drop unique indexes, because even when they are not used for read, they're important for inserts and being dropped would change the schema of the table. Part Of #104143 https://www.loom.com/share/b4e7fae3594944d09ec9af7e53bed7be Release note (ui change): No longer show drop index recommendation for unique indexes. Co-authored-by: maryliag <[email protected]>
cockroachdb · Jun 2, 2023 · b250d9c · b250d9c
2 parents 4ac97a8 + f679234
commit b250d9c
Show file tree

Hide file tree

Showing 8 changed files with 41 additions and 18 deletions.
diff --git a/docs/generated/http/full.md b/docs/generated/http/full.md
@@ -4878,7 +4878,7 @@ Response object returned by TableIndexStatsResponse.
 | index_name | [string](#cockroach.server.serverpb.TableIndexStatsResponse-string) |  | index_name is the name of the index. | [reserved](#support-status) |
 | index_type | [string](#cockroach.server.serverpb.TableIndexStatsResponse-string) |  | index_type is the type of the index i.e. primary, secondary. | [reserved](#support-status) |
 | create_statement | [string](#cockroach.server.serverpb.TableIndexStatsResponse-string) |  | create_statement is the SQL statement that would re-create the current index if executed. | [reserved](#support-status) |
-| created_at | [google.protobuf.Timestamp](#cockroach.server.serverpb.TableIndexStatsResponse-google.protobuf.Timestamp) |  | CreatedAt is an approximate timestamp at which the index was created. Note that it may not always be populated. | [reserved](#support-status) |
+| created_at | [google.protobuf.Timestamp](#cockroach.server.serverpb.TableIndexStatsResponse-google.protobuf.Timestamp) |  | created_at is an approximate timestamp at which the index was created. Note that it may not always be populated. | [reserved](#support-status) |
 | index_id | [string](#cockroach.server.serverpb.TableIndexStatsResponse-string) |  | index_id is the ID of the index. | [reserved](#support-status) |
 | table_id | [string](#cockroach.server.serverpb.TableIndexStatsResponse-string) |  | table_id is the ID of the table which the index belongs to. | [reserved](#support-status) |
 

diff --git a/pkg/server/index_usage_stats.go b/pkg/server/index_usage_stats.go
@@ -249,7 +249,8 @@ func getTableIndexUsageStats(
 			total_reads,
 			last_read,
 			indexdef,
-			ti.created_at
+			ti.created_at,
+			ti.is_unique
 		FROM crdb_internal.index_usage_statistics AS us
   	JOIN crdb_internal.table_indexes AS ti ON us.index_id = ti.index_id 
 		AND us.table_id = ti.descriptor_id
@@ -294,6 +295,7 @@ func getTableIndexUsageStats(
 			ts := tree.MustBeDTimestamp(row[6])
 			createdAt = &ts.Time
 		}
+		isUnique := tree.MustBeDBool(row[7])
 
 		if err != nil {
 			return nil, err
@@ -322,6 +324,7 @@ func getTableIndexUsageStats(
 			CreatedAt:        idxStatsRow.CreatedAt,
 			LastRead:         idxStatsRow.Statistics.Stats.LastRead,
 			IndexType:        idxStatsRow.IndexType,
+			IsUnique:         bool(isUnique),
 			UnusedIndexKnobs: execConfig.UnusedIndexRecommendationsKnobs,
 		}
 		recommendations := statsRow.GetRecommendationsFromIndexStats(req.Database, st)
@@ -396,7 +399,8 @@ func getDatabaseIndexRecommendations(
 			ti.index_id,
 			ti.index_type,
 			last_read,
-			ti.created_at
+			ti.created_at,
+			ti.is_unique
 		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');`, escDBName)
@@ -433,6 +437,7 @@ func getDatabaseIndexRecommendations(
 			ts := tree.MustBeDTimestamp(row[4])
 			createdAt = &ts.Time
 		}
+		isUnique := tree.MustBeDBool(row[5])
 
 		if err != nil {
 			return []*serverpb.IndexRecommendation{}, err
@@ -444,6 +449,7 @@ func getDatabaseIndexRecommendations(
 			CreatedAt:        createdAt,
 			LastRead:         lastRead,
 			IndexType:        string(indexType),
+			IsUnique:         bool(isUnique),
 			UnusedIndexKnobs: knobs,
 		}
 		recommendations := statsRow.GetRecommendationsFromIndexStats(dbName, st)

diff --git a/pkg/server/serverpb/status.proto b/pkg/server/serverpb/status.proto
@@ -1899,7 +1899,7 @@ message TableIndexStatsResponse {
     // create_statement is the SQL statement that would re-create the
     // current index if executed.
     string create_statement = 4;
-    // CreatedAt is an approximate timestamp at which the index was created.
+    // created_at is an approximate timestamp at which the index was created.
     // Note that it may not always be populated.
     google.protobuf.Timestamp created_at = 5 [(gogoproto.stdtime) = true];
     // index_id is the ID of the index.

diff --git a/pkg/sql/idxusage/index_usage_stats_rec.go b/pkg/sql/idxusage/index_usage_stats_rec.go
@@ -33,6 +33,7 @@ type IndexStatsRow struct {
 	CreatedAt        *time.Time
 	LastRead         time.Time
 	IndexType        string
+	IsUnique         bool
 	UnusedIndexKnobs *UnusedIndexRecommendationTestingKnobs
 }
 
@@ -85,7 +86,7 @@ func (i IndexStatsRow) GetRecommendationsFromIndexStats(
 func (i IndexStatsRow) maybeAddUnusedIndexRecommendation(
 	unusedIndexDuration time.Duration,
 ) *serverpb.IndexRecommendation {
-	if i.IndexType == "primary" {
+	if i.IsUnique {
 		return nil
 	}
 

diff --git a/pkg/sql/idxusage/index_usage_stats_rec_test.go b/pkg/sql/idxusage/index_usage_stats_rec_test.go
@@ -35,6 +35,7 @@ func TestGetRecommendationsFromIndexStats(t *testing.T) {
 			- drop unused rec
 			- drop never used rec
 			- recently used index
+			- secondary unique index
 	*/
 
 	testData := []struct {
@@ -50,6 +51,7 @@ func TestGetRecommendationsFromIndexStats(t *testing.T) {
 				IndexID:          2,
 				LastRead:         anHourBefore,
 				IndexType:        "secondary",
+				IsUnique:         false,
 				CreatedAt:        nil,
 				UnusedIndexKnobs: nil,
 			},
@@ -66,6 +68,7 @@ func TestGetRecommendationsFromIndexStats(t *testing.T) {
 				CreatedAt:        nil,
 				UnusedIndexKnobs: nil,
 				IndexType:        "primary",
+				IsUnique:         true,
 			},
 			dbName:              "testdb",
 			unusedIndexDuration: time.Hour,
@@ -78,6 +81,7 @@ func TestGetRecommendationsFromIndexStats(t *testing.T) {
 				IndexID:          2,
 				LastRead:         anHourBefore,
 				IndexType:        "secondary",
+				IsUnique:         false,
 				CreatedAt:        nil,
 				UnusedIndexKnobs: nil,
 			},
@@ -95,11 +99,11 @@ func TestGetRecommendationsFromIndexStats(t *testing.T) {
 		// Index has never been used and has no creation time, expect never used index recommendation.
 		{
 			idxStats: IndexStatsRow{
-				TableID:  1,
-				IndexID:  3,
-				LastRead: time.Time{},
-
+				TableID:          1,
+				IndexID:          3,
+				LastRead:         time.Time{},
 				IndexType:        "secondary",
+				IsUnique:         false,
 				CreatedAt:        nil,
 				UnusedIndexKnobs: nil,
 			},
@@ -121,13 +125,29 @@ func TestGetRecommendationsFromIndexStats(t *testing.T) {
 				IndexID:          4,
 				LastRead:         aMinuteBefore,
 				IndexType:        "secondary",
+				IsUnique:         false,
 				CreatedAt:        nil,
 				UnusedIndexKnobs: nil,
 			},
 			dbName:              "testdb",
 			unusedIndexDuration: defaultUnusedIndexDuration,
 			expectedReturn:      []*serverpb.IndexRecommendation{},
 		},
+		// Index exceeds the unused index duration, but is unique, so expect no index recommendation.
+		{
+			idxStats: IndexStatsRow{
+				TableID:          1,
+				IndexID:          5,
+				LastRead:         anHourBefore,
+				IndexType:        "secondary",
+				IsUnique:         true,
+				CreatedAt:        nil,
+				UnusedIndexKnobs: nil,
+			},
+			dbName:              "testdb",
+			unusedIndexDuration: time.Hour,
+			expectedReturn:      []*serverpb.IndexRecommendation{},
+		},
 	}
 
 	st := cluster.MakeTestingClusterSettings()

diff --git a/pkg/ui/workspaces/cluster-ui/src/api/databaseDetailsApi.ts b/pkg/ui/workspaces/cluster-ui/src/api/databaseDetailsApi.ts
@@ -13,13 +13,10 @@ import {
   executeInternalSql,
   formatApiResult,
   isMaxSizeError,
-  LARGE_RESULT_SIZE,
-  LONG_TIMEOUT,
   SqlApiQueryResponse,
   SqlApiResponse,
   SqlExecutionErrorMessage,
   SqlExecutionRequest,
-  SqlExecutionResponse,
   SqlStatement,
   SqlTxnResult,
   txnResultIsEmpty,
@@ -423,9 +420,9 @@ const getDatabaseIndexUsageStats: DatabaseDetailsQuery<IndexUsageStatistic> = {
                   cs.value::interval as interval_threshold,
                   now() - COALESCE(us.last_read AT TIME ZONE 'UTC', COALESCE(ti.created_at, '0001-01-01')) as unused_interval
                   FROM %1.crdb_internal.index_usage_statistics AS us
-                  JOIN %1.crdb_internal.table_indexes AS ti ON (us.index_id = ti.index_id AND us.table_id = ti.descriptor_id AND ti.index_type = 'secondary')
+                  JOIN %1.crdb_internal.table_indexes AS ti ON (us.index_id = ti.index_id AND us.table_id = ti.descriptor_id)
                   CROSS JOIN cs
-                 WHERE $1 != 'system')
+                 WHERE $1 != 'system' AND ti.is_unique IS false)
                WHERE unused_interval > interval_threshold
                ORDER BY total_reads DESC;`,
         [new Identifier(dbName)],

diff --git a/pkg/ui/workspaces/cluster-ui/src/api/schemaInsightsApi.ts b/pkg/ui/workspaces/cluster-ui/src/api/schemaInsightsApi.ts
@@ -166,7 +166,7 @@ const dropUnusedIndexQuery: SchemaInsightQuery<ClusterIndexUsageStatistic> = {
                             JOIN "".crdb_internal.tables as t
                                  ON t.table_id = ti.descriptor_id and t.name = ti.descriptor_name
                             CROSS JOIN cs
-                   WHERE t.database_name != 'system' AND ti.index_type != 'primary')
+                   WHERE t.database_name != 'system' AND ti.is_unique IS false)
           WHERE unused_interval > interval_threshold
           ORDER BY total_reads DESC;`,
   toSchemaInsight: clusterIndexUsageStatsToSchemaInsight,

diff --git a/pkg/ui/workspaces/cluster-ui/src/api/tableDetailsApi.ts b/pkg/ui/workspaces/cluster-ui/src/api/tableDetailsApi.ts
@@ -494,11 +494,10 @@ const getTableIndexUsageStats: TableDetailsQuery<IndexUsageStatistic> = {
                   JOIN tableId ON us.table_id = tableId.table_id
                   JOIN %1.crdb_internal.table_indexes AS ti ON (
                       us.index_id = ti.index_id AND 
-                      tableId.table_id = ti.descriptor_id AND 
-                      ti.index_type = 'secondary' 
+                      tableId.table_id = ti.descriptor_id
                   )
                   CROSS JOIN cs
-                 WHERE $2 != 'system')
+                 WHERE $2 != 'system' AND ti.is_unique IS false)
                WHERE unused_interval > interval_threshold
                ORDER BY total_reads DESC`,
         [new Identifier(dbName)],