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

crazycs520 · 2019-05-15T04:03:23Z

What problem does this PR solve?

Below sql will cause tidb panic when add index.

create table t1 (a int,b int,  primary key(a)) partition by hash(a) partitions 99;
insert into t1 set a=1;
insert into t1 set a=0;
alter table t1 add index idx(a);  # panic

What is changed and how it works?

cancel add index ddl job when can not find partition
Fix GetPartition function bug.
Make alter table t1 add index idx(a); execute successful.

Check List

Tests

Unit test

Code changes

Has exported function/method change

Side effects

Related changes

Need to cherry-pick to the release branch

…n function bug

crazycs520 · 2019-05-15T04:06:40Z

table/tables/partition.go

@@ -313,7 +313,11 @@ 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]
+	p, ok := t.partitions[pid]


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 }

Golang traps and pitfalls...

A nil of type *partition is a kind of physibleTable

Good catch! @crazycs520

Would like to add some comment here?

codecov · 2019-05-15T04:07:53Z

Codecov Report

Merging #10475 into master will decrease coverage by 0.0119%.
The diff coverage is 42.8571%.

@@               Coverage Diff               @@
##             master     #10475       +/-   ##
===============================================
- Coverage   77.2752%   77.2633%   -0.012%     
===============================================
  Files           413        413               
  Lines         86967      86974        +7     
===============================================
- Hits          67204      67199        -5     
- Misses        14599      14608        +9     
- Partials       5164       5167        +3

crazycs520 · 2019-05-15T05:55:31Z

@winkyao @zimulala @tiancaiamao PTAL

bb7133 · 2019-05-15T06:19:48Z

meta/meta.go

 	// update them to table's in this case.
-	if endHandle == 0 || physicalTableID == 0 {


why ignore endHandle here?

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

Is there any test to cover here?

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

…/crazycs520/tidb into fix-add-index-in-partition-table

…-index-in-partition-table

tiancaiamao · 2019-05-16T03:09:14Z

table/tables/partition.go

@@ -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`


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.

tiancaiamao · 2019-05-16T03:11:48Z

LGTM

tiancaiamao · 2019-05-16T03:12:39Z

/run-all-tests

zimulala

LGTM

winkyao · 2019-05-16T09:15:31Z

ddl/index.go

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


use util.WithRecover instead.

No, I need return cancelled error here.

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

Ok, go WithReocover can not return values

crazycs520 · 2019-05-20T02:31:22Z

/run-all-tests

winkyao

LGTM

…n function bug (pingcap#10475)

…n function bug (#10475) (#10533)

ddl: cancel add index when can not find partition and fix GetPartitio…

0f801fa

…n function bug

crazycs520 added type/bug-fix This PR fixes a bug. component/DDL-need-LGT3 labels May 15, 2019

crazycs520 commented May 15, 2019

View reviewed changes

crazycs520 added 3 commits May 15, 2019 13:40

fix add index on partition and add test.

e9d0a53

fix test

edf9d76

add test

18872d1

crazycs520 requested review from tiancaiamao and zimulala and removed request for tiancaiamao May 15, 2019 05:55

crazycs520 removed the status/WIP label May 15, 2019

Merge branch 'master' into fix-add-index-in-partition-table

d491799

bb7133 reviewed May 15, 2019

View reviewed changes

crazycs520 added 4 commits May 15, 2019 16:57

add recover

52ada5e

Merge branch 'fix-add-index-in-partition-table' of https://github.com…

e45863f

…/crazycs520/tidb into fix-add-index-in-partition-table

add comment

67d9966

Merge branch 'master' of https://github.com/pingcap/tidb into fix-add…

6c14dde

…-index-in-partition-table

tiancaiamao reviewed May 16, 2019

View reviewed changes

tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label May 16, 2019

zimulala reviewed May 16, 2019

View reviewed changes

zimulala added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 16, 2019

winkyao reviewed May 16, 2019

View reviewed changes

ngaut and others added 2 commits May 17, 2019 11:10

Merge branch 'master' into fix-add-index-in-partition-table

3563ba5

Merge branch 'master' into fix-add-index-in-partition-table

fcbd8ed

Merge branch 'master' into fix-add-index-in-partition-table

605a10d

winkyao reviewed May 20, 2019

View reviewed changes

winkyao approved these changes May 20, 2019

View reviewed changes

crazycs520 merged commit f75d9c0 into pingcap:master May 20, 2019

crazycs520 mentioned this pull request May 20, 2019

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

Merged

crazycs520 added a commit to crazycs520/tidb that referenced this pull request May 20, 2019

ddl: cancel add index when can not find partition and fix GetPartitio…

d3232a3

…n function bug (pingcap#10475)

winkyao pushed a commit that referenced this pull request May 20, 2019

ddl: cancel add index when can not find partition and fix GetPartitio…

d651e27

…n function bug (#10475) (#10533)

you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

crazycs520 commented May 15, 2019 •

edited

Loading

crazycs520 May 15, 2019

tiancaiamao May 15, 2019

tiancaiamao May 15, 2019

tiancaiamao May 15, 2019

crazycs520 May 16, 2019

codecov bot commented May 15, 2019 •

edited

Loading

crazycs520 commented May 15, 2019

bb7133 May 15, 2019

crazycs520 May 15, 2019

tiancaiamao May 15, 2019

crazycs520 May 16, 2019

tiancaiamao May 16, 2019

tiancaiamao commented May 16, 2019

tiancaiamao commented May 16, 2019

zimulala left a comment

winkyao May 16, 2019

crazycs520 May 17, 2019

winkyao May 20, 2019

winkyao May 20, 2019

crazycs520 commented May 20, 2019

winkyao left a comment

		// update them to table's in this case.
		if endHandle == 0 \|\| physicalTableID == 0 {

-	// 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.

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

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

Conversation

crazycs520 commented May 15, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 15, 2019 • edited Loading

Codecov Report

crazycs520 commented May 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiancaiamao commented May 16, 2019

tiancaiamao commented May 16, 2019

zimulala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crazycs520 commented May 20, 2019

winkyao left a comment

Choose a reason for hiding this comment

crazycs520 commented May 15, 2019 •

edited

Loading

codecov bot commented May 15, 2019 •

edited

Loading