*: bind user to some resource group #39561

BornChanger · 2022-12-01T17:46:20Z

What problem does this PR solve?

Issue Number: Ref #38825

Problem Summary:
The Spec show details: https://docs.google.com/document/d/1P_D1R1qGMFjmjbGCxYjRodgp1Yeyx4dBvg2W1l_Eiy0/edit?n=1-page_Spec_template#

What is changed and how it works?

add support to bind user to resource group at create/alter user time.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

ti-chi-bot · 2022-12-01T17:46:23Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

CbcWestwolf
tiancaiamao

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

BornChanger · 2022-12-01T17:48:13Z

/assign @nolouch @CbcWestwolf @glorv

BornChanger · 2022-12-01T17:48:54Z

/unassign @nolouch @CbcWestwolf @glorv

BornChanger · 2022-12-01T17:49:15Z

/cc @nolouch @CbcWestwolf @glorv

BornChanger · 2022-12-01T17:52:48Z

/cc @tiancaiamao

tiancaiamao

Isn't the 'alter user RESOURCE GROUP ...' a DDL?
I don't see any of the change of ddl files in this PR.

parser/parser.y

tiancaiamao · 2022-12-05T11:04:41Z

privilege/privileges/privileges.go

+
+	// special handling to existing users or root user initialized with insecure
+	if record.ResourceGroup == "" {
+		record.ResourceGroup = "default"


record is a shallow copy from the source, so change it here also change the original value.
Althrough the correctness still seem to be hold, it may data race here.

Just return "default", there is no need to change the record.ResourceGroup

session/session.go

CbcWestwolf · 2022-12-05T15:35:11Z

parser/parser.y

@@ -12889,6 +12892,11 @@ CommentOrAttributeOption:
 		$$ = &ast.CommentOrAttributeOption{Type: ast.UserAttributeType, Value: $2}
 	}

+|  "RESOURCE" "GROUP" stringLit


I prefer to declare "RESOURCE" "GROUP" stringLit with a new term ResourceGroupOption, and add ResourceGroupOption into CreateUserStmt and AlterUserStmt after CommentOrAttributeOption, like

AlterUserStmt: "ALTER" "USER" IfExists UserSpecList RequireClauseOpt ConnectionOptions PasswordOrLockOptions CommentOrAttributeOption ResourceGroupOption

In my implementation, "resource group" is just one attribute for user, like email. which is stored in User_attributes column of mysql.user.

So can we just use 'alter user ... attribute ...' without provide an extra new syntax?

No. Resource group might be a frequently used attribute. So, we introduce new syntax for it following the pattern of comment.

I prefer to declare "RESOURCE" "GROUP" stringLit with a new term ResourceGroupOption, and add ResourceGroupOption into CreateUserStmt and AlterUserStmt after CommentOrAttributeOption, like

AlterUserStmt: "ALTER" "USER" IfExists UserSpecList RequireClauseOpt ConnectionOptions PasswordOrLockOptions CommentOrAttributeOption ResourceGroupOption

fixed

…ap#26) Signed-off-by: BornChanger <dawn_catcher@126.com>

…ingcap#27) Signed-off-by: BornChanger <dawn_catcher@126.com>

Signed-off-by: BornChanger <dawn_catcher@126.com>

CbcWestwolf

Rest LGTM

executor/simple.go

Co-authored-by: CbcWestwolf <1004626265@qq.com>

tiancaiamao · 2022-12-07T03:12:35Z

/merge

ti-chi-bot · 2022-12-07T03:12:38Z

This pull request has been accepted and is ready to merge.

Commit hash: 7cef273

BornChanger · 2022-12-07T09:43:16Z

/run-check_dev_2

jackysp · 2022-12-07T11:10:00Z

/merge

sre-bot · 2022-12-07T11:52:53Z

TiDB MergeCI notify

✅ Well Done! New fixed [2] after this pr merged.

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci/integration-cdc-test	✅ all 40 tests passed	21 min	Fixed
idc-jenkins-ci-tidb/mybatis-test	✅ all 1 tests passed	3 min 54 sec	Fixed
idc-jenkins-ci-tidb/integration-common-test	🟢 all 17 tests passed	20 min	Existing passed
idc-jenkins-ci-tidb/common-test	🟢 all 11 tests passed	11 min	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	6 min 23 sec	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	5 min 25 sec	Existing passed
idc-jenkins-ci-tidb/integration-ddl-test	🟢 all 6 tests passed	5 min 3 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	4 min 30 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	2 min 34 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

BornChanger requested a review from a team as a code owner December 1, 2022 17:46

ti-chi-bot added do-not-merge/invalid-title release-note-none Denotes a PR that doesn't merit a release note. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 1, 2022

ti-chi-bot assigned CbcWestwolf, glorv and nolouch Dec 1, 2022

ti-chi-bot unassigned glorv, nolouch and CbcWestwolf Dec 1, 2022

ti-chi-bot requested review from CbcWestwolf, glorv and nolouch December 1, 2022 17:49

ti-chi-bot requested a review from tiancaiamao December 1, 2022 17:52

tiancaiamao reviewed Dec 5, 2022

View reviewed changes

CbcWestwolf reviewed Dec 5, 2022

View reviewed changes

BornChanger and others added 3 commits December 6, 2022 09:47

*: add support to create/alter user with resource group option (pingc…

3304176

…ap#26) Signed-off-by: BornChanger <dawn_catcher@126.com>

*: pass user's resource group name to sessionVars.ResourceGroupName (p…

ad41e83

…ingcap#27) Signed-off-by: BornChanger <dawn_catcher@126.com>

*: add new case

f367d60

Signed-off-by: BornChanger <dawn_catcher@126.com>

BornChanger force-pushed the bind_resource_group branch from 43e8715 to 07e5406 Compare December 6, 2022 04:52

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 6, 2022

BornChanger force-pushed the bind_resource_group branch from 07e5406 to 57d07c4 Compare December 6, 2022 04:55

BornChanger changed the title ~~Bind user to some resource group~~ DCL: Bind user to some resource group Dec 6, 2022

ti-chi-bot removed the do-not-merge/invalid-title label Dec 6, 2022

CbcWestwolf approved these changes Dec 6, 2022

View reviewed changes

executor/simple.go Outdated Show resolved Hide resolved

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 6, 2022

Update executor/simple.go

7cef273

Co-authored-by: CbcWestwolf <1004626265@qq.com>

tiancaiamao approved these changes Dec 7, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 7, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 7, 2022

ti-chi-bot added 10 commits December 7, 2022 11:12

Merge branch 'master' into bind_resource_group

e9ae4c9

Merge branch 'master' into bind_resource_group

4b4afc4

Merge branch 'master' into bind_resource_group

a4562a1

Merge branch 'master' into bind_resource_group

8353889

Merge branch 'master' into bind_resource_group

d74f2ba

Merge branch 'master' into bind_resource_group

f9d2799

Merge branch 'master' into bind_resource_group

3f19d4f

Merge branch 'master' into bind_resource_group

1ea6455

Merge branch 'master' into bind_resource_group

fc4a9c9

Merge branch 'master' into bind_resource_group

fedbf1a

ti-chi-bot added 2 commits December 7, 2022 17:48

Merge branch 'master' into bind_resource_group

1f0e7a3

Merge branch 'master' into bind_resource_group

36b57d4

tiancaiamao approved these changes Dec 7, 2022

View reviewed changes

Merge branch 'master' into bind_resource_group

3ee0500

tiancaiamao changed the title ~~DCL: Bind user to some resource group~~ *: bind user to some resource group Dec 7, 2022

jackysp approved these changes Dec 7, 2022

View reviewed changes

ti-chi-bot merged commit 8e3cbac into pingcap:master Dec 7, 2022

nolouch mentioned this pull request Dec 26, 2022

Global Resource Quota Control #38825

Closed

41 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: bind user to some resource group #39561

*: bind user to some resource group #39561

BornChanger commented Dec 1, 2022 •

edited

Loading

ti-chi-bot commented Dec 1, 2022 •

edited

Loading

BornChanger commented Dec 1, 2022

BornChanger commented Dec 1, 2022

BornChanger commented Dec 1, 2022

BornChanger commented Dec 1, 2022

tiancaiamao left a comment

tiancaiamao Dec 5, 2022

tiancaiamao Dec 5, 2022

BornChanger Dec 6, 2022

CbcWestwolf Dec 5, 2022

BornChanger Dec 6, 2022

tiancaiamao Dec 6, 2022

BornChanger Dec 6, 2022

BornChanger Dec 7, 2022

CbcWestwolf left a comment

tiancaiamao commented Dec 7, 2022

ti-chi-bot commented Dec 7, 2022

BornChanger commented Dec 7, 2022

jackysp commented Dec 7, 2022

sre-bot commented Dec 7, 2022

*: bind user to some resource group #39561

*: bind user to some resource group #39561

Conversation

BornChanger commented Dec 1, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Dec 1, 2022 • edited Loading

BornChanger commented Dec 1, 2022

BornChanger commented Dec 1, 2022

BornChanger commented Dec 1, 2022

BornChanger commented Dec 1, 2022

tiancaiamao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CbcWestwolf left a comment

Choose a reason for hiding this comment

tiancaiamao commented Dec 7, 2022

ti-chi-bot commented Dec 7, 2022

BornChanger commented Dec 7, 2022

jackysp commented Dec 7, 2022

sre-bot commented Dec 7, 2022

TiDB MergeCI notify

BornChanger commented Dec 1, 2022 •

edited

Loading

ti-chi-bot commented Dec 1, 2022 •

edited

Loading