-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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 drop partition #6460
Conversation
parser/parser.y
Outdated
@@ -936,6 +936,13 @@ AlterTableSpec: | |||
{ | |||
$$ = &ast.AlterTableSpec{Tp: ast.AlterTableDropPrimaryKey} | |||
} | |||
| "DROP" "PARTITION" Identifier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add test for this.
ddl/ddl_api.go
Outdated
} | ||
t, err := is.TableByName(ident.Schema, ident.Name) | ||
if err != nil { | ||
return errors.Trace(infoschema.ErrTableExists.GenByArgs(ident.Schema, ident.Name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why ErrTableExists
?
ddl/table.go
Outdated
@@ -394,3 +394,67 @@ func updateVersionAndTableInfo(t *meta.Meta, job *model.Job, tblInfo *model.Tabl | |||
} | |||
return ver, t.UpdateTable(job.SchemaID, tblInfo) | |||
} | |||
|
|||
func checkPartitionRangeList(meta *model.TableInfo, partName string) error { | |||
if meta.Partition != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if meta.Partition == nil {
return errors.Trace(infoschema.ErrPartitionMgmtOnNonpartitioned)
}
...
ddl/table.go
Outdated
|
||
func checkPartitionRangeList(meta *model.TableInfo, partName string) error { | ||
if meta.Partition != nil { | ||
noExist := false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you mean exist := false
?
ddl/table.go
Outdated
parInfo.Definitions = make([]model.PartitionDefinition, 0, 1) | ||
} | ||
for _, oldDef := range oldDefs { | ||
if oldDef.Name == partName { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is partition name case sensitive of not?
ddl/table.go
Outdated
return nil | ||
} | ||
|
||
func dropTablePartitionInfo(meta *model.TableInfo, partName string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What will happen if we drop the last partition? Will the table degenerate to a normal table?
What happen when we add a partition back? Will the partition id be the same?
ddl/table.go
Outdated
if err != nil { | ||
return errors.Trace(err) | ||
} | ||
parInfo := &model.PartitionInfo{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you don't need to define a parInfo
, just Definitions
is enough?
622da70
to
2935cda
Compare
@zimulala @tiancaiamao PTAL |
ddl/table.go
Outdated
@@ -26,6 +26,7 @@ import ( | |||
"github.com/pingcap/tidb/table" | |||
"github.com/pingcap/tidb/tablecodec" | |||
log "github.com/sirupsen/logrus" | |||
"strings" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to line 18
ddl/table.go
Outdated
} else { | ||
newDefs = append(newDefs, oldDef) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ddl/table.go
Outdated
partInfo := tblInfo.Partition | ||
originalState := partInfo.State | ||
switch partInfo.State { | ||
case model.StatePublic: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you have to re-design here.
The problem drop index faces, is that not all TiDB nodes see the same state.
If tidb1 think a table has an index, while tidb2 think a table hasn't an index, they will run into trouble.
That why we need several steps and states for one alter table operation.
Table partition is different.
partition by range (id) (
p1 less than (10),
p2 less than (30),
p3 less than (maxvaluex)
)
alter table drop partition p2
Insert into t value (20)
It should locate to p3, how to keep the change atomic ?
@zimulala @tiancaiamao PTAL |
meta/meta.go
Outdated
return errors.Trace(err) | ||
} | ||
if delAutoID { | ||
if err := m.txn.HDel(dbKey, m.autoTableIDKey(pid)); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
autoTableIDKey belongs to table or partition? @zimulala
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
b374ed9
to
d6c2ced
Compare
ddl/ddl.go
Outdated
@@ -168,6 +168,8 @@ var ( | |||
ErrRangeNotIncreasing = terror.ClassSchema.New(codeRangeNotIncreasing, "VALUES LESS THAN value must be strictly increasing for each partition") | |||
// ErrPartitionMaxvalue returns maxvalue can only be used in last partition definition. | |||
ErrPartitionMaxvalue = terror.ClassSchema.New(codePartitionMaxvalue, "MAXVALUE can only be used in last partition definition") | |||
//ErrDropLastPartition returns cannot remove all partitions, use drop table instead. | |||
ErrDropLastPartition = terror.ClassSchema.New(codeDropLastPartition, "Duplicate partition name %s") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why ErrDropLastPartition
message is "Duplicate partition name" ??
ddl/ddl_api.go
Outdated
return errors.Trace(infoschema.ErrTableNotExists.GenByArgs(ident.Schema, ident.Name)) | ||
} | ||
meta := t.Meta() | ||
if meta.GetPartitionInfo() == nil && meta.Partition == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meta.GetPartitionInfo() == nil
is enough
ddl/ddl_db_test.go
Outdated
@@ -2366,7 +2366,7 @@ func (s *testDBSuite) TestAlterTableAddPartition(c *C) { | |||
|
|||
ctx := s.tk.Se.(sessionctx.Context) | |||
is := domain.GetDomain(ctx).InfoSchema() | |||
tbl, err := is.TableByName(model.NewCIStr("test"), model.NewCIStr("tp")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old code success? why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the table tp
exists and is a partitioned table.
ddl/ddl_db_test.go
Outdated
|
||
func (s *testDBSuite) TestAlterTableDropPartition(c *C) { | ||
s.tk.MustExec("use test") | ||
s.tk.MustExec("drop table if employees") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s.tk.MustExec("set @@tidb_enable_table_partition = 1")
I guess you test doesn't cover your code, because Partition.Enable
is false by default.
ddl/ddl_db_test.go
Outdated
tbl, err := is.TableByName(model.NewCIStr("test"), model.NewCIStr("employees")) | ||
c.Assert(err, IsNil) | ||
c.Assert(tbl.Meta().Partition, NotNil) | ||
part := tbl.Meta().Partition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbl.Meta().GetPartitionInfo()
Follow this way, and when we set Partition.Enable
to true by default, the code should works.
ddl/ddl_db_test.go
Outdated
is = domain.GetDomain(ctx).InfoSchema() | ||
tbl, err = is.TableByName(model.NewCIStr("test"), model.NewCIStr("t4")) | ||
c.Assert(err, IsNil) | ||
c.Assert(tbl.Meta().Partition, NotNil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
ddl/delete_range.go
Outdated
@@ -241,6 +241,11 @@ func insertJobIntoDeleteRangeTable(ctx sessionctx.Context, job *model.Job) error | |||
startKey := tablecodec.EncodeTablePrefix(tableID) | |||
endKey := tablecodec.EncodeTablePrefix(tableID + 1) | |||
return doInsert(s, job.ID, tableID, startKey, endKey, now) | |||
case model.ActionDropTablePartition: | |||
tableID := job.TableID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this handle both tikv and mocktikv?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tiancaiamao I have tested that it is normal to delete partition data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use paritionID?
ddl/table.go
Outdated
@@ -484,3 +484,76 @@ func checkAddPartitionValue(meta *model.TableInfo, part *model.PartitionInfo) er | |||
} | |||
return nil | |||
} | |||
|
|||
func checkPartitionRangeList(meta *model.TableInfo, partName string) error { | |||
if meta.Partition == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetPartitionInfo
@tiancaiamao @zimulala @coocood PTAL. |
1 similar comment
@tiancaiamao @zimulala @coocood PTAL. |
ddl/ddl_api.go
Outdated
@@ -1007,6 +1007,8 @@ func (d *ddl) AlterTable(ctx sessionctx.Context, ident ast.Ident, specs []*ast.A | |||
err = d.AddColumn(ctx, ident, spec) | |||
case ast.AlterTableAddPartitions: | |||
err = d.AddTablePartitions(ctx, ident, spec) | |||
case ast.AlterTableDropPartition: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep the cases in alphabet order.
ddl/ddl_worker.go
Outdated
@@ -435,6 +435,8 @@ func (w *worker) runDDLJob(d *ddlCtx, t *meta.Meta, job *model.Job) (ver int64, | |||
ver, err = onModifyTableComment(t, job) | |||
case model.ActionAddTablePartition: | |||
ver, err = onAddTablePartition(t, job) | |||
case model.ActionDropTablePartition: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
ddl/table.go
Outdated
oldDefs := meta.Partition.Definitions | ||
for _, def := range oldDefs { | ||
if strings.EqualFold(def.Name, partName) { | ||
if len(oldDefs) == 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to check this in DDL worker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shenli Parallel drop partition needs to be checked.
meta/meta.go
Outdated
return errors.Trace(err) | ||
} | ||
|
||
tableKey := m.tableKey(pid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a separated partition info outside table info?
@tiancaiamao @zimulala @coocood PTAL. |
ddl/ddl_db_test.go
Outdated
s.testErrorCode(c, sql2, mysql.ErrDropPartitionNonExistent) | ||
|
||
s.tk.MustExec("drop table if t3") | ||
s.tk.MustExec(`"CREATE TABLE t3 ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`" Is this a normal SQL string? "`
ddl/table.go
Outdated
newDefs = make([]model.PartitionDefinition, 0, len(oldDefs)-1) | ||
var pid int64 | ||
for i := 0; i < len(oldDefs); i++ { | ||
if strings.EqualFold(oldDefs[i].Name, partName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if !strings.EqualFold(oldDefs[i].Name, partName) {
continue;
}
BTW: I think we should use CIStr for ldDefs[i].Name.
Args: []interface{}{spec.Name}, | ||
} | ||
|
||
err = d.doDDLJob(ctx, job) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check this error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
ddl/delete_range.go
Outdated
@@ -241,6 +241,11 @@ func insertJobIntoDeleteRangeTable(ctx sessionctx.Context, job *model.Job) error | |||
startKey := tablecodec.EncodeTablePrefix(tableID) | |||
endKey := tablecodec.EncodeTablePrefix(tableID + 1) | |||
return doInsert(s, job.ID, tableID, startKey, endKey, now) | |||
case model.ActionDropTablePartition: | |||
tableID := job.TableID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use paritionID?
ddl/table.go
Outdated
// Finish this job. | ||
job.FinishTableJob(model.JobStateDone, model.StateNone, ver, tblInfo) | ||
|
||
return ver, errors.Trace(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The err must be nil here.
ddl/table.go
Outdated
@@ -484,3 +484,73 @@ func checkAddPartitionValue(meta *model.TableInfo, part *model.PartitionInfo) er | |||
} | |||
return nil | |||
} | |||
|
|||
func checkPartitionRangeList(meta *model.TableInfo, partName string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add comment for this function.
ddl/delete_range.go
Outdated
@@ -241,6 +241,12 @@ func insertJobIntoDeleteRangeTable(ctx sessionctx.Context, job *model.Job) error | |||
startKey := tablecodec.EncodeTablePrefix(tableID) | |||
endKey := tablecodec.EncodeTablePrefix(tableID + 1) | |||
return doInsert(s, job.ID, tableID, startKey, endKey, now) | |||
case model.ActionDropTablePartition: | |||
// the `job.TableID` here stores partitionID. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/the/The
c11e3a4
to
e493083
Compare
LGTM |
ddl/partition.go
Outdated
tblInfo.Partition.Definitions = newDefs | ||
} | ||
|
||
// onDropTablePartition delete old partition meta. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/delete/deletes
ddl/db_test.go
Outdated
part := tbl.Meta().Partition | ||
c.Assert(part.Type, Equals, model.PartitionTypeRange) | ||
c.Assert(part.Expr, Equals, "`hired`") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this blank line.
ddl/db_test.go
Outdated
part = tbl.Meta().Partition | ||
c.Assert(part.Type, Equals, model.PartitionTypeRange) | ||
c.Assert(part.Expr, Equals, "`id`") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
s.tk.MustExec("drop table if exists tr;") | ||
s.tk.MustExec(` create table tr( | ||
id int, name varchar(50), | ||
purchased date |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a key for purchased
? And check the index rows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ddl/partition.go
Outdated
} | ||
tblInfo, err := getTableInfo(t, job, job.SchemaID) | ||
if err != nil { | ||
job.State = model.JobStateCancelled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has been set in getTableInfo
.
73f8684
to
3e856ab
Compare
ddl/partition.go
Outdated
// Finish this job. | ||
job.FinishTableJob(model.JobStateDone, model.StateNone, ver, tblInfo) | ||
// A background job will be created to delete old partition data. | ||
job.TableID = pid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd better not change TableID
meaning. It is used by binlog.
ddl/partition.go
Outdated
func checkDropTablePartition(meta *model.TableInfo, partName string) error { | ||
oldDefs := meta.Partition.Definitions | ||
for _, def := range oldDefs { | ||
if strings.EqualFold(def.Name.O, partName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to use def.Name.L
and partName.L
check?
ddl/partition.go
Outdated
newDefs := make([]model.PartitionDefinition, 0, len(oldDefs)-1) | ||
var pid int64 | ||
for i := 0; i < len(oldDefs); i++ { | ||
if !strings.EqualFold(oldDefs[i].Name.O, partName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we check it with oldDefs[i].Name.L
?
Could we add a test for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/run-all-tests |
alter table drop partition
The first step to implement alter table drop partition.