-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
session: support set current session's resource group name #41722
Conversation
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
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.
lgtm
ptal @tiancaiamao @BornChanger |
@@ -133,3 +133,17 @@ func TestUserAttributes(t *testing.T) { | |||
rootTK.MustExec("alter user usr1 resource group rg1") | |||
rootTK.MustQuery("select user_attributes from mysql.user where user = 'usr1'").Check(testkit.Rows(`{"metadata": {"comment": "comment1"}, "resource_group": "rg1"}`)) | |||
} | |||
|
|||
func TestSetResourceGroup(t *testing.T) { |
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.
Please add case to verify hint overrides session setting and session setting override use binding.
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.
done
@nolouch I have added support for |
/test unit-test |
/test check-dev |
/build |
@tiancaiamao PTAL, as this PR has introduced a new builtin function. |
/test unit-test |
parser/parser.y
Outdated
currentTs "CURRENT_TIMESTAMP" | ||
currentUser "CURRENT_USER" | ||
currentRole "CURRENT_ROLE" | ||
currentResourceGroup "CURRENT_RESOURCE_GROUP" |
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.
MySQL does not have CURRENT_RESOURCE_GROUP
as a reserved keyword https://dev.mysql.com/doc/refman/8.0/en/keywords.html
Maybe we can make it non-keyword...
The different is that you can't create table CURRENT_RESOURCE_GROUP (...)
after this change.
But the current way is still OK to me, since it follows the CURRENT_XXX
as reserved keyword convention in MySQL
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.
Oh, yes, I think we can make it a non-keyword, so the compatibility should be better.
/test build |
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 17c9d01
|
/test build |
@glorv: The specified target(s) for
Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/test build |
/test unit-test |
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 6960de8
|
/test unit-test |
What problem does this PR solve?
Issue Number: ref #38825
Problem Summary:
What is changed and how it works?
Add following two sql:
SET RESOURCE GROUP <name>;
to change the resource group of the current sessionSELECT current_resource_group();
to return the resource group of the current sessionSupport set resource group name for the current session.
NOTE: currently, we don't do privilege for this statement. Will do it in the future version.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.