Skip to content

Commit

Permalink
ddl: fix issue that recover table by job id may cause panic (#56965) (#…
Browse files Browse the repository at this point in the history
  • Loading branch information
ti-chi-bot authored Dec 10, 2024
1 parent 3fbde1a commit c18cdd0
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 2 deletions.
1 change: 1 addition & 0 deletions pkg/executor/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,7 @@ go_test(
"//pkg/ddl",
"//pkg/ddl/placement",
"//pkg/ddl/util",
"//pkg/ddl/util/callback",
"//pkg/distsql",
"//pkg/domain",
"//pkg/domain/infosync",
Expand Down
2 changes: 1 addition & 1 deletion pkg/executor/ddl.go
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,7 @@ func (e *DDLExec) executeRecoverTable(s *ast.RecoverTableStmt) error {
// Check the table ID was not exists.
tbl, ok := dom.InfoSchema().TableByID(tblInfo.ID)
if ok {
return infoschema.ErrTableExists.GenWithStack("Table '%-.192s' already been recover to '%-.192s', can't be recover repeatedly", s.Table.Name.O, tbl.Meta().Name.O)
return infoschema.ErrTableExists.GenWithStack("Table '%-.192s' already been recover to '%-.192s', can't be recover repeatedly", tblInfo.Name.O, tbl.Meta().Name.O)
}

m := domain.GetDomain(e.Ctx()).GetSnapshotMeta(job.StartTS)
Expand Down
24 changes: 23 additions & 1 deletion pkg/executor/recover_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,17 @@ package executor_test
import (
"context"
"fmt"
"strconv"
"testing"
"time"

"github.com/pingcap/failpoint"
ddlutil "github.com/pingcap/tidb/pkg/ddl/util"
"github.com/pingcap/tidb/pkg/ddl/util/callback"
"github.com/pingcap/tidb/pkg/errno"
"github.com/pingcap/tidb/pkg/infoschema"
"github.com/pingcap/tidb/pkg/parser/auth"
"github.com/pingcap/tidb/pkg/parser/model"
"github.com/pingcap/tidb/pkg/sessionctx/variable"
"github.com/pingcap/tidb/pkg/testkit"
"github.com/pingcap/tidb/pkg/util/dbterror"
Expand All @@ -40,7 +43,7 @@ func TestRecoverTable(t *testing.T) {
require.NoError(t, failpoint.Disable("github.com/pingcap/tidb/pkg/meta/autoid/mockAutoIDChange"))
}()

store := testkit.CreateMockStore(t)
store, dom := testkit.CreateMockStoreAndDomain(t)
tk := testkit.NewTestKit(t, store)
tk.MustExec("create database if not exists test_recover")
tk.MustExec("use test_recover")
Expand Down Expand Up @@ -124,6 +127,25 @@ func TestRecoverTable(t *testing.T) {
err = tk.ExecToErr("recover table t_recover")
require.True(t, infoschema.ErrTableExists.Equal(err))

// Test drop table failed and then recover the table should also be failed.
tk.MustExec("drop table if exists t_recover2")
tk.MustExec("create table t_recover2 (a int);")
jobID := int64(0)

hook := &callback.TestDDLCallback{Do: dom}
hook.OnJobRunBeforeExported = func(job *model.Job) {
if job.Type == model.ActionDropTable && jobID == 0 {
jobID = job.ID
}
}
dom.DDL().SetHook(hook)

tk.MustExec("drop table t_recover2")
tk.MustExec("recover table by job " + strconv.Itoa(int(jobID)))
err = tk.ExecToErr("recover table by job " + strconv.Itoa(int(jobID)))
require.Error(t, err)
require.Equal(t, "[schema:1050]Table 't_recover2' already been recover to 't_recover2', can't be recover repeatedly", err.Error())

gcEnable, err := gcutil.CheckGCEnable(tk.Session())
require.NoError(t, err)
require.False(t, gcEnable)
Expand Down

0 comments on commit c18cdd0

Please sign in to comment.