sql: generate index recommendations#85343
Conversation
|
This change is |
a8a9084 to
76ff0cf
Compare
ajwerner
left a comment
There was a problem hiding this comment.
This is a drive-by review. Consider spelling out the interface for the cache a bit more clearly up-front in the sql package and then perhaps write some testing with a simpler implementation that, for example, just has a big ol' mutex on top of it and then iterate to a more optimized version. Generally best to write some micro-benchmarks with some concurrency to ensure that the optimizations are worth it.
| idxRec.mu.RLock() | ||
| defer idxRec.mu.RUnlock() |
There was a problem hiding this comment.
this needs to be locked exclusively
|
|
||
| // ClearOlderIndexRecommendationsCache clear entries that was last updated | ||
| // more than a day ago. Returns the total deleted entries. | ||
| func (idxRec *IndexRecCache) ClearOlderIndexRecommendationsCache() int { |
There was a problem hiding this comment.
This is a very wordy method name. Also, does is need to be exported?
There was a problem hiding this comment.
Initially I was considering also calling this function in another location periodically to help with the cleanup, but decided to keep it just here. Renamed to be local and smaller
| deleted := idxRec.ClearOlderIndexRecommendationsCache() | ||
| // Abort if no entries were deleted. | ||
| if deleted == 0 { | ||
| atomic.AddInt64(idxRec.atomic.uniqueIndexRecInfo, -int64(1)) | ||
| return | ||
| } |
There was a problem hiding this comment.
You're already doing the decrement inside the call to ClearOlderIndexRecommnedationsCache.
There was a problem hiding this comment.
This is the case where the clean up didn't delete anything, meaning we can't add the new entry, so this reverses the value and return the function
| idxRec.mu.RLock() | ||
| defer idxRec.mu.RUnlock() | ||
|
|
||
| _, found := idxRec.getIndexRecommendation(key) |
There was a problem hiding this comment.
consider returning early if you found it? A way I've seen this done elsewhere is to have a getOrCreate method which attempts to do the get and then re-locks exclusively and attempts to do the create assuming there was not a race.
There was a problem hiding this comment.
I can't really return early, this is just looking if the entry exists, then I need to do the actual set of the new value. I did like the idea of the getOrCreate so made some updates to use that (here and in the other functions too)
792beb6 to
38774ed
Compare
7bb5c5b to
e1abfd8
Compare
j82w
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @j82w, and @maryliag)
pkg/sql/idxrecommendations/idx_recommendations_cache.go line 177 at r2 (raw file):
atomic.AddInt64(&idxRec.atomic.uniqueIndexRecInfo, int64(1)) if incrementedCount > limit {
If this hits the limit all the calls will go into the clearOldIdxRecommendations() which takes a lock. This will likely cause lock contention. To avoid the lock contention it might be better to track what the oldest recommendation is and skip the lock if there are none that are old. The other option I can think of it is track when it ran last and let it only run every x minutes.
pkg/sql/idxrecommendations/idx_recommendations_cache.go line 179 at r2 (raw file):
if incrementedCount > limit { // If we have exceeded limit of unique index recommendations try to delete older data. deleted := idxRec.clearOldIdxRecommendations()
Should this check the count again instead of looking at deleted to avoid possible race condition?
Code snippet:
Thread A -> add -> check -> clearOldIdxRecommendations() returns N so continues to add index.
Thread B -> add -> check -> clearOldIdxRecommendations() returns 0 because it just cleaned a moment before by Thread A.pkg/sql/idxrecommendations/idx_recommendations_cache.go line 191 at r2 (raw file):
// For a new entry, we want the lastGeneratedTs to be in the past, in case we reach // the execution count, we should generate new recommendations. recInfo = indexRecInfo{
Would it be better to have a separate create method to avoid creating a default object?
pkg/sql/idxrecommendations/idx_recommendations_cache.go line 192 at r2 (raw file):
// the execution count, we should generate new recommendations. recInfo = indexRecInfo{ lastGeneratedTs: timeutil.Now().Add(-time.Hour),
Why not use the default time time.Time{}?
pkg/sql/idxrecommendations/idx_recommendations_cache.go line 196 at r2 (raw file):
executionCount: 0, } idxRec.mu.idxRecommendations[key] = recInfo
I don't see a lock between the read above and this create. This allows a possible race condition where the original get returns false for the same key across multiple threads. They each then will come down and take the idxRec lock one by one and replace the recInfo just returned by the previous thread.
bd10fa4 to
5df9fff
Compare
maryliag
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/idxrecommendations/idx_recommendations_cache.go line 177 at r2 (raw file):
Previously, j82w wrote…
If this hits the limit all the calls will go into the clearOldIdxRecommendations() which takes a lock. This will likely cause lock contention. To avoid the lock contention it might be better to track what the oldest recommendation is and skip the lock if there are none that are old. The other option I can think of it is track when it ran last and let it only run every x minutes.
Done: decided to go with the approach of last cleanup ts.
pkg/sql/idxrecommendations/idx_recommendations_cache.go line 179 at r2 (raw file):
Previously, j82w wrote…
Should this check the count again instead of looking at deleted to avoid possible race condition?
Done
pkg/sql/idxrecommendations/idx_recommendations_cache.go line 191 at r2 (raw file):
Previously, j82w wrote…
Would it be better to have a separate create method to avoid creating a default object?
Sorry, I don't follow... this is the method to create. Do you mean one that just returns:
indexRecInfo{
lastGeneratedTs: timeutil.Now().Add(-time.Hour),
recommendations: []string{},
executionCount: 0,
}
pkg/sql/idxrecommendations/idx_recommendations_cache.go line 192 at r2 (raw file):
Previously, j82w wrote…
Why not use the default time
time.Time{}?
Imagine I have a query that runs hourly, this means on hour 5 I will get a recommendation for it, but if on hour 3 we reached the cache limit I'll try to delete data older than 24h, this means:
with this approach: this entry will continue on the cache and will eventually get the recommendation
with the approach of using time.Time{} this entry will get deleted and I won't get recommendations for it
pkg/sql/idxrecommendations/idx_recommendations_cache.go line 196 at r2 (raw file):
Previously, j82w wrote…
I don't see a lock between the read above and this create. This allows a possible race condition where the original get returns false for the same key across multiple threads. They each then will come down and take the idxRec lock one by one and replace the recInfo just returned by the previous thread.
Currently I do a RLock to get the value, if it returns false, I do a Lock to create a value, which could have a race condition, but the alternative would be starting a full lock, getting the value and if false creating the new entry, and then unlocking. The problem with that approach is that because we have a lot more gets than create, you could create more contention if we had to do a full lock for all gets. If the alternative is to maybe have a race condition, I think it's okay for this case, since the first few executions wouldn't generate recommendation anyway.
464f0cf to
31aa741
Compare
j82w
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/idxrecommendations/idx_recommendations_cache.go line 191 at r2 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
Sorry, I don't follow... this is the method to create. Do you mean one that just returns:
indexRecInfo{ lastGeneratedTs: timeutil.Now().Add(-time.Hour), recommendations: []string{}, executionCount: 0, }
I was trying to think of a solution to avoid that ambiguity of it returning a new indexRecInfo vs one that is already exists with a recommendation. Looking it over again I think it is fine.
pkg/sql/idxrecommendations/idx_recommendations_cache.go line 192 at r2 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
Imagine I have a query that runs hourly, this means on hour 5 I will get a recommendation for it, but if on hour 3 we reached the cache limit I'll try to delete data older than 24h, this means:
with this approach: this entry will continue on the cache and will eventually get the recommendation
with the approach of usingtime.Time{}this entry will get deleted and I won't get recommendations for it
Good point, I didn't consider the clean up task. My only concern is if it the clean up window gets lowered to 1h for some reason this will break.
pkg/sql/idxrecommendations/idx_recommendations_cache.go line 196 at r2 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
Currently I do a RLock to get the value, if it returns false, I do a Lock to create a value, which could have a race condition, but the alternative would be starting a full lock, getting the value and if false creating the new entry, and then unlocking. The problem with that approach is that because we have a lot more gets than create, you could create more contention if we had to do a full lock for all gets. If the alternative is to maybe have a race condition, I think it's okay for this case, since the first few executions wouldn't generate recommendation anyway.
This is the solution I've seen implemented where it does another check after taking the full lock to verify another thread did not update it.
Code snippet:
recInfo, found := idxRec.getIndexRecommendation(key)
if found {
return recInfo, true
}
...
idxRec.mu.Lock()
defer idxRec.mu.Unlock()
recInfo, found = idxRec.mu.idxRecommendations[key]
if found {
return recInfo, true
}
recInfo = indexRecInfo{...}
return recInfo, true
j82w
left a comment
There was a problem hiding this comment.
Reviewed 13 of 16 files at r2, 3 of 3 files at r5, all commit messages.
Reviewable status:
yuzefovich
left a comment
There was a problem hiding this comment.
Changes in sql package LGTM, just a few nits.
Reviewed 4 of 16 files at r2, 1 of 3 files at r5.
Reviewable status:
pkg/roachpb/app_stats.go line 158 at r5 (raw file):
s.Nodes = util.CombineUniqueInt64(s.Nodes, other.Nodes) s.PlanGists = util.CombineUniqueString(s.PlanGists, other.PlanGists) s.IndexRecommendations = util.CombineUniqueString(s.IndexRecommendations, other.IndexRecommendations)
Why did this change?
pkg/sql/instrumentation.go line 669 at r5 (raw file):
} // SetIndexRecommendations check if we should generate a new index recommendation.
nit: s/check/checks/ s/use the value/uses the value/ s/and update/and updates/.
pkg/sql/instrumentation.go line 686 at r5 (raw file):
) { f := opc.optimizer.Factory() // EvalContext() has the context of the already closed span, so we need to update with the current context.
nit: s/of the/with the, also wrap at 80 characters.
|
TFTR! |
|
Merge conflict. |
We want to collect index recommendations
per statement and save it to the statement_statistics table.
To accomplish this, a new cache map was created.
The key for the map is a combination of the
statement fingerprint, the database and the plan hash,
since those are the parameters that would affect
an index recommendation (from the parameters we use
as keys for the statement fingerprint ID at least).
This cache has a limit of unique recommendations,
limited by a new cluster setting called
`sql.metrics.statement_details.max_mem_reported_idx_recommendations`
with a default value of 100k.
If this limit is reached, we delete entries that
had their last update made more than 24h ago.
A new recommendations is generated if has been more
than 1h since the last recommendation (saved on cache)
and it has at least a few executions (so we handle the case
where we have just one execution of several different
fingerprints, and is not worth calculating recommendations
for them).
The recommendations are saved as a list, with each
recommendations being "type : sql command", e.g.
`{"creation : CREATE INDEX ON t2 (i) STORING (k);"}`
and
`{"replacement : CREATE UNIQUE INDEX ON t1 (i) STORING (k);
DROP INDEX t1@existing_t1_i;"}`
Closes cockroachdb#83782
Release note (sql change): Index recommendations are generated
for statements. New recommendations are generated every hour and
available on `system.statement_statistics` and
`crdb_internal.statement_statistics`.
New cluster setting with default value of 100k
sql.metrics.statement_details.max_mem_reported_idx_recommendations
|
bors r+ |
|
Build succeeded: |
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/roachpb/app_stats.go line 158 at r5 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
Why did this change?
@maryliag independently from the comment from two months ago I arrived at this code line again, and I'm still curious - why did this line change in this PR? :)
maryliag
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/roachpb/app_stats.go line 158 at r5 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
@maryliag independently from the comment from two months ago I arrived at this code line again, and I'm still curious - why did this line change in this PR? :)
(I opened here just to find that I did write the reply, but was still on draft and never got send 🤦 sorry about that)
When I added this line on a previous PR I hadn't quite decided the format it would come from the index recommendation on cache, so it was just preparing to persist and I just used the default as combining like we have for everything else.
Now I always want to update my stats with the latest recommendation I have. If I use the combine I might get some weird combination that no longer make sense.
Imagine you run a statement and have 2 recommendations, the format of the IndexRecommendations is a string with all recommendations, e.g. rec1; rec2;, then you run again, but this time you get rec3, rec 4;, I only care about rec3; rec4;, so I overwrite what I had on the previous one, otherwise combining would give me rec1; rec2; rec3; rec4;. Maybe the confusion is coming from imagining this is combining a rec1 and rec2, which is not the case.
Does that help?
|
Yes, makes sense, thanks for explaining! |
In cockroachdb#85343 for 22.2 we added automatic collection of index recommendations for DML statements. This collection occurred after execution, and initially re-ran optbuild for the query, before doing the usual index recommendation generation steps of (1) detatching the memo, (2) optimizing with hypothetical indexes, and then (3) restoring the detatched memo. In cockroachdb#99081 for 23.2 we moved collection of index recommendations from after execution to between planning and execution, which simplified some things. But this revealed that some queries refer to the memo (and its metadata) during execution, and re-running optbuild can change the contents of the memo (and its metadata) from how they were after planning, expecially if the original memo was cached and re-used. I think this initial optbuild step was added to ensure we always have a root expression in the memo. This commit changes index recommendation collection to only run optbuild if the memo is empty, and otherwise use the memo that comes out of planning. Fixes: cockroachdb#117307 Release note (bug fix): Fix a bug introduced in 23.2 that causes internal errors and panics when certain queries run with automatic index recommendation collection enabled.
In cockroachdb#85343 for 22.2 we added automatic collection of index recommendations for DML statements. This collection occurred after execution, and initially re-ran optbuild for the query, before doing the usual index recommendation generation steps of (1) detatching the memo, (2) optimizing with hypothetical indexes, and then (3) restoring the detatched memo. In cockroachdb#99081 for 23.2 we moved collection of index recommendations from after execution to between planning and execution, which simplified some things. But this revealed that some queries refer to the memo (and its metadata) during execution, and re-running optbuild can change the contents of the memo (and its metadata) from how they were after planning, expecially if the original memo was cached and re-used. I think this initial optbuild step was added to ensure we always have a root expression in the memo. This commit changes index recommendation collection to only run optbuild if the memo is empty, and otherwise use the memo that comes out of planning. Fixes: cockroachdb#117307 Release note (bug fix): Fix a bug introduced in 23.2 that causes internal errors and panics when certain queries run with automatic index recommendation collection enabled.
In cockroachdb#85343 for 22.2 we added automatic collection of index recommendations for DML statements. This collection occurred after execution, and initially re-ran optbuild for the query, before doing the usual index recommendation generation steps of (1) detaching the memo, (2) optimizing with hypothetical indexes, and then (3) restoring the detached memo. In cockroachdb#99081 for 23.2 we moved collection of index recommendations from after execution to between planning and execution, which simplified some things. But this revealed that some queries refer to the memo (and its metadata) during execution, and re-running optbuild can change the contents of the memo (and its metadata) from how they were after planning, especially if the original memo was cached and re-used. I think this initial optbuild step was added to ensure we always have a root expression in the memo. This commit changes index recommendation collection to only run optbuild if the memo is empty, and otherwise use the memo that comes out of planning. Fixes: cockroachdb#117307 Release note (bug fix): Fix a bug introduced in 23.2 that causes internal errors and panics when certain queries run with automatic index recommendation collection enabled.
117433: sql: do not re-run optbuild before collecting index recommendations r=DrewKimball a=michae2 In #85343 for 22.2 we added automatic collection of index recommendations for DML statements. This collection occurred after execution, and initially re-ran optbuild for the query, before doing the usual index recommendation generation steps of (1) detaching the memo, (2) optimizing with hypothetical indexes, and then (3) restoring the detached memo. In #99081 for 23.2 we moved collection of index recommendations from after execution to between planning and execution, which simplified some things. But this revealed that some queries refer to the memo (and its metadata) during execution, and re-running optbuild can change the contents of the memo (and its metadata) from how they were after planning, especially if the original memo was cached and re-used. I think this initial optbuild step was added to ensure we always have a root expression in the memo. This commit changes index recommendation collection to only run optbuild if the memo is empty, and otherwise use the memo that comes out of planning. Fixes: #117307 Release note (bug fix): Fix a bug introduced in 23.2 that causes internal errors and panics when certain queries run with automatic index recommendation collection enabled. Co-authored-by: Michael Erickson <michae2@cockroachlabs.com>
In #85343 for 22.2 we added automatic collection of index recommendations for DML statements. This collection occurred after execution, and initially re-ran optbuild for the query, before doing the usual index recommendation generation steps of (1) detaching the memo, (2) optimizing with hypothetical indexes, and then (3) restoring the detached memo. In #99081 for 23.2 we moved collection of index recommendations from after execution to between planning and execution, which simplified some things. But this revealed that some queries refer to the memo (and its metadata) during execution, and re-running optbuild can change the contents of the memo (and its metadata) from how they were after planning, especially if the original memo was cached and re-used. I think this initial optbuild step was added to ensure we always have a root expression in the memo. This commit changes index recommendation collection to only run optbuild if the memo is empty, and otherwise use the memo that comes out of planning. Fixes: #117307 Release note (bug fix): Fix a bug introduced in 23.2 that causes internal errors and panics when certain queries run with automatic index recommendation collection enabled.
In #85343 for 22.2 we added automatic collection of index recommendations for DML statements. This collection occurred after execution, and initially re-ran optbuild for the query, before doing the usual index recommendation generation steps of (1) detaching the memo, (2) optimizing with hypothetical indexes, and then (3) restoring the detached memo. In #99081 for 23.2 we moved collection of index recommendations from after execution to between planning and execution, which simplified some things. But this revealed that some queries refer to the memo (and its metadata) during execution, and re-running optbuild can change the contents of the memo (and its metadata) from how they were after planning, especially if the original memo was cached and re-used. I think this initial optbuild step was added to ensure we always have a root expression in the memo. This commit changes index recommendation collection to only run optbuild if the memo is empty, and otherwise use the memo that comes out of planning. Fixes: #117307 Release note (bug fix): Fix a bug introduced in 23.2 that causes internal errors and panics when certain queries run with automatic index recommendation collection enabled.
We want to collect index recommendations
per statement and save it to the statement_statistics table.
To accomplish this, a new cache map was created.
The key for the map is a combination of the
statement fingerprint, the database and the plan hash,
since those are the parameters that would affect
an index recommendation (from the parameters we use
as keys for the statement fingerprint ID at least).
This cache has a limit of unique recommendations,
limited by a new cluster setting called
sql.metrics.statement_details.max_mem_reported_idx_recommendationswith a default value of 100k.
If this limit is reached, we delete entries that
had their last update made more than 24h ago.
A new recommendations is generated if has been more
than 1h since the last recommendation (saved on cache)
and it has at least a few executions (so we handle the case
where we have just one execution of several different
fingerprints, and is not worth calculating recommendations
for them).
The recommendations are saved as a list, with each
recommendations being "type : sql command", e.g.
{"creation : CREATE INDEX ON t2 (i) STORING (k);"}and
{"replacement : CREATE UNIQUE INDEX ON t1 (i) STORING (k); DROP INDEX t1@existing_t1_i;"}Fixes #83782
Different approaches tested:
Min Execution Count is the minimum value of execution the statement must have so we decide to generate recommendation
Running TPC C on all branches, results:
The most significant change is when there is no cache, confirming the cache is necessary.
The decision on keeping or removing the execution count check wasn't clear, so I decided to keep the count with a value of 5 and run another workload.
Running YSCB A:
Keeping the execution count has a lower performance when there is more contention, but since we have customers that generate several similar fingerprints by using different columns order on selects etc, we want to avoid creating recommendations for those cases, since it would be almost as having no cache.
So I created another option (branch idx-rec-cache), where we do have a count, but once the count reaches the min count, we don't need to update it anymore (helping with contention on the cache itself).
Running workloads with those options again:
TPC C
YSCB A
Release note (sql change): Index recommendations are generated
for statements. New recommendations are generated every hour and
available on
system.statement_statisticsandcrdb_internal.statement_statistics.New cluster setting with default value of 100k
sql.metrics.statement_details.max_mem_reported_idx_recommendations