-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Expose session ID to clients #41622
Expose session ID to clients #41622
Conversation
Hey @georgebuckerfield so sorry about the delay here. |
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.
Thanks for the contribution! Couple minor things but generally looks great. Please rebase and squash your commits. And just to match our conventions, please update the commit subject to sql: expose session ID to clients
.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @georgebuckerfield, @rohany, and @solongordon)
pkg/sql/crdb_internal.go, line 903 at r1 (raw file):
clusterSessionID := BytesToClusterWideID(session.ID) sessionID = tree.NewDString(clusterSessionID.String()) }
It looks like this logic is now duplicated with populateSessionsTable
. Could you pull this out into a function?
pkg/sql/planner.go, line 41 at r3 (raw file):
tree.EvalContext sessionID *ClusterWideID
FYI I believe this was just added for other reasons, so you can just rebase and use the existing field.
0d3800f
to
1e94859
Compare
Thanks for the review @solongordon! I think it should be ready for another look now - let me know if there are any other changes you'd like. |
This change exposes the ID of a session in two ways: - as a session variable (session_id) - as a column in SHOW QUERIES Fixes cockroachdb#32055 Release note (sql change): The ID of the current session is now available via a session_id variable. Session IDs are also now shown in SHOW QUERIES results.
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.
I'm just going to add a release note to your commit message to notify our docs team of the change.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @georgebuckerfield and @rohany)
1e94859
to
251885f
Compare
bors r+ |
41622: Expose session ID to clients r=solongordon a=georgebuckerfield This change exposes the ID of a session to clients in two ways: - as a session variable (session_id) - as a column in SHOW QUERIES e.g. ``` root@:26257/defaultdb> show session_id; session_id +----------------------------------+ 15cded2b6282789c0000000000000001 (1 row) Time: 3.6358ms root@:26257/defaultdb> show queries; query_id | node_id | session_id | user_name | start | query | client_address | application_name | distributed | phase +----------------------------------+---------+----------------------------------+-----------+----------------------------------+----------------------+-----------------+------------------+-------------+-----------+ 15cdefea7e589d3c0000000000000001 | 1 | 15cded2b6282789c0000000000000001 | root | 2019-10-15 21:40:35.442981+00:00 | SHOW CLUSTER QUERIES | 127.0.0.1:39450 | $ cockroach sql | false | executing (1 row) Time: 25.2691ms ``` Fixes #32055 Release note: None Co-authored-by: georgebuckerfield <georgebuckerfield@gmail.com>
Build succeeded |
This change exposes the ID of a session to clients in two ways:
e.g.
Fixes #32055
Release note: None