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: support alter table auto_increment #5472

Merged
merged 12 commits into from
Dec 27, 2017
35 changes: 35 additions & 0 deletions ddl/ddl_api.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"strings"
"time"

"github.com/cznic/mathutil"
"github.com/juju/errors"
"github.com/pingcap/tidb/ast"
"github.com/pingcap/tidb/context"
Expand Down Expand Up @@ -885,6 +886,13 @@ func (d *ddl) AlterTable(ctx context.Context, ident ast.Ident, specs []*ast.Alte
err = d.RenameTable(ctx, ident, newIdent)
case ast.AlterTableDropPrimaryKey:
err = ErrUnsupportedModifyPrimaryKey.GenByArgs("drop")
case ast.AlterTableOption:
for _, opt := range spec.Options {
if opt.Tp == ast.TableOptionAutoIncrement {
err = d.RebaseAutoID(ctx, ident, int64(opt.UintValue))
break
}
}
default:
// Nothing to do now.
}
Expand All @@ -897,6 +905,33 @@ func (d *ddl) AlterTable(ctx context.Context, ident ast.Ident, specs []*ast.Alte
return nil
}

func (d *ddl) RebaseAutoID(ctx context.Context, ident ast.Ident, newBase int64) error {
is := d.GetInformationSchema()
schema, ok := is.SchemaByName(ident.Schema)
if !ok {
return infoschema.ErrDatabaseNotExists.GenByArgs(ident.Schema)
}
t, err := is.TableByName(ident.Schema, ident.Name)
if err != nil {
return errors.Trace(infoschema.ErrTableNotExists.GenByArgs(ident.Schema, ident.Name))
}
autoIncID, err := t.Allocator(ctx).NextGlobalAutoID(t.Meta().ID)
Copy link
Contributor

Choose a reason for hiding this comment

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

We'd can put this logical to onRebaseAutoID。Because now autoIncID is n,its value may change when you rebase the global auto ID in onRebaseAutoID

Copy link
Member Author

Choose a reason for hiding this comment

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

There is another check in storage node to guarantee the value be set properly. The comparison here is trying to set a more reasonable value in table info.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems that the global auto-id is not the current max id, for example, if the current max id is 1, the new base is 1000, then the next id inserted will be 5001. Is it desirable?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is our expected result. I'll add a test for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. PTAL @lamxTyler

if err != nil {
return errors.Trace(err)
}
newBase = mathutil.MaxInt64(newBase, autoIncID)
job := &model.Job{
SchemaID: schema.ID,
TableID: t.Meta().ID,
Type: model.ActionRebaseAutoID,
BinlogInfo: &model.HistoryInfo{},
Args: []interface{}{newBase},
}
err = d.doDDLJob(ctx, job)
err = d.callHookOnChanged(err)
return errors.Trace(err)
}

func checkColumnConstraint(constraints []*ast.ColumnOption) error {
for _, constraint := range constraints {
switch constraint.Tp {
Expand Down
28 changes: 28 additions & 0 deletions ddl/ddl_db_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1676,3 +1676,31 @@ func (s *testDBSuite) TestComment(c *C) {

s.tk.MustExec("drop table if exists ct, ct1")
}

func (s *testDBSuite) TestRebaseAutoID(c *C) {
s.tk = testkit.NewTestKit(c, s.store)
s.tk.MustExec("use " + s.schemaName)

s.tk.MustExec("drop database if exists tidb;")
s.tk.MustExec("create database tidb;")
s.tk.MustExec("use tidb;")
s.tk.MustExec("create table tidb.test (a int auto_increment primary key, b int);")
s.tk.MustExec("insert tidb.test values (null, 1);")
s.tk.MustQuery("select * from tidb.test").Check(testkit.Rows("1 1"))
s.tk.MustExec("alter table tidb.test auto_increment = 6000;")
s.tk.MustExec("insert tidb.test values (null, 1);")
s.tk.MustQuery("select * from tidb.test").Check(testkit.Rows("1 1", "6000 1"))
s.tk.MustExec("alter table tidb.test auto_increment = 5;")
s.tk.MustExec("insert tidb.test values (null, 1);")
s.tk.MustQuery("select * from tidb.test").Check(testkit.Rows("1 1", "6000 1", "11000 1"))

// Current range for table test is [11000, 15999].
// Though it does not have a tuple "a = 15999", its global next auto increment id should be 16000.
// Anyway it is not compatible with MySQL.
s.tk.MustExec("alter table tidb.test auto_increment = 12000;")
s.tk.MustExec("insert tidb.test values (null, 1);")
s.tk.MustQuery("select * from tidb.test").Check(testkit.Rows("1 1", "6000 1", "11000 1", "16000 1"))

s.tk.MustExec("create table tidb.test2 (a int);")
s.testErrorCode(c, "alter table tidb.test2 add column b int auto_increment key, auto_increment=10;", tmysql.ErrUnknown)
}
2 changes: 2 additions & 0 deletions ddl/ddl_worker.go
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,8 @@ func (d *ddl) runDDLJob(t *meta.Meta, job *model.Job) (ver int64) {
ver, err = d.onDropForeignKey(t, job)
case model.ActionTruncateTable:
ver, err = d.onTruncateTable(t, job)
case model.ActionRebaseAutoID:
ver, err = d.onRebaseAutoID(t, job)
case model.ActionRenameTable:
ver, err = d.onRenameTable(t, job)
case model.ActionSetDefaultValue:
Expand Down
37 changes: 37 additions & 0 deletions ddl/table.go
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,43 @@ func (d *ddl) onTruncateTable(t *meta.Meta, job *model.Job) (ver int64, _ error)
return ver, nil
}

func (d *ddl) onRebaseAutoID(t *meta.Meta, job *model.Job) (ver int64, _ error) {
schemaID := job.SchemaID
var newBase int64
err := job.DecodeArgs(&newBase)
if err != nil {
job.State = model.JobStateCancelled
return ver, errors.Trace(err)
}
tblInfo, err := getTableInfo(t, job, schemaID)
if err != nil {
job.State = model.JobStateCancelled
return ver, errors.Trace(err)
}
tblInfo.AutoIncID = newBase
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check that if the new auto-id is not less than current id?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, though it will be checked in storage, it should set a more properly info here. Fixed.

tbl, err := d.getTable(schemaID, tblInfo)
if err != nil {
job.State = model.JobStateCancelled
return ver, errors.Trace(err)
}
// The operation of the minus 1 to make sure that the current value doesn't be used,
// the next Alloc operation will get this value.
// Its behavior is consistent with MySQL.
err = tbl.RebaseAutoID(nil, tblInfo.AutoIncID-1, false)
if err != nil {
job.State = model.JobStateCancelled
return ver, errors.Trace(err)
}
job.State = model.JobStateDone
job.BinlogInfo.AddTableInfo(ver, tblInfo)
ver, err = updateTableInfo(t, job, tblInfo, tblInfo.State)
if err != nil {
job.State = model.JobStateCancelled
return ver, errors.Trace(err)
}
return ver, nil
}

func (d *ddl) onRenameTable(t *meta.Meta, job *model.Job) (ver int64, _ error) {
var oldSchemaID int64
var tableName model.CIStr
Expand Down
2 changes: 1 addition & 1 deletion infoschema/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ func (b *Builder) ApplyDiff(m *meta.Meta, diff *model.SchemaDiff) ([]int64, erro
// We try to reuse the old allocator, so the cached auto ID can be reused.
var alloc autoid.Allocator
if tableIDIsValid(oldTableID) {
if oldTableID == newTableID && diff.Type != model.ActionRenameTable {
if oldTableID == newTableID && diff.Type != model.ActionRenameTable && diff.Type != model.ActionRebaseAutoID {
alloc, _ = b.is.AllocByID(oldTableID)
}
if diff.Type == model.ActionRenameTable {
Expand Down
3 changes: 3 additions & 0 deletions model/ddl.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ const (
ActionDropForeignKey
ActionTruncateTable
ActionModifyColumn
ActionRebaseAutoID
ActionRenameTable
ActionSetDefaultValue
)
Expand Down Expand Up @@ -70,6 +71,8 @@ func (action ActionType) String() string {
return "truncate table"
case ActionModifyColumn:
return "modify column"
case ActionRebaseAutoID:
return "rebase auto_increment ID"
case ActionRenameTable:
return "rename table"
case ActionSetDefaultValue:
Expand Down
7 changes: 0 additions & 7 deletions plan/preprocess.go
Original file line number Diff line number Diff line change
Expand Up @@ -333,13 +333,6 @@ func (p *preprocessor) checkAlterTableGrammar(stmt *ast.AlterTableStmt) {
default:
// Nothing to do now.
}
case ast.AlterTableOption:
for _, opt := range spec.Options {
if opt.Tp == ast.TableOptionAutoIncrement {
p.err = ErrAlterAutoID
return
}
}
default:
// Nothing to do now.
}
Expand Down
7 changes: 3 additions & 4 deletions plan/preprocess_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,8 @@ func (s *testValidatorSuite) TestValidator(c *C) {
errors.New("[schema:1068]Multiple primary key defined")},
{"create table t(c1 int not null, c2 int not null, primary key(c1), primary key(c2))", true,
errors.New("[schema:1068]Multiple primary key defined")},
{"alter table t auto_increment=1", true, errors.New("[autoid:3]No support for setting auto_increment using alter_table")},
{"alter table t add column c int auto_increment key, auto_increment=10", true,
errors.New("[autoid:3]No support for setting auto_increment using alter_table")},
{"alter table t auto_increment=1", true, nil},
{"alter table t add column c int auto_increment key, auto_increment=10", true, nil},
Copy link
Contributor

Choose a reason for hiding this comment

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

This SQL is different with SQL in 78. Should we add tests in ddl_db_test.go?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! PTAL @zimulala

{"alter table t add column c int auto_increment key", true, nil},
{"alter table t add column char4294967295 char(255)", true, nil},
{"create table t (c float(53))", true, nil},
Expand Down Expand Up @@ -184,6 +183,6 @@ func (s *testValidatorSuite) TestValidator(c *C) {
c.Assert(stmts, HasLen, 1)
stmt := stmts[0]
err = plan.Preprocess(ctx, stmt, is, tt.inPrepare)
c.Assert(terror.ErrorEqual(err, tt.err), IsTrue)
c.Assert(terror.ErrorEqual(err, tt.err), IsTrue, Commentf("sql: %s", tt.sql))
}
}