From 829cc7c22573bc16a336ebb67789f4ef731df633 Mon Sep 17 00:00:00 2001 From: Jensen Date: Tue, 15 Oct 2024 11:33:36 +0800 Subject: [PATCH 1/3] clean code --- pkg/frontend/authenticate.go | 82 ------------------------- pkg/frontend/back_exec.go | 2 +- pkg/frontend/compiler_context.go | 4 +- pkg/frontend/self_handle.go | 12 +--- pkg/frontend/session.go | 58 +++++------------ pkg/frontend/types.go | 2 +- pkg/sql/compile/sql_executor_context.go | 2 +- 7 files changed, 23 insertions(+), 139 deletions(-) diff --git a/pkg/frontend/authenticate.go b/pkg/frontend/authenticate.go index ade332d3c699..e199bf77b533 100644 --- a/pkg/frontend/authenticate.go +++ b/pkg/frontend/authenticate.go @@ -8245,7 +8245,6 @@ func doAlterDatabaseConfig(ctx context.Context, ses *Session, ad *tree.AlterData configVar := make(map[tree.DatabaseConfig]string, 0) configVar[tree.MYSQL_COMPATIBILITY_MODE] = "version_compatibility" - configVar[tree.UNIQUE_CHECK_ON_AUTOINCR] = "unique_check_on_autoincr" dbName := ad.DbName updateConfig := ad.UpdateConfig @@ -8307,27 +8306,6 @@ func doAlterDatabaseConfig(ctx context.Context, ses *Session, ad *tree.AlterData return rtnErr } - rtnErr = bh.Exec(ctx, sql) - case tree.UNIQUE_CHECK_ON_AUTOINCR: - valueCheck := func(value string) error { - switch value { - case "None": - case "Check": - case "Error": - default: - return moerr.NewBadConfigf(ctx, "unique_check_on_autoincr %s", value) - } - return nil - } - - if rtnErr = valueCheck(updateConfig); rtnErr != nil { - return rtnErr - } - - sql, rtnErr = getSqlForupdateConfigurationByDbNameAndAccountName(ctx, updateConfig, accountName, dbName, configVar[configTyp]) - if rtnErr != nil { - return rtnErr - } rtnErr = bh.Exec(ctx, sql) default: panic("unknown database config type") @@ -8410,9 +8388,6 @@ func insertRecordToMoMysqlCompatibilityMode(ctx context.Context, ses *Session, s versionValue, _ := ses.GetSessionSysVar("version") variableValue1 := getVariableValue(versionValue) - variableName2 := "unique_check_on_autoincr" - variableValue2 := "None" - if createDatabaseStmt, ok := stmt.(*tree.CreateDatabase); ok { dbName = string(createDatabaseStmt.Name) //if create sys database, do nothing @@ -8449,16 +8424,6 @@ func insertRecordToMoMysqlCompatibilityMode(ctx context.Context, ses *Session, s sql = fmt.Sprintf(initMoMysqlCompatibilityModeFormat, accountId, accountName, dbName, variableName1, variableValue1, false) rtnErr = bh.Exec(ctx, sql) - if rtnErr != nil { - return rtnErr - } - - sql = fmt.Sprintf(initMoMysqlCompatibilityModeFormat, accountId, accountName, dbName, variableName2, variableValue2, false) - - rtnErr = bh.Exec(ctx, sql) - if rtnErr != nil { - return rtnErr - } return rtnErr } err = insertRecordFunc() @@ -8467,11 +8432,7 @@ func insertRecordToMoMysqlCompatibilityMode(ctx context.Context, ses *Session, s } } - ses.SetConfig(dbName, variableName1, variableValue1) - ses.SetConfig(dbName, variableName2, variableValue2) - return nil - } func deleteRecordToMoMysqlCompatbilityMode(ctx context.Context, ses *Session, stmt tree.Statement) error { @@ -8510,8 +8471,6 @@ func deleteRecordToMoMysqlCompatbilityMode(ctx context.Context, ses *Session, st return err } } - ses.DeleteConfig(ctx, datname, "version_compatibility") - ses.DeleteConfig(ctx, datname, "unique_check_on_autoincr") return nil } @@ -8556,47 +8515,6 @@ func GetVersionCompatibility(ctx context.Context, ses *Session, dbName string) ( return resultConfig, err } -func GetUniqueCheckOnAutoIncr(ctx context.Context, ses *Session, dbName string) (ret string, err error) { - var erArray []ExecResult - var sql string - var resultConfig string - defaultConfig := "None" - variableName := "unique_check_on_autoincr" - bh := ses.GetBackgroundExec(ctx) - defer bh.Close() - - err = bh.Exec(ctx, "begin") - defer func() { - err = finishTxn(ctx, bh, err) - }() - if err != nil { - return defaultConfig, err - } - - sql = getSqlForGetSystemVariableValueWithDatabase(dbName, variableName) - - bh.ClearExecResultSet() - err = bh.Exec(ctx, sql) - if err != nil { - return defaultConfig, err - } - - // risky : this error is actually dropped to pass TestSession_Migrate - erArray, err = getResultSet(ctx, bh) - if err != nil { - return defaultConfig, nil - } - - if execResultArrayHasData(erArray) { - resultConfig, err = erArray[0].GetString(ctx, 0, 0) - if err != nil { - return defaultConfig, err - } - } - - return resultConfig, err -} - func doInterpretCall(ctx context.Context, ses *Session, call *tree.CallStmt) ([]ExecResult, error) { // fetch related var spBody string diff --git a/pkg/frontend/back_exec.go b/pkg/frontend/back_exec.go index 783b59c098cb..10881ce6e9b1 100644 --- a/pkg/frontend/back_exec.go +++ b/pkg/frontend/back_exec.go @@ -713,7 +713,7 @@ func (backSes *backSession) SendRows() int64 { return 0 } -func (backSes *backSession) GetConfig(ctx context.Context, dbName, varName string) (any, error) { +func (backSes *backSession) GetConfig(ctx context.Context, varName, dbName, tblName string) (any, error) { return nil, moerr.NewInternalError(ctx, "do not support get config in background exec") } diff --git a/pkg/frontend/compiler_context.go b/pkg/frontend/compiler_context.go index d45cc335eff4..2caeaf6160b0 100644 --- a/pkg/frontend/compiler_context.go +++ b/pkg/frontend/compiler_context.go @@ -248,7 +248,7 @@ func (tcc *TxnCompilerContext) GetDatabaseId(dbName string, snapshot *plan2.Snap return databaseId, nil } -func (tcc *TxnCompilerContext) GetDbLevelConfig(dbName string, varName string) (string, error) { +func (tcc *TxnCompilerContext) GetConfig(varName string, dbName string, tblName string) (string, error) { switch varName { case "unique_check_on_autoincr": // check if the database is a system database @@ -257,7 +257,7 @@ func (tcc *TxnCompilerContext) GetDbLevelConfig(dbName string, varName string) ( if _, ok := ses.(*backSession); ok { return "None", nil } - ret, err := ses.GetConfig(tcc.GetContext(), dbName, varName) + ret, err := ses.GetConfig(tcc.GetContext(), varName, dbName, tblName) if err != nil { return "", err } diff --git a/pkg/frontend/self_handle.go b/pkg/frontend/self_handle.go index 881e9afe9905..d38d8b1d1b52 100644 --- a/pkg/frontend/self_handle.go +++ b/pkg/frontend/self_handle.go @@ -41,19 +41,9 @@ func execInFrontend(ses *Session, execCtx *ExecCtx) (err error) { case *tree.Use: ses.EnterFPrint(FPUse) defer ses.ExitFPrint(FPUse) - var uniqueCheckOnAuto string dbName := st.Name.Compare() //use database - err = handleChangeDB(ses, execCtx, dbName) - if err != nil { - return - } - - uniqueCheckOnAuto, err = GetUniqueCheckOnAutoIncr(execCtx.reqCtx, ses, dbName) - if err != nil { - return - } - ses.SetConfig(dbName, "unique_check_on_autoincr", uniqueCheckOnAuto) + return handleChangeDB(ses, execCtx, dbName) case *tree.MoDump: //dump diff --git a/pkg/frontend/session.go b/pkg/frontend/session.go index a580706581f6..302e38ef504f 100644 --- a/pkg/frontend/session.go +++ b/pkg/frontend/session.go @@ -130,10 +130,6 @@ type Session struct { showStmtType ShowStatementType userDefinedVars map[string]*UserDefinedVar - // db/tbl level config store in mo_mysql_compatibility_mode, need to read and write through SQL - // this map is maintained at the session level to cache configuration results. - configs map[string]interface{} - prepareStmts map[string]*PrepareStmt lastStmtId uint32 @@ -147,8 +143,7 @@ type Session struct { cache *privilegeCache - mu sync.Mutex - rwmu sync.RWMutex + mu sync.Mutex lastInsertID uint64 @@ -553,7 +548,6 @@ func NewSession( statsCache: plan2.NewStatsCache(), } ses.userDefinedVars = make(map[string]*UserDefinedVar) - ses.configs = make(map[string]interface{}) ses.prepareStmts = make(map[string]*PrepareStmt) // For seq init values. ses.seqCurValues = make(map[uint64]string) @@ -1025,52 +1019,34 @@ func (ses *Session) GetUserDefinedVar(name string) (*UserDefinedVar, error) { return val, nil } -// SetUserDefinedVar sets the config to the value in session -func (ses *Session) SetConfig(dbName, varName string, valValue any) error { - ses.rwmu.Lock() - defer ses.rwmu.Unlock() - - // TODO : validate the config name and value - ses.configs[dbName+"-"+varName] = valValue - return nil -} - // GetUserDefinedVar gets value of the config -func (ses *Session) GetConfig(ctx context.Context, dbName, varName string) (any, error) { - ses.rwmu.RLock() - defer ses.rwmu.RUnlock() - if val, ok := ses.configs[dbName+"-"+varName]; ok { - return val, nil - } - if varName == "unique_check_on_autoincr" { - ret, err := GetUniqueCheckOnAutoIncr(ctx, ses, dbName) - if err != nil { - return nil, err - } - ses.configs[dbName+"-"+varName] = ret - return ret, nil - } +func (ses *Session) GetConfig(ctx context.Context, varName, dbName, tblName string) (any, error) { + // if val, ok := ses.configs[dbName+"-"+varName]; ok { + // return val, nil + // } + // if varName == "unique_check_on_autoincr" { + // ret, err := GetUniqueCheckOnAutoIncr(ctx, ses, dbName) + // if err != nil { + // return nil, err + // } + // ses.configs[dbName+"-"+varName] = ret + // return ret, nil + // } return nil, moerr.NewInternalError(ctx, errorConfigDoesNotExist()) } func (ses *Session) SetCreateVersion(version string) { - ses.rwmu.Lock() - defer ses.rwmu.Unlock() + ses.mu.Lock() + defer ses.mu.Unlock() ses.createVersion = version } func (ses *Session) GetCreateVersion() string { - ses.rwmu.RLock() - defer ses.rwmu.RUnlock() + ses.mu.Lock() + defer ses.mu.Unlock() return ses.createVersion } -func (ses *Session) DeleteConfig(ctx context.Context, dbName, varName string) { - ses.rwmu.Lock() - defer ses.rwmu.Unlock() - delete(ses.configs, dbName+"-"+varName) -} - func (ses *Session) GetTxnInfo() string { txnH := ses.GetTxnHandler() if txnH == nil { diff --git a/pkg/frontend/types.go b/pkg/frontend/types.go index 3b57969db47f..d999e303c89e 100644 --- a/pkg/frontend/types.go +++ b/pkg/frontend/types.go @@ -462,7 +462,7 @@ type FeSession interface { GetSql() string GetAccountId() uint32 GetTenantInfo() *TenantInfo - GetConfig(ctx context.Context, dbName, varName string) (any, error) + GetConfig(ctx context.Context, varName, dbName, tblName string) (any, error) GetBackgroundExec(ctx context.Context) BackgroundExec GetRawBatchBackgroundExec(ctx context.Context) BackgroundExec GetGlobalSysVars() *SystemVariables diff --git a/pkg/sql/compile/sql_executor_context.go b/pkg/sql/compile/sql_executor_context.go index 4fd9578be591..04ec2de59a3b 100644 --- a/pkg/sql/compile/sql_executor_context.go +++ b/pkg/sql/compile/sql_executor_context.go @@ -231,7 +231,7 @@ func (c *compilerContext) GetDatabaseId(dbName string, snapshot *plan.Snapshot) return databaseId, nil } -func (c *compilerContext) GetDbLevelConfig(dbName string, varName string) (string, error) { +func (c *compilerContext) GetConfig(varName, dbName, tblName string) (string, error) { switch varName { // For scenarios that are background SQL, use the default configuration to avoid triggering background SQL again. case "unique_check_on_autoincr": From 384fcf503759c09acaa6821474e4f6606dec250a Mon Sep 17 00:00:00 2001 From: Jensen Date: Tue, 15 Oct 2024 14:26:04 +0800 Subject: [PATCH 2/3] fix coverage --- pkg/frontend/compiler_context.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/pkg/frontend/compiler_context.go b/pkg/frontend/compiler_context.go index 2caeaf6160b0..3c01b4efc50f 100644 --- a/pkg/frontend/compiler_context.go +++ b/pkg/frontend/compiler_context.go @@ -253,18 +253,18 @@ func (tcc *TxnCompilerContext) GetConfig(varName string, dbName string, tblName case "unique_check_on_autoincr": // check if the database is a system database if _, ok := sysDatabases[dbName]; !ok { - ses := tcc.GetSession() - if _, ok := ses.(*backSession); ok { - return "None", nil - } - ret, err := ses.GetConfig(tcc.GetContext(), varName, dbName, tblName) - if err != nil { - return "", err - } - if ret != nil { - return ret.(string), nil - } - return "None", nil + // ses := tcc.GetSession() + // if _, ok := ses.(*backSession); ok { + // return "None", nil + // } + // ret, err := ses.GetConfig(tcc.GetContext(), varName, dbName, tblName) + // if err != nil { + // return "", err + // } + // if ret != nil { + // return ret.(string), nil + // } + // return "None", nil } return "Check", nil default: From 1160609226984d4c6f7a98802edbd8dc1588452e Mon Sep 17 00:00:00 2001 From: Jensen Date: Tue, 15 Oct 2024 16:07:43 +0800 Subject: [PATCH 3/3] add ut --- pkg/frontend/compiler_context.go | 24 +++++----- pkg/frontend/compiler_context_test.go | 69 +++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 12 deletions(-) create mode 100644 pkg/frontend/compiler_context_test.go diff --git a/pkg/frontend/compiler_context.go b/pkg/frontend/compiler_context.go index 3c01b4efc50f..2caeaf6160b0 100644 --- a/pkg/frontend/compiler_context.go +++ b/pkg/frontend/compiler_context.go @@ -253,18 +253,18 @@ func (tcc *TxnCompilerContext) GetConfig(varName string, dbName string, tblName case "unique_check_on_autoincr": // check if the database is a system database if _, ok := sysDatabases[dbName]; !ok { - // ses := tcc.GetSession() - // if _, ok := ses.(*backSession); ok { - // return "None", nil - // } - // ret, err := ses.GetConfig(tcc.GetContext(), varName, dbName, tblName) - // if err != nil { - // return "", err - // } - // if ret != nil { - // return ret.(string), nil - // } - // return "None", nil + ses := tcc.GetSession() + if _, ok := ses.(*backSession); ok { + return "None", nil + } + ret, err := ses.GetConfig(tcc.GetContext(), varName, dbName, tblName) + if err != nil { + return "", err + } + if ret != nil { + return ret.(string), nil + } + return "None", nil } return "Check", nil default: diff --git a/pkg/frontend/compiler_context_test.go b/pkg/frontend/compiler_context_test.go new file mode 100644 index 000000000000..d7387757ad74 --- /dev/null +++ b/pkg/frontend/compiler_context_test.go @@ -0,0 +1,69 @@ +// Copyright 2024 Matrix Origin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package frontend + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestGetConfig(t *testing.T) { + tcc := &TxnCompilerContext{ + execCtx: &ExecCtx{ + ses: &Session{}, + }, + } + + tests := []struct { + varName string + dbName string + tblName string + expected string + expectErr bool + }{ + { + varName: "unique_check_on_autoincr", + dbName: "test_db", + tblName: "test_tbl", + expected: "None", + expectErr: true, + }, + { + varName: "unique_check_on_autoincr", + dbName: "mo_catalog", + tblName: "test_tbl", + expected: "Check", + }, + { + varName: "invalid_var", + dbName: "test_db", + tblName: "test_tbl", + expectErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.varName, func(t *testing.T) { + val, err := tcc.GetConfig(tt.varName, tt.dbName, tt.tblName) + if tt.expectErr { + require.Error(t, err) + } else { + require.NoError(t, err) + require.Equal(t, tt.expected, val) + } + }) + } +}