From 4bad49eb3b6243a1db2a44be034ea49c2e8a5af7 Mon Sep 17 00:00:00 2001 From: lance6716 Date: Fri, 24 Jun 2022 19:54:11 +0800 Subject: [PATCH 1/5] ddl: for schema-level DDL method parameter is now XXXStmt Signed-off-by: lance6716 --- ddl/ddl.go | 4 +- ddl/ddl_api.go | 84 ++++++++++++++++--- domain/domain_test.go | 3 +- executor/ddl.go | 75 +---------------- .../realtikvtest/sessiontest/session_test.go | 2 +- 5 files changed, 77 insertions(+), 91 deletions(-) diff --git a/ddl/ddl.go b/ddl/ddl.go index d5336e360957d..fd53aeee79262 100644 --- a/ddl/ddl.go +++ b/ddl/ddl.go @@ -101,9 +101,9 @@ var ( // DDL is responsible for updating schema in data store and maintaining in-memory InfoSchema cache. type DDL interface { - CreateSchema(ctx sessionctx.Context, schema model.CIStr, charsetInfo *ast.CharsetOpt, placementPolicyRef *model.PolicyRefInfo) error + CreateSchema(ctx sessionctx.Context, stmt *ast.CreateDatabaseStmt) error AlterSchema(sctx sessionctx.Context, stmt *ast.AlterDatabaseStmt) error - DropSchema(ctx sessionctx.Context, schema model.CIStr) error + DropSchema(ctx sessionctx.Context, stmt *ast.DropDatabaseStmt) error CreateTable(ctx sessionctx.Context, stmt *ast.CreateTableStmt) error CreateView(ctx sessionctx.Context, stmt *ast.CreateViewStmt) error DropTable(ctx sessionctx.Context, tableIdent ast.Ident) (err error) diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go index 0105ef79f7d0b..03364b7e6ccfa 100644 --- a/ddl/ddl_api.go +++ b/ddl/ddl_api.go @@ -82,21 +82,76 @@ const ( tiflashCheckPendingTablesRetry = 7 ) -func (d *ddl) CreateSchema(ctx sessionctx.Context, schema model.CIStr, charsetInfo *ast.CharsetOpt, placementPolicyRef *model.PolicyRefInfo) (err error) { - dbInfo := &model.DBInfo{Name: schema} - if charsetInfo != nil { - chs, coll, err := ResolveCharsetCollation(ast.CharsetOpt{Chs: charsetInfo.Chs, Col: charsetInfo.Col}) +func (d *ddl) CreateSchema(ctx sessionctx.Context, stmt *ast.CreateDatabaseStmt) (err error) { + var placementPolicyRef *model.PolicyRefInfo + sessionVars := ctx.GetSessionVars() + + // If no charset and/or collation is specified use collation_server and character_set_server + charsetOpt := &ast.CharsetOpt{} + if sessionVars.GlobalVarsAccessor != nil { + charsetOpt.Col, err = variable.GetSessionOrGlobalSystemVar(sessionVars, variable.CollationServer) if err != nil { - return errors.Trace(err) + return err + } + charsetOpt.Chs, err = variable.GetSessionOrGlobalSystemVar(sessionVars, variable.CharacterSetServer) + if err != nil { + return err + } + } + + explicitCharset := false + explicitCollation := false + if len(stmt.Options) != 0 { + for _, val := range stmt.Options { + switch val.Tp { + case ast.DatabaseOptionCharset: + charsetOpt.Chs = val.Value + explicitCharset = true + case ast.DatabaseOptionCollate: + charsetOpt.Col = val.Value + explicitCollation = true + case ast.DatabaseOptionPlacementPolicy: + placementPolicyRef = &model.PolicyRefInfo{ + Name: model.NewCIStr(val.Value), + } + } } - dbInfo.Charset = chs - dbInfo.Collate = coll - } else { - dbInfo.Charset, dbInfo.Collate = charset.GetDefaultCharsetAndCollate() } + if charsetOpt.Col != "" { + coll, err := collate.GetCollationByName(charsetOpt.Col) + if err != nil { + return err + } + + // The collation is not valid for the specified character set. + // Try to remove any of them, but not if they are explicitly defined. + if coll.CharsetName != charsetOpt.Chs { + if explicitCollation && !explicitCharset { + // Use the explicitly set collation, not the implicit charset. + charsetOpt.Chs = "" + } + if !explicitCollation && explicitCharset { + // Use the explicitly set charset, not the (session) collation. + charsetOpt.Col = "" + } + } + + } + dbInfo := &model.DBInfo{Name: stmt.Name} + chs, coll, err := ResolveCharsetCollation(ast.CharsetOpt{Chs: charsetOpt.Chs, Col: charsetOpt.Col}) + if err != nil { + return errors.Trace(err) + } + dbInfo.Charset = chs + dbInfo.Collate = coll dbInfo.PlacementPolicyRef = placementPolicyRef - return d.CreateSchemaWithInfo(ctx, dbInfo, OnExistError) + + onExist := OnExistError + if stmt.IfNotExists { + onExist = OnExistIgnore + } + return d.CreateSchemaWithInfo(ctx, dbInfo, onExist) } func (d *ddl) CreateSchemaWithInfo( @@ -520,10 +575,13 @@ func (d *ddl) AlterSchema(sctx sessionctx.Context, stmt *ast.AlterDatabaseStmt) return nil } -func (d *ddl) DropSchema(ctx sessionctx.Context, schema model.CIStr) (err error) { +func (d *ddl) DropSchema(ctx sessionctx.Context, stmt *ast.DropDatabaseStmt) (err error) { is := d.GetInfoSchemaWithInterceptor(ctx) - old, ok := is.SchemaByName(schema) + old, ok := is.SchemaByName(stmt.Name) if !ok { + if stmt.IfExists { + return nil + } return errors.Trace(infoschema.ErrDatabaseNotExists) } job := &model.Job{ @@ -543,7 +601,7 @@ func (d *ddl) DropSchema(ctx sessionctx.Context, schema model.CIStr) (err error) return nil } // Clear table locks hold by the session. - tbs := is.SchemaTables(schema) + tbs := is.SchemaTables(stmt.Name) lockTableIDs := make([]int64, 0) for _, tb := range tbs { if ok, _ := ctx.CheckTableLocked(tb.Meta().ID); ok { diff --git a/domain/domain_test.go b/domain/domain_test.go index b5783a2b97013..468669a229ef1 100644 --- a/domain/domain_test.go +++ b/domain/domain_test.go @@ -30,7 +30,6 @@ import ( "github.com/pingcap/tidb/kv" "github.com/pingcap/tidb/metrics" "github.com/pingcap/tidb/parser/ast" - "github.com/pingcap/tidb/parser/model" "github.com/pingcap/tidb/sessionctx/variable" "github.com/pingcap/tidb/store/mockstore" "github.com/pingcap/tidb/util/mock" @@ -128,7 +127,7 @@ func TestInfo(t *testing.T) { Col: "utf8_bin", } ctx := mock.NewContext() - require.NoError(t, dom.ddl.CreateSchema(ctx, model.NewCIStr("aaa"), cs, nil)) + require.NoError(t, dom.ddl.CreateSchema(ctx, nil)) require.NoError(t, dom.Reload()) require.Equal(t, int64(1), dom.InfoSchema().SchemaMetaVersion()) diff --git a/executor/ddl.go b/executor/ddl.go index af8742fb1f901..1553be2299fd7 100644 --- a/executor/ddl.go +++ b/executor/ddl.go @@ -34,7 +34,6 @@ import ( "github.com/pingcap/tidb/table" "github.com/pingcap/tidb/table/temptable" "github.com/pingcap/tidb/util/chunk" - "github.com/pingcap/tidb/util/collate" "github.com/pingcap/tidb/util/dbterror" "github.com/pingcap/tidb/util/gcutil" "github.com/pingcap/tidb/util/logutil" @@ -248,70 +247,7 @@ func (e *DDLExec) executeRenameTable(s *ast.RenameTableStmt) error { } func (e *DDLExec) executeCreateDatabase(s *ast.CreateDatabaseStmt) error { - var opt *ast.CharsetOpt - var placementPolicyRef *model.PolicyRefInfo - var err error - sessionVars := e.ctx.GetSessionVars() - - // If no charset and/or collation is specified use collation_server and character_set_server - opt = &ast.CharsetOpt{} - if sessionVars.GlobalVarsAccessor != nil { - opt.Col, err = variable.GetSessionOrGlobalSystemVar(sessionVars, variable.CollationServer) - if err != nil { - return err - } - opt.Chs, err = variable.GetSessionOrGlobalSystemVar(sessionVars, variable.CharacterSetServer) - if err != nil { - return err - } - } - - explicitCharset := false - explicitCollation := false - if len(s.Options) != 0 { - for _, val := range s.Options { - switch val.Tp { - case ast.DatabaseOptionCharset: - opt.Chs = val.Value - explicitCharset = true - case ast.DatabaseOptionCollate: - opt.Col = val.Value - explicitCollation = true - case ast.DatabaseOptionPlacementPolicy: - placementPolicyRef = &model.PolicyRefInfo{ - Name: model.NewCIStr(val.Value), - } - } - } - } - - if opt.Col != "" { - coll, err := collate.GetCollationByName(opt.Col) - if err != nil { - return err - } - - // The collation is not valid for the specified character set. - // Try to remove any of them, but not if they are explicitly defined. - if coll.CharsetName != opt.Chs { - if explicitCollation && !explicitCharset { - // Use the explicitly set collation, not the implicit charset. - opt.Chs = "" - } - if !explicitCollation && explicitCharset { - // Use the explicitly set charset, not the (session) collation. - opt.Col = "" - } - } - - } - - err = domain.GetDomain(e.ctx).DDL().CreateSchema(e.ctx, s.Name, opt, placementPolicyRef) - if err != nil { - if infoschema.ErrDatabaseExists.Equal(err) && s.IfNotExists { - err = nil - } - } + err := domain.GetDomain(e.ctx).DDL().CreateSchema(e.ctx, s) return err } @@ -383,14 +319,7 @@ func (e *DDLExec) executeDropDatabase(s *ast.DropDatabaseStmt) error { return errors.New("Drop 'mysql' database is forbidden") } - err := domain.GetDomain(e.ctx).DDL().DropSchema(e.ctx, dbName) - if infoschema.ErrDatabaseNotExists.Equal(err) { - if s.IfExists { - err = nil - } else { - err = infoschema.ErrDatabaseDropExists.GenWithStackByArgs(s.Name) - } - } + err := domain.GetDomain(e.ctx).DDL().DropSchema(e.ctx, s) sessionVars := e.ctx.GetSessionVars() if err == nil && strings.ToLower(sessionVars.CurrentDB) == dbName.L { sessionVars.CurrentDB = "" diff --git a/tests/realtikvtest/sessiontest/session_test.go b/tests/realtikvtest/sessiontest/session_test.go index 4bc983d94b40f..c883918f6b889 100644 --- a/tests/realtikvtest/sessiontest/session_test.go +++ b/tests/realtikvtest/sessiontest/session_test.go @@ -1363,7 +1363,7 @@ func TestDoDDLJobQuit(t *testing.T) { defer func() { require.NoError(t, failpoint.Disable("github.com/pingcap/tidb/ddl/storeCloseInLoop")) }() // this DDL call will enter deadloop before this fix - err = dom.DDL().CreateSchema(se, model.NewCIStr("testschema"), nil, nil) + err = dom.DDL().CreateSchema(se, nil) require.Equal(t, "context canceled", err.Error()) } From 6b1525e062ad78b5493f92445880a723ff05fa4d Mon Sep 17 00:00:00 2001 From: lance6716 Date: Fri, 24 Jun 2022 20:12:38 +0800 Subject: [PATCH 2/5] fix test Signed-off-by: lance6716 --- domain/domain_test.go | 20 ++++++++++++++----- .../realtikvtest/sessiontest/session_test.go | 3 ++- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/domain/domain_test.go b/domain/domain_test.go index 468669a229ef1..98776381d1ae8 100644 --- a/domain/domain_test.go +++ b/domain/domain_test.go @@ -30,6 +30,7 @@ import ( "github.com/pingcap/tidb/kv" "github.com/pingcap/tidb/metrics" "github.com/pingcap/tidb/parser/ast" + "github.com/pingcap/tidb/parser/model" "github.com/pingcap/tidb/sessionctx/variable" "github.com/pingcap/tidb/store/mockstore" "github.com/pingcap/tidb/util/mock" @@ -121,13 +122,22 @@ func TestInfo(t *testing.T) { } require.True(t, syncerStarted) - // Make sure loading schema is normal. - cs := &ast.CharsetOpt{ - Chs: "utf8", - Col: "utf8_bin", + stmt := &ast.CreateDatabaseStmt{ + Name: model.NewCIStr("aaa"), + // Make sure loading schema is normal. + Options: []*ast.DatabaseOption{ + { + Tp: ast.DatabaseOptionCharset, + Value: "utf8", + }, + { + Tp: ast.DatabaseOptionCollate, + Value: "utf8_bin", + }, + }, } ctx := mock.NewContext() - require.NoError(t, dom.ddl.CreateSchema(ctx, nil)) + require.NoError(t, dom.ddl.CreateSchema(ctx, stmt)) require.NoError(t, dom.Reload()) require.Equal(t, int64(1), dom.InfoSchema().SchemaMetaVersion()) diff --git a/tests/realtikvtest/sessiontest/session_test.go b/tests/realtikvtest/sessiontest/session_test.go index c883918f6b889..9262ebe196498 100644 --- a/tests/realtikvtest/sessiontest/session_test.go +++ b/tests/realtikvtest/sessiontest/session_test.go @@ -30,6 +30,7 @@ import ( "github.com/pingcap/tidb/domain" "github.com/pingcap/tidb/errno" "github.com/pingcap/tidb/kv" + "github.com/pingcap/tidb/parser/ast" "github.com/pingcap/tidb/parser/auth" "github.com/pingcap/tidb/parser/format" "github.com/pingcap/tidb/parser/model" @@ -1363,7 +1364,7 @@ func TestDoDDLJobQuit(t *testing.T) { defer func() { require.NoError(t, failpoint.Disable("github.com/pingcap/tidb/ddl/storeCloseInLoop")) }() // this DDL call will enter deadloop before this fix - err = dom.DDL().CreateSchema(se, nil) + err = dom.DDL().CreateSchema(se, &ast.CreateDatabaseStmt{Name: model.NewCIStr("testschema")}) require.Equal(t, "context canceled", err.Error()) } From 3ec01c86ec148a6b55c99b7914bb5be6accf0873 Mon Sep 17 00:00:00 2001 From: lance6716 Date: Fri, 24 Jun 2022 21:49:00 +0800 Subject: [PATCH 3/5] fix CI --- ddl/ddl_api.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go index 03364b7e6ccfa..2f47f5ab2b842 100644 --- a/ddl/ddl_api.go +++ b/ddl/ddl_api.go @@ -595,6 +595,9 @@ func (d *ddl) DropSchema(ctx sessionctx.Context, stmt *ast.DropDatabaseStmt) (er err = d.DoDDLJob(ctx, job) err = d.callHookOnChanged(job, err) if err != nil { + if infoschema.ErrDatabaseNotExists.Equal(err) && stmt.IfExists { + return nil + } return errors.Trace(err) } if !config.TableLockEnabled() { From e8cdfe8c6077a5103ee64d452b8cf09110b5d0e3 Mon Sep 17 00:00:00 2001 From: lance6716 Date: Fri, 24 Jun 2022 22:08:15 +0800 Subject: [PATCH 4/5] error name looks same :P --- ddl/ddl_api.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go index 2f47f5ab2b842..0376763a4bee5 100644 --- a/ddl/ddl_api.go +++ b/ddl/ddl_api.go @@ -582,7 +582,7 @@ func (d *ddl) DropSchema(ctx sessionctx.Context, stmt *ast.DropDatabaseStmt) (er if stmt.IfExists { return nil } - return errors.Trace(infoschema.ErrDatabaseNotExists) + return infoschema.ErrDatabaseDropExists.GenWithStackByArgs(stmt.Name) } job := &model.Job{ SchemaID: old.ID, @@ -595,8 +595,11 @@ func (d *ddl) DropSchema(ctx sessionctx.Context, stmt *ast.DropDatabaseStmt) (er err = d.DoDDLJob(ctx, job) err = d.callHookOnChanged(job, err) if err != nil { - if infoschema.ErrDatabaseNotExists.Equal(err) && stmt.IfExists { - return nil + if infoschema.ErrDatabaseNotExists.Equal(err) { + if stmt.IfExists { + return nil + } + return infoschema.ErrDatabaseDropExists.GenWithStackByArgs(stmt.Name) } return errors.Trace(err) } From 99f40ae5109b915bf1bbaee4930dc15623d1e5c6 Mon Sep 17 00:00:00 2001 From: lance6716 Date: Fri, 24 Jun 2022 22:47:16 +0800 Subject: [PATCH 5/5] fix test for CREATE DATABASE; --- ddl/ddl_api.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go index 0376763a4bee5..948457f499444 100644 --- a/ddl/ddl_api.go +++ b/ddl/ddl_api.go @@ -202,6 +202,12 @@ func (d *ddl) CreateSchemaWithInfo( err = d.DoDDLJob(ctx, job) err = d.callHookOnChanged(job, err) + + if infoschema.ErrDatabaseExists.Equal(err) && onExist == OnExistIgnore { + ctx.GetSessionVars().StmtCtx.AppendNote(err) + return nil + } + return errors.Trace(err) }