-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
contention: store contention events on non-SQL keys #61828
Conversation
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 @yuzefovich)
pkg/sql/contention/registry.go, line 83 at r1 (raw file):
const estimatedAverageKeySize = 64 txnsMapSize := maxNumTxns * int(unsafe.Sizeof(uuid.UUID{})+unsafe.Sizeof(int(0))) orderedKeyMapSize := orderedKeyMapMaxSize * (int(unsafe.Sizeof(comparableKey{})+estimatedAverageKeySize) + txnsMapSize)
Notable change here, I think the previous calculation was incorrect.
Also, what do we want to do with this method? We're now using the registry, but it's estimated max memory footprint (assuming it is about 4.5MiB) seems low enough to me that we can just ignore it since it is on a per node basis. I think we could be concerned if the keys in the contention events are large, but then the estimate doesn't help us much.
75cc1c9
to
c0c39ce
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.
Reviewed 11 of 12 files at r1, 2 of 2 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)
pkg/sql/contention/registry.go, line 83 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
Notable change here, I think the previous calculation was incorrect.
Also, what do we want to do with this method? We're now using the registry, but it's estimated max memory footprint (assuming it is about 4.5MiB) seems low enough to me that we can just ignore it since it is on a per node basis. I think we could be concerned if the keys in the contention events are large, but then the estimate doesn't help us much.
I think it's fine to delete it.
Previously, whenever we tried to add a contention event on a non-SQL key, it would encounter an error during decoding tableID/indexID pair, and the event was dropped. This commit extends the contention registry to additionally store information about contention on non-SQL keys. That information is stored in two levels: - on the top level, all `SingleNonSQLKeyContention` objects are ordered by their keys - on the bottom level, all `SingleTxnContention` objects are ordered by the number of times that transaction was observed to contend with other transactions. `SingleTxnContention` protobuf message is moved out of `SingleKeyContention` and is reused for non-SQL keys. This commit also updates the status server API response. I assume that no changes are needed with regards to backwards compatibility since the original version was merged just a few weeks ago, and we haven't had a beta released since then. Release note (sql change): CockroachDB now also stores the information about contention on non-SQL keys.
c0c39ce
to
2041f62
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 (and 1 stale) (waiting on @asubiotto)
pkg/sql/contention/registry.go, line 83 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
I think it's fine to delete it.
Cool, removed.
TFTR! bors r+ |
bors r+ |
Already running a review |
Build succeeded: |
Previously, whenever we tried to add a contention event on a non-SQL
key, it would encounter an error during decoding tableID/indexID pair,
and the event was dropped. This commit extends the contention registry
to additionally store information about contention on non-SQL keys. That
information is stored in two levels:
SingleNonSQLKeyContention
objects are orderedby their keys
SingleTxnContention
objects are ordered by thenumber of times that transaction was observed to contend with other
transactions.
SingleTxnContention
protobuf message is moved out ofSingleKeyContention
and is reused for non-SQL keys. This commit alsoupdates the status server API response. I assume that no changes are
needed with regards to backwards compatibility since the original
version was merged just a few weeks ago, and we haven't had a beta
released since then.
Fixes: #60669.
Release note (sql change): CockroachDB now also stores the information
about contention on non-SQL keys.