server: refactor database index recommendations for DatabaseDetails API #93937

THardy98 · 2022-12-19T20:41:39Z

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.

SHORT DEMOS
Short demos of the change in latency before/after running demo on db-console. Notably, movr is the only database that we check for index recommendations.

Before
https://www.loom.com/share/fc7ca49e4f9c46738831c23742112069

After
https://www.loom.com/share/be6e4711ca1d43409774995dece0673b

Noted Improvements:

The latency on fetching stats for movr improves from ~250ms to ~60ms
Not shown in the videos above but the number of query calls improves from 45 to 4.

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.

cockroach-teamcity · 2022-12-19T20:41:57Z

This change is

maryliag

Reviewed 1 of 4 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @THardy98)

pkg/server/index_usage_stats.go line 392 at r1 (raw file):

	// Omit fetching index recommendations on the default databases 'system',
	// 'defaultdb', and 'postgres'.
	if dbName == "system" || dbName == "defaultdb" || dbName == "postgres" {

maybe worth creating a list of "databases to ignore", this way you can use the constant in different places if necessary, and you don't need to create a check for any new ones, you would just add it to the list, and here check if the list includes the current one.

Also, I don't think defaultdb should be on the list, since that is (as the name say), the default, users can create their owns tables on it, and we do want to see recommendations for it.

pkg/sql/idxusage/index_usage_stats_rec.go line 75 at r1 (raw file):

	// Omit calculating index recommendations on the default databases 'system',
	// 'defaultdb', and 'postgres' and primary indexes.
	if dbName == "system" || dbName == "postgres" || dbName == "defaultdb" || i.IndexType == "primary" {

same comment as above (here you would use the same constant list)

pkg/sql/idxusage/index_usage_stats_rec_test.go line 74 at r1 (raw file):

			expectedReturn:      []*serverpb.IndexRecommendation{},
		},
		// Database name "defaultdb", expect no index recommendation.

recommendations here could indeed exist and be valid

THardy98

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @maryliag)

pkg/server/index_usage_stats.go line 392 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

maybe worth creating a list of "databases to ignore", this way you can use the constant in different places if necessary, and you don't need to create a check for any new ones, you would just add it to the list, and here check if the list includes the current one.

Also, I don't think defaultdb should be on the list, since that is (as the name say), the default, users can create their owns tables on it, and we do want to see recommendations for it.

Added IgnoredDatabases to the idxusage package.

pkg/sql/idxusage/index_usage_stats_rec.go line 75 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

same comment as above (here you would use the same constant list)

Done.

pkg/sql/idxusage/index_usage_stats_rec_test.go line 74 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

recommendations here could indeed exist and be valid

Removed this test case.

knz

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)

pkg/server/index_usage_stats.go line 392 at r1 (raw file):

Previously, THardy98 (Thomas Hardy) wrote…

Added IgnoredDatabases to the idxusage package.

I don't believe we should ignore any of them actually, other than perhaps system. What's the rationale here?

(Also for system you can use catconstants.SystemDatabaseName)

pkg/sql/idxusage/index_usage_stats_rec.go line 75 at r1 (raw file):

Previously, THardy98 (Thomas Hardy) wrote…

Done.

ditto

I'm also not sure why exclude the primary index.

THardy98

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)

pkg/server/index_usage_stats.go line 392 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

I don't believe we should ignore any of them actually, other than perhaps system. What's the rationale here?

(Also for system you can use catconstants.SystemDatabaseName)

These index recommendations are surfaced and can be actioned by users in the UI (as in, we allow users to drop indexes we recommend dropping). I don't think we would like users to be able to drop indexes that belong to our system or postgres databases (as that's our domain). Additionally, recommendations that are of interest/beneficial to the user would likely be index recommendations for databases they own.

pkg/sql/idxusage/index_usage_stats_rec.go line 75 at r1 (raw file):

These index recommendations are surfaced and can be actioned by users in the UI (as in, we allow users to drop indexes we recommend dropping). I don't think we would like users to be able to drop indexes that belong to our system or postgres databases (as that's our domain). Additionally, recommendations that are of interest/beneficial to the user would likely be index recommendations for databases they own.

We don't include the primary index for the same reason - not sure it's in the user's best interest to drop/alter the primary index.

knz

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag and @THardy98)

pkg/server/index_usage_stats.go line 392 at r1 (raw file):

Previously, THardy98 (Thomas Hardy) wrote…

These index recommendations are surfaced and can be actioned by users in the UI (as in, we allow users to drop indexes we recommend dropping). I don't think we would like users to be able to drop indexes that belong to our system or postgres databases (as that's our domain). Additionally, recommendations that are of interest/beneficial to the user would likely be index recommendations for databases they own.

postgres is equivalent for defaultdb. It's created for control by users. We introduced it because some tools (psql and some graphical UI db navigators) auto-default to a db named postgres.

Generally we've designed crdb such that only the system database is special. everything else is for the user's benefit.

pkg/server/index_usage_stats.go line 391 at r2 (raw file):

	// Omit fetching index recommendations on the default databases 'system' and 'postgres'.
	if _, ok := idxusage.IgnoredDatabases[dbName]; ok {

so, no; dbName != catconstants.SystemDatabaseName is sufficient here.

pkg/sql/idxusage/index_usage_stats_rec.go line 75 at r1 (raw file):

Previously, THardy98 (Thomas Hardy) wrote…

These index recommendations are surfaced and can be actioned by users in the UI (as in, we allow users to drop indexes we recommend dropping). I don't think we would like users to be able to drop indexes that belong to our system or postgres databases (as that's our domain). Additionally, recommendations that are of interest/beneficial to the user would likely be index recommendations for databases they own.

We don't include the primary index for the same reason - not sure it's in the user's best interest to drop/alter the primary index.

I fundamentally disagree. A primary index is like any other index wrt design and insights; if I create a SQL schema and i make a mistake in my choice of PK, i want my insights tool to tell me. A user can change the PK after the table is created.

THardy98

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @maryliag)

pkg/server/index_usage_stats.go line 392 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

postgres is equivalent for defaultdb. It's created for control by users. We introduced it because some tools (psql and some graphical UI db navigators) auto-default to a db named postgres.

Generally we've designed crdb such that only the system database is special. everything else is for the user's benefit.

Sounds good. I've changed to only omit the system database.

pkg/server/index_usage_stats.go line 391 at r2 (raw file):

Sounds good. I've changed to only omit the system database.
Done.

pkg/sql/idxusage/index_usage_stats_rec.go line 75 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

I fundamentally disagree. A primary index is like any other index wrt design and insights; if I create a SQL schema and i make a mistake in my choice of PK, i want my insights tool to tell me. A user can change the PK after the table is created.

Fair, but currently the index recommendations being populated here are exclusively DROP index recommendations which don't make sense in the context of a primary index (which is why I'm hesitant to remove this conditional).

I would agree more if the recommendation were to ALTER or REPLACE the primary index. However, CREATE, ALTER, and REPLACE index recommendations are offered on the Insights page, but haven't been tracked by this endpoint yet (which is to be addressed in a separate issue).

knz

Reviewed 1 of 3 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)

pkg/sql/idxusage/index_usage_stats_rec.go line 75 at r1 (raw file):

Previously, THardy98 (Thomas Hardy) wrote…

Fair, but currently the index recommendations being populated here are exclusively DROP index recommendations which don't make sense in the context of a primary index (which is why I'm hesitant to remove this conditional).

I would agree more if the recommendation was to ALTER the primary index. CREATE, ALTER, and REPLACE index recommendations are offered on the Insights page, but haven't been tracked by this endpoint yet (which is to be addressed).

What about you extend this conditional to only exclude recommendations that don't make sense, i.e. only those that are DROP on primary indexes and keep the others?

THardy98

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @maryliag)

pkg/sql/idxusage/index_usage_stats_rec.go line 75 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

What about you extend this conditional to only exclude recommendations that don't make sense, i.e. only those that are DROP on primary indexes and keep the others?

Moved the primary index check to maybeAddUnusedIndexRecommendation, which checks for DROP recommendations.

knz

Nice. No further comment from me. Please still get your review approval from your team.

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)

matthewtodd

Nice! Just a couple of questions from me.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @maryliag, and @THardy98)

pkg/server/index_usage_stats.go line 433 at r5 (raw file):

	for ok, err = it.Next(ctx); ok; ok, err = it.Next(ctx) {
		var row tree.Datums
		if row = it.Cur(); row == nil {

Can this happen? I'm surprised.

pkg/server/index_usage_stats.go line 437 at r5 (raw file):

		}

		if row.Len() != expectedNumDatums {

It seems unlikely to me that this would happen! Is this a pattern we use elsewhere?

THardy98

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @maryliag, and @matthewtodd)

pkg/server/index_usage_stats.go line 433 at r5 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

Can this happen? I'm surprised.

I notice in combined_statement_stats, thought it'd be prudent to follow, but happy to remove if you think it's safe/ineffective.

pkg/server/index_usage_stats.go line 437 at r5 (raw file):

I notice in combined_statement_stats, thought it'd be prudent to follow, but happy to remove if you think it's safe/ineffective.

Ditto :)

matthewtodd

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @maryliag, and @THardy98)

pkg/server/index_usage_stats.go line 433 at r5 (raw file):

Previously, THardy98 (Thomas Hardy) wrote…

I notice in combined_statement_stats, thought it'd be prudent to follow, but happy to remove if you think it's safe/ineffective.

Yeah, I strongly suspect this is deletable fear. :-)

pkg/server/index_usage_stats.go line 437 at r5 (raw file):

Previously, THardy98 (Thomas Hardy) wrote…

I notice in combined_statement_stats, thought it'd be prudent to follow, but happy to remove if you think it's safe/ineffective.
Ditto :)

Hmm, maybe worth asking the team (who have collectively written all the usages I can find of an expectedNumDatums). I suspect it's mostly been copy-pasted and we can drop it.

THardy98

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @maryliag, and @matthewtodd)

pkg/server/index_usage_stats.go line 433 at r5 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

Yeah, I strongly suspect this is deletable fear. :-)

Have removed both these checks from the index stats queries (database and table index stats)

xinhaoz · 2022-12-22T17:27:47Z

pkg/server/admin.go

@@ -761,22 +761,13 @@ func (s *adminServer) getDatabaseStats(
 }

 func (s *adminServer) getNumDatabaseIndexRecommendations(


nit: There's only one caller of getNumDatabaseIndexRecommendations which is just essentially len(getDatabaseIndexRecommendations) right now. I'm wondering if it'll make things slightly clearer to get rid of this wrapper and just use getDatabaseIndexRecommendations directly at the db details function?

Yea makes sense, I've moved it to use getDatabaseIndexRecommendations directly

Resolves: cockroachdb#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.

THardy98 · 2022-12-22T19:51:41Z

TYFR :)

THardy98 · 2022-12-22T19:51:45Z

bors r+

THardy98 · 2022-12-22T20:25:41Z

bors r-

craig · 2022-12-22T20:25:43Z

Canceled.

THardy98 · 2022-12-22T20:25:49Z

bors r+

craig · 2022-12-22T23:24:18Z

Build succeeded:

Bazel Essential CI (Cockroach)

blathers-crl · 2022-12-22T23:24:25Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from 5786a71 to blathers/backport-release-22.2-93937: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

THardy98 requested a review from a team December 19, 2022 20:41

THardy98 requested review from a team as code owners December 19, 2022 20:41

THardy98 requested review from knz and a team December 19, 2022 20:46

THardy98 added T-sql-observability labels Dec 19, 2022

THardy98 force-pushed the fix_database_index_recommendations branch from 4b2c008 to 6678a49 Compare December 19, 2022 20:53

maryliag reviewed Dec 20, 2022

View reviewed changes

THardy98 force-pushed the fix_database_index_recommendations branch from 6678a49 to 947e576 Compare December 20, 2022 14:59

THardy98 commented Dec 20, 2022

View reviewed changes

THardy98 requested a review from maryliag December 20, 2022 16:01

knz reviewed Dec 20, 2022

View reviewed changes

THardy98 commented Dec 20, 2022

View reviewed changes

THardy98 force-pushed the fix_database_index_recommendations branch from 947e576 to ed38453 Compare December 20, 2022 21:01

THardy98 requested a review from knz December 20, 2022 21:01

knz reviewed Dec 20, 2022

View reviewed changes

THardy98 force-pushed the fix_database_index_recommendations branch from ed38453 to 3eec4db Compare December 20, 2022 22:05

THardy98 commented Dec 20, 2022

View reviewed changes

THardy98 requested a review from knz December 20, 2022 22:07

knz reviewed Dec 21, 2022

View reviewed changes

THardy98 force-pushed the fix_database_index_recommendations branch from 3eec4db to dbc5ded Compare December 21, 2022 15:07

THardy98 commented Dec 21, 2022

View reviewed changes

knz reviewed Dec 21, 2022

View reviewed changes

THardy98 force-pushed the fix_database_index_recommendations branch from dbc5ded to 317dbd2 Compare December 21, 2022 15:18

THardy98 requested a review from a team December 21, 2022 15:38

matthewtodd reviewed Dec 21, 2022

View reviewed changes

THardy98 force-pushed the fix_database_index_recommendations branch from 317dbd2 to cd7b581 Compare December 21, 2022 18:51

THardy98 commented Dec 21, 2022

View reviewed changes

THardy98 requested review from matthewtodd and a team December 21, 2022 20:00

matthewtodd reviewed Dec 21, 2022

View reviewed changes

THardy98 force-pushed the fix_database_index_recommendations branch from cd7b581 to a6ad0d8 Compare December 22, 2022 14:54

THardy98 commented Dec 22, 2022

View reviewed changes

THardy98 requested review from matthewtodd and a team December 22, 2022 15:16

THardy98 force-pushed the fix_database_index_recommendations branch 3 times, most recently from adcc997 to 399a1f7 Compare December 22, 2022 16:40

xinhaoz approved these changes Dec 22, 2022

View reviewed changes

THardy98 force-pushed the fix_database_index_recommendations branch from 399a1f7 to 5786a71 Compare December 22, 2022 18:13

craig bot merged commit 6b72f70 into cockroachdb:master Dec 22, 2022

cockroach-teamcity mentioned this pull request Dec 23, 2022

PR #93937 - server: refactor database index recommendations for DatabaseDetails API cockroachdb/docs#15888

Closed

This was referenced Jan 9, 2023

release-22.2: server: refactor database index recommendations for DatabaseDetails API #94923

Merged

sql: databases page and databases details pages are expensive and inefficient with many tables #90196

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: refactor database index recommendations for DatabaseDetails API #93937

server: refactor database index recommendations for DatabaseDetails API #93937

THardy98 commented Dec 19, 2022 •

edited

Loading

cockroach-teamcity commented Dec 19, 2022

maryliag left a comment

THardy98 left a comment

knz left a comment

THardy98 left a comment

knz left a comment

THardy98 left a comment •

edited

Loading

knz left a comment

THardy98 left a comment •

edited

Loading

knz left a comment

matthewtodd left a comment

THardy98 left a comment •

edited

Loading

matthewtodd left a comment

THardy98 left a comment

xinhaoz Dec 22, 2022

THardy98 Dec 22, 2022

THardy98 commented Dec 22, 2022

THardy98 commented Dec 22, 2022

THardy98 commented Dec 22, 2022

craig bot commented Dec 22, 2022

THardy98 commented Dec 22, 2022

craig bot commented Dec 22, 2022

blathers-crl bot commented Dec 22, 2022

		@@ -761,22 +761,13 @@ func (s *adminServer) getDatabaseStats(
		}

		func (s *adminServer) getNumDatabaseIndexRecommendations(

server: refactor database index recommendations for DatabaseDetails API #93937

server: refactor database index recommendations for DatabaseDetails API #93937

Conversation

THardy98 commented Dec 19, 2022 • edited Loading

cockroach-teamcity commented Dec 19, 2022

maryliag left a comment

Choose a reason for hiding this comment

THardy98 left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

THardy98 left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

THardy98 left a comment • edited Loading

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

THardy98 left a comment • edited Loading

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

matthewtodd left a comment

Choose a reason for hiding this comment

THardy98 left a comment • edited Loading

Choose a reason for hiding this comment

matthewtodd left a comment

Choose a reason for hiding this comment

THardy98 left a comment

Choose a reason for hiding this comment

xinhaoz Dec 22, 2022

Choose a reason for hiding this comment

THardy98 Dec 22, 2022

Choose a reason for hiding this comment

THardy98 commented Dec 22, 2022

THardy98 commented Dec 22, 2022

THardy98 commented Dec 22, 2022

craig bot commented Dec 22, 2022

THardy98 commented Dec 22, 2022

craig bot commented Dec 22, 2022

blathers-crl bot commented Dec 22, 2022

THardy98 commented Dec 19, 2022 •

edited

Loading

THardy98 left a comment •

edited

Loading

THardy98 left a comment •

edited

Loading

THardy98 left a comment •

edited

Loading