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.

Sign up for GitHub

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: cancel add index when can not find partition and fix GetPartition function bug #10475

Merged
merged 12 commits into from
May 20, 2019
Merged

ddl: cancel add index when can not find partition and fix GetPartition function bug #10475

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
0f801fa
ddl: cancel add index when can not find partition and fix GetPartitio…
crazycs520 May 15, 2019
e9d0a53
fix add index on partition and add test.
crazycs520 May 15, 2019
edf9d76
fix test
crazycs520 May 15, 2019
18872d1
add test
crazycs520 May 15, 2019
d491799
Merge branch 'master' into fix-add-index-in-partition-table
crazycs520 May 15, 2019
52ada5e
add recover
crazycs520 May 15, 2019
e45863f
Merge branch 'fix-add-index-in-partition-table' of https://github.com…
crazycs520 May 15, 2019
67d9966
add comment
crazycs520 May 16, 2019
6c14dde
Merge branch 'master' of https://github.com/pingcap/tidb into fix-add…
crazycs520 May 16, 2019
3563ba5
Merge branch 'master' into fix-add-index-in-partition-table
ngaut May 17, 2019
fcbd8ed
Merge branch 'master' into fix-add-index-in-partition-table
crazycs520 May 17, 2019
605a10d
Merge branch 'master' into fix-add-index-in-partition-table
crazycs520 May 20, 2019
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
17 changes: 17 additions & 0 deletions ddl/db_partition_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1279,6 +1279,23 @@ func (s *testIntegrationSuite1) TestPartitionAddIndex(c *C) {
hired date not null
) partition by hash( year(hired) ) partitions 4;`)
testPartitionAddIndex(tk, c)

// Test hash partition for pr 10475.
tk.MustExec("drop table if exists t1")
defer tk.MustExec("drop table if exists t1")
tk.MustExec("set @@session.tidb_enable_table_partition = '1';")
tk.MustExec("create table t1 (a int,b int, primary key(a)) partition by hash(a) partitions 5;")
tk.MustExec("insert into t1 values (0,0),(1,1),(2,2),(3,3);")
tk.MustExec("alter table t1 add index idx(a)")
tk.MustExec("admin check table t1;")

// Test range partition for pr 10475.
tk.MustExec("drop table t1")
tk.MustExec("create table t1 (a int,b int, primary key(a)) partition by range (a) (partition p0 values less than (10), partition p1 values less than (20));")
tk.MustExec("insert into t1 values (0,0);")
tk.MustExec("alter table t1 add index idx(a)")
tk.MustExec("admin check table t1;")

}

func testPartitionAddIndex(tk *testkit.TestKit, c *C) {
Expand Down
13 changes: 11 additions & 2 deletions ddl/index.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,16 @@ func (w *worker) onCreateIndex(d *ddlCtx, t *meta.Meta, job *model.Job) (ver int
return ver, errors.Trace(err)
}

err = w.runReorgJob(t, reorgInfo, d.lease, func() error {
err = w.runReorgJob(t, reorgInfo, d.lease, func() (addIndexErr error) {
defer func() {
r := recover()
Copy link
Contributor

Choose a reason for hiding this comment

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

use util.WithRecover instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I need return cancelled error here.

Copy link
Contributor

Choose a reason for hiding this comment

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

you can change util.WithReocover to return value from exec func

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, go WithReocover can not return values

if r != nil {
buf := util.GetStack()
logutil.Logger(ddlLogCtx).Error("[ddl] add table index panic", zap.Any("panic", r), zap.String("stack", string(buf)))
metrics.PanicCounter.WithLabelValues(metrics.LabelDDL).Inc()
addIndexErr = errCancelledDDLJob.GenWithStack("add table `%v` index `%v` panic", tblInfo.Name, indexInfo.Name)
}
}()
return w.addTableIndex(tbl, indexInfo, reorgInfo)
})
if err != nil {
Expand Down Expand Up @@ -1200,7 +1209,7 @@ func (w *worker) addTableIndex(t table.Table, idx *model.IndexInfo, reorgInfo *r
for !finish {
p := tbl.GetPartition(reorgInfo.PhysicalTableID)
if p == nil {
return errors.Errorf("Can not find partition id %d for table %d", reorgInfo.PhysicalTableID, t.Meta().ID)
return errCancelledDDLJob.GenWithStack("Can not find partition id %d for table %d", reorgInfo.PhysicalTableID, t.Meta().ID)
}
err = w.addPhysicalTableIndex(p, idx, reorgInfo)
if err != nil {
Expand Down
4 changes: 2 additions & 2 deletions meta/meta.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -733,9 +733,9 @@ func (m *Meta) GetDDLReorgHandle(job *model.Job) (startHandle, endHandle, physic
err = errors.Trace(err)
return
}
// endHandle or physicalTableID may be 0, because older version TiDB (without table partition) doesn't store them.
// physicalTableID may be 0, because older version TiDB (without table partition) doesn't store them.
// update them to table's in this case.
if endHandle == 0 || physicalTableID == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

why ignore endHandle here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because endHandle is 0 don't mean doesn't store them before. Maybe the stored endHandle is 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any test to cover here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, https://github.com/pingcap/tidb/pull/10475/files#diff-e259172e2953a3597b096f11d3c8d3daR1289 this test do this job.

if physicalTableID == 0 {
if job.ReorgMeta != nil {
endHandle = job.ReorgMeta.EndHandle
} else {
Expand Down
8 changes: 7 additions & 1 deletion table/tables/partition.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,13 @@ func (t *partitionedTable) locateHashPartition(ctx sessionctx.Context, pi *model

// GetPartition returns a Table, which is actually a partition.
func (t *partitionedTable) GetPartition(pid int64) table.PhysicalTable {
return t.partitions[pid]
// Attention, can't simply use `return t.partitions[pid]` here.
// Because A nil of type *partition is a kind of `table.PhysicalTable`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Because A nil of type *partition is a kind of `table.PhysicalTable`
// Because A nil of type *partition is a kind of `table.PhysicalTable`,
// the final result will be a non-nil interface whose underlying value is nil,
// use that result will get a nil pointer panic.

p, ok := t.partitions[pid]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

eg:

type partition struct {
	tableCommon
}

type partitionTables struct {
	ps map[int]*partition
}

func (p *partition) GetPhysicalID() int {
	return 0
}

type tableCommon struct {
	tableID int64
}

type physibleTable interface {
	GetPhysicalID() int
}

func (pt *partitionTables) getPartition(id int) physibleTable {
	return pt.ps[id]
}

func (pt *partitionTables) getPartition2(id int) physibleTable {
	p, ok := pt.ps[id]
	if !ok {
		return nil
	}
	return p
}

func main() {
	ps := make(map[int]*partition)
	pt := &partitionTables{ps: ps}
	fmt.Println(pt.getPartition(0) == nil)  // false
	fmt.Println(pt.getPartition2(0) == nil) // true
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Golang traps and pitfalls...

A nil of type *partition is a kind of physibleTable

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch! @crazycs520

Copy link
Contributor

Choose a reason for hiding this comment

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

Would like to add some comment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

if !ok {
return nil
}
return p
}

// GetPartitionByRow returns a Table, which is actually a Partition.
Expand Down