[Feature] Support setting session vars in user property #48477

yandongxiao · 2024-07-17T02:08:20Z

Why I'm doing:

When the user connects to FE SQL, the Session variables need to be initialized according to the user's attribute information.

What I'm doing:

Support to CRUD user's properties.

Fixes: #48478

What type of PR is this:

Does this PR entail a change in behavior?

Yes, this PR will result in a change in behavior.
No, this PR will not result in a change in behavior.

If yes, please specify the type of change:

Interface/UI changes: syntax, type conversion, expression evaluation, display information
Parameter changes: default values, similar parameters but with different default values
Policy changes: use new policy to replace old one, functionality automatically enabled
Feature removed
Miscellaneous: upgrade & downgrade compatibility, etc.

Checklist:

I have added test cases for my bug fix or my new feature
This pr needs user documentation (for new or modified features or behaviors)
- I have added documentation for my new feature or new function
This is a backport pr

Bugfix cherry-pick branch check:

alvin-celerdata · 2024-07-17T02:21:31Z

@yandongxiao
Please create a Feature-Request GitHub Issue, and fill the necessary design doc in it.

yandongxiao · 2024-07-17T02:40:24Z

@yandongxiao Please create a Feature-Request GitHub Issue, and fill the necessary design doc in it.

Done

fe/fe-core/src/main/java/com/starrocks/sql/parser/StarRocks.g4

fe/fe-core/src/main/java/com/starrocks/mysql/nio/AcceptListener.java

fe/fe-core/src/main/java/com/starrocks/authentication/UserProperty.java

nshangyiming · 2024-07-23T12:30:35Z

fe/fe-core/src/main/java/com/starrocks/authentication/UserProperty.java

+        }
+    }
+
+    private UserIdentity getUserIdentityByName(String userName) {


put it into AuthenticationMgr

fe/fe-core/src/main/java/com/starrocks/authentication/UserProperty.java

yandongxiao · 2024-07-24T06:09:25Z

Fixed all based on review comments

HangyuanLiu · 2024-07-26T03:33:10Z

fe/fe-core/src/test/java/com/starrocks/sql/analyzer/AnalyzeStmtTest.java

@@ -122,7 +122,8 @@ public void testShowUserProperty() {

    @Test
    public void testSetUserProperty() {
-        String sql = "SET PROPERTY FOR 'tom' 'max_user_connections' = 'value', 'test' = 'true'";
+        String sql = "SET PROPERTY FOR 'tom' 'max_user_connections' = '100  " +
+                "git '";


good catch!

HangyuanLiu · 2024-07-26T08:35:48Z

fe/fe-core/src/main/java/com/starrocks/qe/ConnectContext.java

+    public void updateByUserProperty(UserProperty userProperty) {
+        try {
+            // set catalog and database
+            boolean dBHasBeenSetByUser = !getCurrentCatalog().equals(InternalCatalog.DEFAULT_INTERNAL_CATALOG_NAME) ||


Suggested change

boolean dBHasBeenSetByUser = !getCurrentCatalog().equals(InternalCatalog.DEFAULT_INTERNAL_CATALOG_NAME) ||

boolean dbHasBeenSetByUser = !getCurrentCatalog().equals(InternalCatalog.DEFAULT_INTERNAL_CATALOG_NAME) ||

HangyuanLiu · 2024-07-26T08:40:52Z

fe/fe-core/src/main/java/com/starrocks/qe/ConnectContext.java

+        try {
+            // set catalog and database
+            boolean dBHasBeenSetByUser = !getCurrentCatalog().equals(InternalCatalog.DEFAULT_INTERNAL_CATALOG_NAME) ||
+                    !getDatabase().isEmpty();


Now there is a scenario where the default catalog of the global session variable is not default_catalog. Then does this user's property catalog become ineffective?

HangyuanLiu · 2024-07-26T08:56:21Z

fe/fe-core/src/main/java/com/starrocks/authentication/AuthenticationMgr.java

+                        properties.containsKey(UserProperty.PROP_DATABASE)) {
+                    // Authorizer.checkAnyActionOnOrInDb and Authorizer.checkAnyActionOnCatalog methods are not allowed to be called,
+                    // because they assume the user has been created.
+                    throw new PrivilegeException(String.format("User property %s and %s are not allowed to set in CREATE USER",


There is no need to check permissions when creating a user. You can check permissions when connecting, and the behavior is similar to mysql --database. Because even if you can check permissions when altering user, what if the permissions are revoked later? How do you deal with it?

HangyuanLiu · 2024-07-26T09:03:54Z

fe/fe-core/src/main/java/com/starrocks/authentication/UserProperty.java

+
+        try {
+            if (!CatalogMgr.isInternalCatalog(value)) {
+                Authorizer.checkAnyActionOnCatalog(user, roleIds, value);


I think the same as above. No need check privilege

HangyuanLiu · 2024-07-26T09:08:29Z

fe/fe-core/src/main/java/com/starrocks/authentication/UserProperty.java

    @SerializedName(value = "m")
-    private long maxConn = 1024;
+    private long maxConn = MAX_CONN_DEFAULT_VALUE;


Why do we need to manage these separately? Can we unify the coding with system variables? Just make sure that user properties and system variables do not have the same name (this is easy for us to do because user properties are also under our control). The current approach 1. It brings complexity to the code. 2. The logic is not unified. There is a catalog in the user property and there is also a catalog in the session variable, and it is called "session.catalog". How to maintain the relationship between these two pieces of data, and what is the meaning of these two identical pieces of data? We should use user properties as a supplement and overwrite of system variables.

Signed-off-by: yandongxiao <[email protected]>

sonarcloud · 2024-08-02T02:25:54Z

Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

github-actions · 2024-08-02T04:24:47Z

[FE Incremental Coverage Report]

✅ pass : 195 / 223 (87.44%)

file detail

	path	covered_line	new_line	coverage	not_covered_line_detail
🔵	com/starrocks/sql/ast/ShowUserPropertyStmt.java	0	6	00.00%	[75, 76, 77, 78, 79, 80]
🔵	com/starrocks/persist/EditLog.java	1	2	50.00%	[1078]
🔵	com/starrocks/persist/AlterUserInfo.java	3	4	75.00%	[59]
🔵	com/starrocks/qe/VariableMgr.java	6	7	85.71%	[673]
🔵	com/starrocks/authentication/AuthenticationMgr.java	25	29	86.21%	[322, 323, 611, 622]
🔵	com/starrocks/authentication/UserProperty.java	121	134	90.30%	[138, 165, 191, 206, 215, 246, 247, 250, 252, 256, 258, 262, 303]
🔵	com/starrocks/qe/ConnectContext.java	22	24	91.67%	[1124, 1130]
🔵	com/starrocks/mysql/nio/AcceptListener.java	3	3	100.00%	[]
🔵	com/starrocks/qe/ExecuteAsExecutor.java	6	6	100.00%	[]
🔵	com/starrocks/sql/ast/AlterUserStmt.java	1	1	100.00%	[]
🔵	com/starrocks/privilege/AuthorizationMgr.java	1	1	100.00%	[]
🔵	com/starrocks/qe/SetExecutor.java	1	1	100.00%	[]
🔵	com/starrocks/sql/analyzer/ShowUserPropertyAnalyzer.java	1	1	100.00%	[]
🔵	com/starrocks/sql/ast/BaseCreateAlterUserStmt.java	2	2	100.00%	[]
🔵	com/starrocks/sql/ast/CreateUserStmt.java	1	1	100.00%	[]
🔵	com/starrocks/qe/DDLStmtExecutor.java	1	1	100.00%	[]

github-actions · 2024-08-02T04:24:54Z

[BE Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

yandongxiao · 2024-08-15T03:14:02Z

@mergify backport 3.3

mergify · 2024-08-15T03:14:23Z

backport 3.3

❌ No backport have been created

Backport to branch 3.3 failed

Git reported the following error:

fatal: couldn't find remote ref 3.3

yandongxiao · 2024-08-15T03:27:37Z

@mergify backport-3.3

mergify · 2024-08-15T03:27:46Z

backport -3.3

❌ No backport have been created

Backport to branch -3.3 failed

GitHub error: Branch not found

yandongxiao · 2024-08-15T03:28:10Z

@mergify backport branch-3.3

mergify · 2024-08-15T03:28:17Z

backport branch-3.3

✅ Backports have been created

#49827 [Feature] Support setting session vars in user property (backport #48477) has been created for branch branch-3.3 but encountered conflicts

Signed-off-by: yandongxiao <[email protected]> (cherry picked from commit 17d1914) # Conflicts: # fe/fe-core/src/main/java/com/starrocks/sql/ast/AlterUserStmt.java # fe/fe-core/src/main/java/com/starrocks/sql/ast/BaseCreateAlterUserStmt.java # fe/fe-core/src/main/java/com/starrocks/sql/ast/CreateUserStmt.java # fe/fe-core/src/main/java/com/starrocks/sql/parser/AstBuilder.java

) Signed-off-by: yandongxiao <[email protected]>

) (#49828) Signed-off-by: yandongxiao <[email protected]>

yandongxiao requested review from a team as code owners July 17, 2024 02:08

wanpengfei-git added the META-REVIEW label Jul 17, 2024

wanpengfei-git requested a review from a team July 17, 2024 02:08

mergify bot assigned yandongxiao Jul 17, 2024

nshangyiming self-assigned this Jul 17, 2024

yandongxiao force-pushed the feature/support-session-var-in-user-property-next branch 2 times, most recently from b1db23f to 792180f Compare July 19, 2024 03:36

nshangyiming reviewed Jul 23, 2024

View reviewed changes

yandongxiao requested a review from nshangyiming July 24, 2024 06:13

HangyuanLiu self-assigned this Jul 25, 2024

HangyuanLiu reviewed Jul 26, 2024

View reviewed changes

nshangyiming previously approved these changes Jul 29, 2024

View reviewed changes

yandongxiao dismissed nshangyiming’s stale review via 8b1b92d July 29, 2024 04:59

yandongxiao requested a review from a team as a code owner July 29, 2024 12:57

yandongxiao force-pushed the feature/support-session-var-in-user-property-next branch from 6fd28cd to ee1d2d2 Compare July 30, 2024 04:11

nshangyiming previously approved these changes Jul 31, 2024

View reviewed changes

yandongxiao dismissed nshangyiming’s stale review via 8ad6cc9 August 1, 2024 03:13

HangyuanLiu previously approved these changes Aug 1, 2024

View reviewed changes

[Feature] Support setting session vars in user property

44ff04e

Signed-off-by: yandongxiao <[email protected]>

yandongxiao dismissed HangyuanLiu’s stale review via 44ff04e August 2, 2024 02:18

yandongxiao force-pushed the feature/support-session-var-in-user-property-next branch from 514234a to 44ff04e Compare August 2, 2024 02:18

HangyuanLiu approved these changes Aug 2, 2024

View reviewed changes

nshangyiming approved these changes Aug 2, 2024

View reviewed changes

andyziye enabled auto-merge (squash) August 2, 2024 06:08

Seaven approved these changes Aug 2, 2024

View reviewed changes

andyziye merged commit 17d1914 into StarRocks:main Aug 2, 2024
49 of 50 checks passed

github-actions bot added the version:3.4 label Aug 2, 2024

wanpengfei-git mentioned this pull request Aug 2, 2024

Support setting session vars in user property #48478

Open

1 task

mergify bot mentioned this pull request Aug 15, 2024

[Feature] Support setting session vars in user property (backport #48477) #49827

Closed

24 tasks

yandongxiao added a commit to yandongxiao/starrocks that referenced this pull request Aug 15, 2024

[Feature] Support setting session vars in user property (StarRocks#48477

bb05f43

) Signed-off-by: yandongxiao <[email protected]>

andyziye pushed a commit that referenced this pull request Aug 15, 2024

[Feature] Support setting session vars in user property (backport #48477

58ef227

) (#49828) Signed-off-by: yandongxiao <[email protected]>

github-actions bot added the 3.3-merged label Aug 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Support setting session vars in user property #48477

[Feature] Support setting session vars in user property #48477

yandongxiao commented Jul 17, 2024 •

edited

Loading

alvin-celerdata commented Jul 17, 2024

yandongxiao commented Jul 17, 2024

nshangyiming Jul 23, 2024

yandongxiao Jul 24, 2024

yandongxiao commented Jul 24, 2024

HangyuanLiu Jul 26, 2024

yandongxiao Jul 26, 2024

HangyuanLiu Jul 26, 2024

HangyuanLiu Jul 26, 2024

HangyuanLiu Jul 26, 2024

HangyuanLiu Jul 26, 2024

HangyuanLiu Jul 26, 2024

sonarcloud bot commented Aug 2, 2024

github-actions bot commented Aug 2, 2024

github-actions bot commented Aug 2, 2024

yandongxiao commented Aug 15, 2024

mergify bot commented Aug 15, 2024 •

edited

Loading

yandongxiao commented Aug 15, 2024

mergify bot commented Aug 15, 2024

yandongxiao commented Aug 15, 2024

mergify bot commented Aug 15, 2024 •

edited

Loading

	boolean dBHasBeenSetByUser = !getCurrentCatalog().equals(InternalCatalog.DEFAULT_INTERNAL_CATALOG_NAME) \|\|
	boolean dbHasBeenSetByUser = !getCurrentCatalog().equals(InternalCatalog.DEFAULT_INTERNAL_CATALOG_NAME) \|\|

[Feature] Support setting session vars in user property #48477

[Feature] Support setting session vars in user property #48477

Conversation

yandongxiao commented Jul 17, 2024 • edited Loading

Why I'm doing:

What I'm doing:

What type of PR is this:

Checklist:

Bugfix cherry-pick branch check:

alvin-celerdata commented Jul 17, 2024

yandongxiao commented Jul 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yandongxiao commented Jul 24, 2024

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

sonarcloud bot commented Aug 2, 2024

Quality Gate failed

github-actions bot commented Aug 2, 2024

[FE Incremental Coverage Report]

file detail

github-actions bot commented Aug 2, 2024

[BE Incremental Coverage Report]

yandongxiao commented Aug 15, 2024

mergify bot commented Aug 15, 2024 • edited Loading

❌ No backport have been created

yandongxiao commented Aug 15, 2024

mergify bot commented Aug 15, 2024

❌ No backport have been created

yandongxiao commented Aug 15, 2024

mergify bot commented Aug 15, 2024 • edited Loading

✅ Backports have been created

yandongxiao commented Jul 17, 2024 •

edited

Loading

mergify bot commented Aug 15, 2024 •

edited

Loading

mergify bot commented Aug 15, 2024 •

edited

Loading