-
Couldn't load subscription status.
- Fork 4k
sql/inspect: add hash-based precheck to index consistency validation #155720
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sql/inspect: add hash-based precheck to index consistency validation #155720
Conversation
10e8237 to
ee37440
Compare
|
Here are the results of the performance tests when I ran this in roachtest: 1. Admission Control Test (25M rows)Fast version:
Slow version:
Performance improvement (fast vs slow):
2. Throughput Test - 500M rows, 1 indexFast version:
Slow version:
Performance improvement:
3. Throughput Test - 1B rows, 2 indexesFast version:
Slow version:
Performance improvement:
|
ee37440 to
380b450
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a great improvement, nice work! lgtm -- my comments are just about error wrapping and word choice, so feel free to merge after responding.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @spilchen)
pkg/sql/inspect/index_consistency_check.go line 242 at r1 (raw file):
// be ignored. if isQueryConstructionError(hashErr) { return hashErr
shouldn't we never expect a query construction error? if so, then this could be wrapped with an assertion failure.
pkg/sql/inspect/index_consistency_check.go line 244 at r1 (raw file):
return hashErr } log.Dev.Infof(ctx, "hash precheck for index consistency failed; falling back to full check: %v", hashErr)
super nit: instead of "failed" could we use language closer to "did not match"? i'm thinking that might make this log message more clear, since although the check did "fail" someone who sees this might think that CRDB itself encountered a failure.
380b450 to
ced327e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @rafiss)
pkg/sql/inspect/index_consistency_check.go line 242 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
shouldn't we never expect a query construction error? if so, then this could be wrapped with an assertion failure.
Yes, makes sense. We should never expect this since it's an internally generated query.
pkg/sql/inspect/index_consistency_check.go line 244 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
super nit: instead of "failed" could we use language closer to "did not match"? i'm thinking that might make this log message more clear, since although the check did "fail" someone who sees this might think that CRDB itself encountered a failure.
Done.
ced327e to
d5f078b
Compare
Before performing the expensive full join to compare primary and secondary indexes during the INSPECT index consistency check, this change adds a fast, low-cost precheck step. We now compute and compare row counts and hash values from both indexes. If the counts and hashes match, we can confidently skip the full check. This optimization should improves performance for healthy indexes. Closes: cockroachdb#150927 Epic: CRDB-55075 Release note: None
d5f078b to
da01a52
Compare
|
TFTR! bors r+ |
Before performing the expensive full join to compare primary and secondary indexes during the INSPECT index consistency check, this change adds a fast, low-cost precheck step.
We now compute and compare row counts and hash values from both indexes. If the counts and hashes match, we can confidently skip the full check. This optimization should improves performance for healthy indexes.
Closes: #150927
Epic: CRDB-55075
Release note: None