refactor system variables to support account isolation

[ck89119]

What type of PR is this?

• API-change
• BUG
• Improvement
• Documentation
• Feature
• Test and CI
• Code Refactoring

Which issue(s) this PR fixes:

issue #16047

What this PR does / why we need it:

• system variable now is account isolated
• table mo_mysql_compatibility_mode only saves delta info between account's and cluster's default system variable values
• always use session variable except show global variables

[codiumai-pr-agent-pro]

PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

PR Review 🔍

⏱️ Estimated effort to review [1-5]	4, because the PR involves substantial changes across multiple files, including refactoring of system variables handling, authentication processes, and session management. The changes impact core functionalities such as session initialization, variable management, and transaction handling. Additionally, the PR modifies both the frontend and backend components, which requires careful review to ensure consistency and correctness across the system.
🧪 Relevant tests	Yes
⚡ Possible issues	Possible Bug: The removal of GlobalSystemVariables and introduction of GlobalSysVarsMgr could lead to issues if not all instances where global system variables were used have been updated to use the new manager. This includes ensuring that all necessary locks and thread-safety mechanisms are correctly implemented.
	Refactoring Impact: The changes in session and variable handling need thorough testing to ensure that existing functionalities related to transaction management, session variables, and system variables are not adversely affected.
	Performance Concerns: The new locking mechanisms and changes in the management of system variables could introduce performance bottlenecks, especially in high concurrency environments. It's crucial to benchmark these changes under load to identify any potential degradation in performance.
🔒 Security concerns	No

[codiumai-pr-agent-pro]

PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

PR Code Suggestions ✨

Category	Suggestion	Score
Best practice	Handle errors from getSystemVariables more gracefully by returning the error to the caller The getSystemVariables function call in genSession should handle errors more gracefully by returning the error to the caller instead of calling t.Error(err). pkg/frontend/session_test.go [236-238] sv, err := getSystemVariables("test/system_vars_config.toml") if err != nil { - t.Error(err) + return nil, err } Suggestion importance[1-10]: 8 Why: This suggestion addresses a best practice by improving error handling in the genSession function. It correctly identifies the specific lines where the error handling can be improved, making the function more robust and maintainable by returning errors instead of directly logging them. This change is crucial for better error propagation and handling in test scenarios.	8
	Handle the error returned by uuid.NewV7() in the NewSession function to ensure proper UUID generation In the NewSession function, handle the error returned by uuid.NewV7() to ensure that the session UUID is correctly generated. pkg/frontend/session.go [505] -ses.uuid, _ = uuid.NewV7() +if ses.uuid, err = uuid.NewV7(); err != nil { + return nil, fmt.Errorf("failed to generate session UUID: %w", err) +} Suggestion importance[1-10]: 7 Why: Handling errors from uuid.NewV7() is important for ensuring the session UUID is correctly generated, which is crucial for session management and tracking.	7
	Add a call to ctrl.Finish() to ensure all mock expectations are met and to avoid memory leaks The Test_checkTenantExistsOrNot function should include a call to ctrl.Finish() at the end of the test to ensure that all expectations are met and to avoid potential memory leaks. pkg/frontend/authenticate_test.go [178-179] convey.Convey("check tenant exists or not", t, func() { ctrl := gomock.NewController(t) + defer ctrl.Finish() Suggestion importance[1-10]: 7 Why: The suggestion correctly identifies a missing defer ctrl.Finish() in the test function, which is important for proper cleanup of resources and ensuring that all expectations on the mock controller are met. This is a good practice in Go testing with mocks.	7
Possible bug	Add a nil check for ses.GetTenantInfo() in InitSystemVariables to avoid potential nil pointer dereference The InitSystemVariables function should handle the case where ses.GetTenantInfo() returns nil to avoid potential nil pointer dereference. pkg/frontend/session.go [234] -if ses.gSysVars, err = GSysVarsMgr.Get(ses.GetTenantInfo().TenantID, ses, ctx); err != nil { +tenantInfo := ses.GetTenantInfo() +if tenantInfo == nil { + return errors.New("tenant info is nil") +} +if ses.gSysVars, err = GSysVarsMgr.Get(tenantInfo.TenantID, ses, ctx); err != nil { Suggestion importance[1-10]: 8 Why: This suggestion correctly identifies a potential nil pointer dereference, which is a critical issue that could cause runtime panics.	8
Maintainability	Move the genSession function outside of the TestSession_TxnBegin function to avoid redefining it multiple times The genSession function in TestSession_TxnBegin does not need to be defined inside the test function. Consider moving it outside to avoid redefining it multiple times if used in other tests. pkg/frontend/session_test.go [230-257] +func genSession(ctrl *gomock.Controller) *Session { + ioSes := mock_frontend.NewMockIOSession(ctrl) + ioSes.EXPECT().OutBuf().Return(buf.NewByteBuf(1024)).AnyTimes() + ioSes.EXPECT().Write(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + ioSes.EXPECT().RemoteAddress().Return("").AnyTimes() + ioSes.EXPECT().Ref().AnyTimes() + sv, err := getSystemVariables("test/system_vars_config.toml") + if err != nil { + t.Error(err) + } + setGlobalPu(&config.ParameterUnit{ + SV: sv, + }) + proto := NewMysqlClientProtocol(0, ioSes, 1024, sv) + txnOperator := mock_frontend.NewMockTxnOperator(ctrl) + txnOperator.EXPECT().Txn().Return(txn.TxnMeta{}).AnyTimes() + txnOperator.EXPECT().Commit(gomock.Any()).Return(nil).AnyTimes() + eng := mock_frontend.NewMockEngine(ctrl) + eng.EXPECT().New(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + getGlobalPu().TxnClient = txnClient + getGlobalPu().StorageEngine = eng + session := NewSession(ctx, proto, nil) + return session +} + func TestSession_TxnBegin(t *testing.T) { ctx := defines.AttachAccountId(context.Background(), sysAccountID) - genSession := func(ctrl *gomock.Controller) *Session { - ioSes := mock_frontend.NewMockIOSession(ctrl) - ioSes.EXPECT().OutBuf().Return(buf.NewByteBuf(1024)).AnyTimes() - ioSes.EXPECT().Write(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() - ioSes.EXPECT().RemoteAddress().Return("").AnyTimes() - ioSes.EXPECT().Ref().AnyTimes() - sv, err := getSystemVariables("test/system_vars_config.toml") - if err != nil { - t.Error(err) - } - setGlobalPu(&config.ParameterUnit{ - SV: sv, - }) - proto := NewMysqlClientProtocol(0, ioSes, 1024, sv) - txnOperator := mock_frontend.NewMockTxnOperator(ctrl) - txnOperator.EXPECT().Txn().Return(txn.TxnMeta{}).AnyTimes() - txnOperator.EXPECT().Commit(gomock.Any()).Return(nil).AnyTimes() - eng := mock_frontend.NewMockEngine(ctrl) - eng.EXPECT().New(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() - getGlobalPu().TxnClient = txnClient - getGlobalPu().StorageEngine = eng - session := NewSession(ctx, proto, nil) - return session - } Suggestion importance[1-10]: 7 Why: The suggestion to move genSession outside of TestSession_TxnBegin is valid for maintainability and reusability. It correctly identifies the function within the new hunk of the PR code, and the improvement would help avoid redundancy if the function is used in multiple tests.	7
	Move the genSession function outside of the TestSession_TxnCompilerContext function to avoid redefining it multiple times The genSession function in TestSession_TxnCompilerContext does not need to be defined inside the test function. Consider moving it outside to avoid redefining it multiple times if used in other tests. pkg/frontend/session_test.go [295-308] +func genSession(ctrl *gomock.Controller, pu *config.ParameterUnit) *Session { + ioses := mock_frontend.NewMockIOSession(ctrl) + ioses.EXPECT().OutBuf().Return(buf.NewByteBuf(1024)).AnyTimes() + ioses.EXPECT().Write(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + ioses.EXPECT().RemoteAddress().Return("").AnyTimes() + ioses.EXPECT().Ref().AnyTimes() + sv, err := getSystemVariables("test/system_vars_config.toml") + if err != nil { + t.Error(err) + } + setGlobalPu(&config.ParameterUnit{ + SV: sv, + }) + proto := NewMysqlClientProtocol(0, ioses, 1024, sv) + ctx := defines.AttachAccountId(context.Background(), sysAccountID) + session := NewSession(ctx, proto, nil) + return session +} + func TestSession_TxnCompilerContext(t *testing.T) { - genSession := func(ctrl *gomock.Controller, pu *config.ParameterUnit) *Session { - ioses := mock_frontend.NewMockIOSession(ctrl) - ioses.EXPECT().OutBuf().Return(buf.NewByteBuf(1024)).AnyTimes() - ioses.EXPECT().Write(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() - ioses.EXPECT().RemoteAddress().Return("").AnyTimes() - ioses.EXPECT().Ref().AnyTimes() - sv, err := getSystemVariables("test/system_vars_config.toml") - if err != nil { - t.Error(err) - } - setGlobalPu(&config.ParameterUnit{ - SV: sv, - }) - proto := NewMysqlClientProtocol(0, ioses, 1024, sv) - ctx := defines.AttachAccountId(context.Background(), sysAccountID) - session := NewSession(ctx, proto, nil) - return session - } Suggestion importance[1-10]: 7 Why: Similar to the first suggestion, moving genSession outside of TestSession_TxnCompilerContext enhances maintainability. The suggestion correctly targets the function within the new hunk of the PR code, promoting better code organization and reuse.	7
	Rename the function to maintain consistency with naming conventions Consider renaming the function getSqlForGetSysVarWithAccount to getSqlForGetSystemVariableWithAccount for consistency with the naming convention used in the rest of the code. pkg/frontend/authenticate.go [1931-1933] -func getSqlForGetSysVarWithAccount(accountId uint64, varName string) string { +func getSqlForGetSystemVariableWithAccount(accountId uint64, varName string) string { return fmt.Sprintf(getSystemVariableWithAccountFormat, accountId, varName) } Suggestion importance[1-10]: 7 Why: Renaming getSqlForGetSysVarWithAccount to getSqlForGetSystemVariableWithAccount improves consistency with other function names, enhancing readability and maintainability.	7
	Create a helper function to set up the global parameter unit to reduce code duplication The setGlobalPu function is called multiple times in the tests. Consider creating a helper function to set up the global parameter unit to reduce code duplication. pkg/frontend/session_test.go [81-83] -pu, err := getParameterUnit("test/system_vars_config.toml", eng, txnClient) +func setupGlobalPu(configPath string, eng Engine, txnClient TxnClient) error { + pu, err := getParameterUnit(configPath, eng, txnClient) + if err != nil { + return err + } + setGlobalPu(pu) + return nil +} + +// Usage in tests +err := setupGlobalPu("test/system_vars_config.toml", eng, txnClient) convey.So(err, convey.ShouldBeNil) -setGlobalPu(pu) Suggestion importance[1-10]: 6 Why: This suggestion correctly identifies repetitive code related to setting up the global parameter unit across different test functions. Creating a helper function would reduce duplication and improve code clarity, aligning well with the context of the PR changes.	6
	Remove commented-out code to keep the codebase clean and maintainable In the function createSubscriptionDatabase, consider removing the commented-out code to keep the codebase clean and maintainable. pkg/frontend/authenticate.go [8018-8056] // TODO implement this function (#8946) by other ways return nil -//ctx, span := trace.Debug(ctx, "createSubscriptionDatabase") -//defer span.End() -// -//subscriptions := make([]string, 0) -////process the syspublications -//sysPubsVar, err := ses.GetSessionSysVar("syspublications") -//if err != nil { -// return err -//} -// -//if sysPublications, ok := sysPubsVar.(string); ok { -// if len(sysPublications) == 0 { -// return err -// } -// subscriptions = strings.Split(sysPublications, ",") -//} -//// if no subscriptions, return -//if len(subscriptions) == 0 { -// return err -//} -// -////with new tenant -//ctx = defines.AttachAccount(ctx, uint32(newTenant.GetTenantID()), uint32(newTenant.GetUserID()), uint32(newTenant.GetDefaultRoleID())) -// -//createSubscriptionFormat := `create database %s from sys publication %s;` -//sqls := make([]string, 0, len(subscriptions)) -//for _, subscription := range subscriptions { -// sqls = append(sqls, fmt.Sprintf(createSubscriptionFormat, subscription, subscription)) -//} -//for _, sql := range sqls { -// bh.ClearExecResultSet() -// err = bh.Exec(ctx, sql) -// if err != nil { -// return err -// } -//} -//return err Suggestion importance[1-10]: 6 Why: Removing extensive commented-out code in createSubscriptionDatabase helps maintain a cleaner and more maintainable codebase, reducing clutter.	6
Readability	Rename the parameter for better clarity and consistency In the function getSqlForUpdateSysVarValue, consider renaming the parameter varValue to variableValue for better clarity and consistency with other function parameters. pkg/frontend/authenticate.go [1940-1942] -func getSqlForUpdateSysVarValue(varValue string, accountId uint64, varName string) string { - return fmt.Sprintf(updateSystemVariableValueFormat, varValue, accountId, varName) +func getSqlForUpdateSysVarValue(variableValue string, accountId uint64, varName string) string { + return fmt.Sprintf(updateSystemVariableValueFormat, variableValue, accountId, varName) } Suggestion importance[1-10]: 5 Why: Renaming varValue to variableValue in getSqlForUpdateSysVarValue slightly enhances clarity and consistency with naming conventions, though the impact on overall code quality is moderate.	5
	Simplify the function by using a single-line return statement In the function getSqlForInsertSysVarWithAccount, consider using a single-line return statement for better readability. pkg/frontend/authenticate.go [1935-1937] func getSqlForInsertSysVarWithAccount(accountId uint64, accountName string, varName string, varValue string) string { - return fmt.Sprintf(insertSystemVariableWithAccountFormat, accountId, accountName, varName, varValue, true) + return fmt.Sprintf(insertSystemVariableWithAccountFormat, accountId, accountName, varName, varValue, true) } Suggestion importance[1-10]: 4 Why: The suggestion to use a single-line return statement in getSqlForInsertSysVarWithAccount is valid for readability, but the impact is minimal as the function is already quite simple.	4