Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ddl: migrate part of ddl package code from Execute/ExecRestricted to safe API (1) #22670

Merged
merged 17 commits into from
Feb 5, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions ddl/column.go
Original file line number Diff line number Diff line change
Expand Up @@ -939,7 +939,12 @@ func (w *worker) doModifyColumnTypeWithData(
}
defer w.sessPool.put(ctx)

_, _, err = ctx.(sqlexec.RestrictedSQLExecutor).ExecRestrictedSQL(valStr)
stmt, err := ctx.(sqlexec.RestrictedSQLExecutor).ParseWithParams(context.Background(), valStr)
if err != nil {
job.State = model.JobStateCancelled
failpoint.Return(ver, err)
}
_, _, err = ctx.(sqlexec.RestrictedSQLExecutor).ExecRestrictedStmt(context.Background(), stmt)
if err != nil {
job.State = model.JobStateCancelled
failpoint.Return(ver, err)
Expand Down Expand Up @@ -1526,15 +1531,23 @@ func checkAndApplyNewAutoRandomBits(job *model.Job, t *meta.Meta, tblInfo *model
// `isDataTruncated` indicates whether the new field and the old field type are the same, in order to be compatible with mysql.
func checkForNullValue(ctx sessionctx.Context, isDataTruncated bool, schema, table, newCol model.CIStr, oldCols ...*model.ColumnInfo) error {
colsStr := ""
paramsList := make([]interface{}, 0, 2+len(oldCols))
paramsList = append(paramsList, schema.L, table.L)
for i, col := range oldCols {
if i == 0 {
colsStr += "`" + col.Name.L + "` is null"
colsStr += "%n is null"
paramsList = append(paramsList, col.Name.L)
} else {
colsStr += " or `" + col.Name.L + "` is null"
colsStr += " or %n is null"
paramsList = append(paramsList, col.Name.L)
}
}
sql := fmt.Sprintf("select 1 from `%s`.`%s` where %s limit 1;", schema.L, table.L, colsStr)
rows, _, err := ctx.(sqlexec.RestrictedSQLExecutor).ExecRestrictedSQL(sql)
sql := "select 1 from %n.%n where " + colsStr + " limit 1;"
stmt, err := ctx.(sqlexec.RestrictedSQLExecutor).ParseWithParams(context.Background(), sql, paramsList...)
Copy link
Contributor

@xhebox xhebox Feb 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not considered safe to use concat and ParseWithParams at the same time. Though this case is safe.

fmt.Sprintf(fmt.Sprintf("%s", "%s")) will report an error.

You may want to check this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good~
the PR link is not right

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, here it is: 22674

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest that you can use fmt.Fprintf.

if err != nil {
return errors.Trace(err)
}
rows, _, err := ctx.(sqlexec.RestrictedSQLExecutor).ExecRestrictedStmt(context.Background(), stmt)
if err != nil {
return errors.Trace(err)
}
Expand Down
2 changes: 1 addition & 1 deletion ddl/db_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ func setupIntegrationSuite(s *testIntegrationSuite, c *C) {
se, err := session.CreateSession4Test(s.store)
c.Assert(err, IsNil)
s.ctx = se.(sessionctx.Context)
_, err = se.Execute(context.Background(), "create database test_db")
_, err = se.ExecuteInternal(context.Background(), "create database test_db")
c.Assert(err, IsNil)
}

Expand Down
6 changes: 3 additions & 3 deletions ddl/db_partition_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3185,7 +3185,7 @@ func (s *testIntegrationSuite7) TestCommitWhenSchemaChange(c *C) {
defer func() {
atomic.StoreUint32(&session.SchemaChangedWithoutRetry, 0)
}()
_, err := tk.Se.Execute(context.Background(), "commit")
_, err := tk.Se.ExecuteInternal(context.Background(), "commit")
c.Assert(domain.ErrInfoSchemaChanged.Equal(err), IsTrue)

// Cover a bug that schema validator does not prevent transaction commit when
Expand All @@ -3203,7 +3203,7 @@ func (s *testIntegrationSuite7) TestCommitWhenSchemaChange(c *C) {
tk.MustExec("insert into nt values (1), (3), (5);")
tk2.MustExec("alter table pt exchange partition p1 with table nt;")
tk.MustExec("insert into nt values (7), (9);")
_, err = tk.Se.Execute(context.Background(), "commit")
_, err = tk.Se.ExecuteInternal(context.Background(), "commit")
c.Assert(domain.ErrInfoSchemaChanged.Equal(err), IsTrue)

tk.MustExec("admin check table pt")
Expand All @@ -3215,7 +3215,7 @@ func (s *testIntegrationSuite7) TestCommitWhenSchemaChange(c *C) {
tk.MustExec("insert into pt values (1), (3), (5);")
tk2.MustExec("alter table pt exchange partition p1 with table nt;")
tk.MustExec("insert into pt values (7), (9);")
_, err = tk.Se.Execute(context.Background(), "commit")
_, err = tk.Se.ExecuteInternal(context.Background(), "commit")
c.Assert(domain.ErrInfoSchemaChanged.Equal(err), IsTrue)

tk.MustExec("admin check table pt")
Expand Down
20 changes: 10 additions & 10 deletions ddl/db_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,13 +115,13 @@ func setUpSuite(s *testDBSuite, c *C) {
s.s, err = session.CreateSession4Test(s.store)
c.Assert(err, IsNil)

_, err = s.s.Execute(context.Background(), "create database test_db")
_, err = s.s.ExecuteInternal(context.Background(), "create database test_db")
c.Assert(err, IsNil)
s.s.Execute(context.Background(), "set @@global.tidb_max_delta_schema_count= 4096")
s.s.ExecuteInternal(context.Background(), "set @@global.tidb_max_delta_schema_count= 4096")
}

func tearDownSuite(s *testDBSuite, c *C) {
s.s.Execute(context.Background(), "drop database if exists test_db")
s.s.ExecuteInternal(context.Background(), "drop database if exists test_db")
s.s.Close()
s.dom.Close()
s.store.Close()
Expand Down Expand Up @@ -277,12 +277,12 @@ func backgroundExec(s kv.Storage, sql string, done chan error) {
return
}
defer se.Close()
_, err = se.Execute(context.Background(), "use test_db")
_, err = se.ExecuteInternal(context.Background(), "use test_db")
if err != nil {
done <- errors.Trace(err)
return
}
_, err = se.Execute(context.Background(), sql)
_, err = se.ExecuteInternal(context.Background(), sql)
done <- errors.Trace(err)
}

Expand Down Expand Up @@ -2048,9 +2048,9 @@ func (s *testDBSuite1) TestColumn(c *C) {
func sessionExec(c *C, s kv.Storage, sql string) {
se, err := session.CreateSession4Test(s)
c.Assert(err, IsNil)
_, err = se.Execute(context.Background(), "use test_db")
_, err = se.ExecuteInternal(context.Background(), "use test_db")
c.Assert(err, IsNil)
rs, err := se.Execute(context.Background(), sql)
rs, err := se.ExecuteInternal(context.Background(), sql)
c.Assert(err, IsNil, Commentf("err:%v", errors.ErrorStack(err)))
c.Assert(rs, IsNil)
se.Close()
Expand Down Expand Up @@ -2420,7 +2420,7 @@ func (s *testDBSuite5) TestRenameColumn(c *C) {
}

func (s *testDBSuite7) TestSelectInViewFromAnotherDB(c *C) {
_, _ = s.s.Execute(context.Background(), "create database test_db2")
_, _ = s.s.ExecuteInternal(context.Background(), "create database test_db2")
tk := testkit.NewTestKit(c, s.store)
tk.MustExec("use " + s.schemaName)
tk.MustExec("create table t(a int)")
Expand Down Expand Up @@ -5770,12 +5770,12 @@ func (s *testDBSuite4) testParallelExecSQL(c *C, sql1, sql2 string, se1, se2 ses
}()
go func() {
defer wg.Done()
_, err1 = se1.Execute(context.Background(), sql1)
_, err1 = se1.ExecuteInternal(context.Background(), sql1)
}()
go func() {
defer wg.Done()
<-ch
_, err2 = se2.Execute(context.Background(), sql2)
_, err2 = se2.ExecuteInternal(context.Background(), sql2)
}()

wg.Wait()
Expand Down
2 changes: 1 addition & 1 deletion ddl/ddl_worker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1446,5 +1446,5 @@ func (s *testDDLSuite) TestDDLPackageExecuteSQL(c *C) {
c.Assert(err, IsNil)
defer worker.sessPool.put(sess)
se := sess.(sqlexec.SQLExecutor)
_, _ = se.Execute(context.Background(), "create table t(a int);")
_, _ = se.ExecuteInternal(context.Background(), "create table t(a int);")
}
2 changes: 1 addition & 1 deletion ddl/failtest/fail_db_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ func (s *testFailDBSuite) SetUpSuite(c *C) {
}

func (s *testFailDBSuite) TearDownSuite(c *C) {
_, err := s.se.Execute(context.Background(), "drop database if exists test_db_state")
_, err := s.se.ExecuteInternal(context.Background(), "drop database if exists test_db_state")
c.Assert(err, IsNil)
s.se.Close()
s.dom.Close()
Expand Down
4 changes: 2 additions & 2 deletions ddl/testutil/testutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,13 @@ func ExecMultiSQLInGoroutine(c *check.C, s kv.Storage, dbName string, multiSQL [
return
}
defer se.Close()
_, err = se.Execute(context.Background(), "use "+dbName)
_, err = se.ExecuteInternal(context.Background(), "use %n", dbName)
if err != nil {
done <- errors.Trace(err)
return
}
for _, sql := range multiSQL {
rs, err := se.Execute(context.Background(), sql)
AilinKid marked this conversation as resolved.
Show resolved Hide resolved
rs, err := se.ExecuteInternal(context.Background(), sql)
if err != nil {
done <- errors.Trace(err)
return
Expand Down
44 changes: 20 additions & 24 deletions ddl/util/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,12 @@ import (
const (
deleteRangesTable = `gc_delete_range`
doneDeleteRangesTable = `gc_delete_range_done`
loadDeleteRangeSQL = `SELECT HIGH_PRIORITY job_id, element_id, start_key, end_key FROM mysql.%s WHERE ts < %v`
recordDoneDeletedRangeSQL = `INSERT IGNORE INTO mysql.gc_delete_range_done SELECT * FROM mysql.gc_delete_range WHERE job_id = %d AND element_id = %d`
completeDeleteRangeSQL = `DELETE FROM mysql.gc_delete_range WHERE job_id = %d AND element_id = %d`
completeDeleteMultiRangesSQL = `DELETE FROM mysql.gc_delete_range WHERE job_id = %d AND element_id in (%v)`
updateDeleteRangeSQL = `UPDATE mysql.gc_delete_range SET start_key = "%s" WHERE job_id = %d AND element_id = %d AND start_key = "%s"`
deleteDoneRecordSQL = `DELETE FROM mysql.gc_delete_range_done WHERE job_id = %d AND element_id = %d`
loadDeleteRangeSQL = `SELECT HIGH_PRIORITY job_id, element_id, start_key, end_key FROM mysql.%n WHERE ts < %?`
recordDoneDeletedRangeSQL = `INSERT IGNORE INTO mysql.gc_delete_range_done SELECT * FROM mysql.gc_delete_range WHERE job_id = %? AND element_id = %?`
completeDeleteRangeSQL = `DELETE FROM mysql.gc_delete_range WHERE job_id = %? AND element_id = %?`
completeDeleteMultiRangesSQL = `DELETE FROM mysql.gc_delete_range WHERE job_id = %? AND element_id in (%?)`
updateDeleteRangeSQL = `UPDATE mysql.gc_delete_range SET start_key = "%?" WHERE job_id = %? AND element_id = %? AND start_key = "%?"`
Copy link
Contributor

@xhebox xhebox Feb 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete the quotes around %?.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch

deleteDoneRecordSQL = `DELETE FROM mysql.gc_delete_range_done WHERE job_id = %? AND element_id = %?`
)

// DelRangeTask is for run delete-range command in gc_worker.
Expand All @@ -62,16 +62,14 @@ func LoadDoneDeleteRanges(ctx sessionctx.Context, safePoint uint64) (ranges []De
}

func loadDeleteRangesFromTable(ctx sessionctx.Context, table string, safePoint uint64) (ranges []DelRangeTask, _ error) {
sql := fmt.Sprintf(loadDeleteRangeSQL, table, safePoint)
rss, err := ctx.(sqlexec.SQLExecutor).Execute(context.TODO(), sql)
if len(rss) > 0 {
defer terror.Call(rss[0].Close)
rs, err := ctx.(sqlexec.SQLExecutor).ExecuteInternal(context.TODO(), loadDeleteRangeSQL, table, safePoint)
if rs != nil {
defer terror.Call(rs.Close)
}
if err != nil {
return nil, errors.Trace(err)
}

rs := rss[0]
req := rs.NewChunk()
it := chunk.NewIterator4Chunk(req)
for {
Expand Down Expand Up @@ -106,8 +104,7 @@ func loadDeleteRangesFromTable(ctx sessionctx.Context, table string, safePoint u
// CompleteDeleteRange moves a record from gc_delete_range table to gc_delete_range_done table.
// NOTE: This function WILL NOT start and run in a new transaction internally.
func CompleteDeleteRange(ctx sessionctx.Context, dr DelRangeTask) error {
sql := fmt.Sprintf(recordDoneDeletedRangeSQL, dr.JobID, dr.ElementID)
_, err := ctx.(sqlexec.SQLExecutor).Execute(context.TODO(), sql)
_, err := ctx.(sqlexec.SQLExecutor).ExecuteInternal(context.TODO(), recordDoneDeletedRangeSQL, dr.JobID, dr.ElementID)
if err != nil {
return errors.Trace(err)
}
Expand All @@ -117,8 +114,7 @@ func CompleteDeleteRange(ctx sessionctx.Context, dr DelRangeTask) error {

// RemoveFromGCDeleteRange is exported for ddl pkg to use.
func RemoveFromGCDeleteRange(ctx sessionctx.Context, jobID, elementID int64) error {
sql := fmt.Sprintf(completeDeleteRangeSQL, jobID, elementID)
_, err := ctx.(sqlexec.SQLExecutor).Execute(context.TODO(), sql)
_, err := ctx.(sqlexec.SQLExecutor).ExecuteInternal(context.TODO(), completeDeleteRangeSQL, jobID, elementID)
return errors.Trace(err)
}

Expand All @@ -131,24 +127,21 @@ func RemoveMultiFromGCDeleteRange(ctx sessionctx.Context, jobID int64, elementID
}
buf.WriteString(strconv.FormatInt(elementID, 10))
}
sql := fmt.Sprintf(completeDeleteMultiRangesSQL, jobID, buf.String())
_, err := ctx.(sqlexec.SQLExecutor).Execute(context.TODO(), sql)
_, err := ctx.(sqlexec.SQLExecutor).ExecuteInternal(context.TODO(), completeDeleteMultiRangesSQL, jobID, buf.String())
AilinKid marked this conversation as resolved.
Show resolved Hide resolved
return errors.Trace(err)
}

// DeleteDoneRecord removes a record from gc_delete_range_done table.
func DeleteDoneRecord(ctx sessionctx.Context, dr DelRangeTask) error {
sql := fmt.Sprintf(deleteDoneRecordSQL, dr.JobID, dr.ElementID)
_, err := ctx.(sqlexec.SQLExecutor).Execute(context.TODO(), sql)
_, err := ctx.(sqlexec.SQLExecutor).ExecuteInternal(context.TODO(), deleteDoneRecordSQL, dr.JobID, dr.ElementID)
return errors.Trace(err)
}

// UpdateDeleteRange is only for emulator.
func UpdateDeleteRange(ctx sessionctx.Context, dr DelRangeTask, newStartKey, oldStartKey kv.Key) error {
newStartKeyHex := hex.EncodeToString(newStartKey)
oldStartKeyHex := hex.EncodeToString(oldStartKey)
sql := fmt.Sprintf(updateDeleteRangeSQL, newStartKeyHex, dr.JobID, dr.ElementID, oldStartKeyHex)
_, err := ctx.(sqlexec.SQLExecutor).Execute(context.TODO(), sql)
_, err := ctx.(sqlexec.SQLExecutor).ExecuteInternal(context.TODO(), updateDeleteRangeSQL, newStartKeyHex, dr.JobID, dr.ElementID, oldStartKeyHex)
return errors.Trace(err)
}

Expand All @@ -164,7 +157,7 @@ func LoadDDLVars(ctx sessionctx.Context) error {
return LoadGlobalVars(ctx, []string{variable.TiDBDDLErrorCountLimit})
}

const loadGlobalVarsSQL = "select HIGH_PRIORITY variable_name, variable_value from mysql.global_variables where variable_name in (%s)"
const loadGlobalVarsSQL = "select HIGH_PRIORITY variable_name, variable_value from mysql.global_variables where variable_name in (%?)"

// LoadGlobalVars loads global variable from mysql.global_variables.
func LoadGlobalVars(ctx sessionctx.Context, varNames []string) error {
Expand All @@ -176,8 +169,11 @@ func LoadGlobalVars(ctx sessionctx.Context, varNames []string) error {
}
nameList += fmt.Sprintf("'%s'", name)
}
sql := fmt.Sprintf(loadGlobalVarsSQL, nameList)
rows, _, err := sctx.ExecRestrictedSQL(sql)
stmt, err := sctx.ParseWithParams(context.Background(), loadGlobalVarsSQL, nameList)
AilinKid marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return errors.Trace(err)
}
rows, _, err := sctx.ExecRestrictedStmt(context.Background(), stmt)
if err != nil {
return errors.Trace(err)
}
Expand Down