ddl:support drop index for the partitioned table. #7306

ciscoxll · 2018-08-07T11:45:31Z

What have you changed? (mandatory)

support drop index for the partitioned table

We first need to get all the partition IDs and then we need to delete the index in each partition.
Fix issue support drop index in range partition #7222

Take this as an example:

mysql> set @@tidb_enable_table_partition = 1;
Query OK, 0 rows affected (0.00 sec)

mysql> create table tr(
    ->         id int, name varchar(50),
    ->         purchased date
    ->     )
    ->     partition by range( year(purchased) ) (
    ->         partition p0 values less than (1990),
    ->         partition p1 values less than (1995),
    ->         partition p2 values less than (2000),
    ->         partition p3 values less than (2005),
    ->         partition p4 values less than (2010),
    ->         partition p5 values less than (2015)
    ->        );
Query OK, 0 rows affected (0.01 sec)

mysql>  INSERT INTO tr VALUES
    ->     (1, 'desk organiser', '2003-10-15'),
    ->     (2, 'alarm clock', '1997-11-05'),
    ->     (3, 'chair', '2009-03-10'),
    ->     (4, 'bookcase', '1989-01-10'),
    ->     (5, 'exercise bike', '2014-05-09'),
    ->     (6, 'sofa', '1987-06-05'),
    ->     (7, 'espresso maker', '2011-11-22'),
    ->     (8, 'aquarium', '1992-08-04'),
    ->     (9, 'study desk', '2006-09-16'),
    ->     (10, 'lava lamp', '1998-12-25');
Query OK, 10 rows affected (0.10 sec)

mysql> create index idx1 on tr (purchased);
Query OK, 0 rows affected (0.04 sec)

mysql> select count(purchased) from tr use index(idx1);
+------------------+
| count(purchased) |
+------------------+
|               10 |
+------------------+
1 row in set (0.00 sec)

mysql> drop index idx1 on tr;
Query OK, 0 rows affected (0.03 sec)

mysql> select count(purchased) from tr use index(idx1);
ERROR 1176 (42000): Key 'idx1' doesn't exist in table 'tr'

What is the type of the changes? (mandatory)

New feature

New feature (non-breaking change which adds functionality)

How has this PR been tested? (mandatory)

Unit tests.

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

No

Does this PR affect tidb-ansible update? (mandatory)

No

Does this PR need to be added to the release notes? (mandatory)

No

tiancaiamao · 2018-08-08T06:18:14Z

ddl/db_test.go

+    (9, 'study desk', '2006-09-16'),
+    (10, 'lava lamp', '1998-12-25');`)
+
+	tk.MustExec("create index idx1 on tr (purchased)")


It test drop index, so how about create the table with index together?

tiancaiamao · 2018-08-08T06:20:01Z

ddl/db_test.go

+	tk.MustQuery("select count(purchased) from tr use index(idx1)").Check(testkit.Rows("10"))
+	tk.MustExec("admin check table tr")
+	// Wait for add index done.
+	time.Sleep(waitForCleanDataInterval)


There is no need to wait.
create index idx1 on tr (purchased) blocks for a while, as long as it returns, add index will be done.

tiancaiamao · 2018-08-08T06:32:43Z

ddl/index.go

@@ -436,7 +436,7 @@ func onDropIndex(t *meta.Meta, job *model.Job) (ver int64, _ error) {
 			job.Args[0] = indexInfo.ID
 		} else {
 			job.FinishTableJob(model.JobStateDone, model.StateNone, ver, tblInfo)
-			job.Args = append(job.Args, indexInfo.ID)
+			job.Args = append(job.Args, indexInfo.ID, getPartitionIDs(tblInfo))


If the table is not a partition, what will getPartitionIDs return?

getPartitionIDs will return []int64{}.

tiancaiamao · 2018-08-10T06:14:08Z

table/tables/tables.go

@@ -81,10 +81,10 @@ func MockTableFromMeta(tblInfo *model.TableInfo) table.Table {

 	var t Table
 	initTableCommon(&t.tableCommon, tblInfo, tblInfo.ID, columns, nil)
+	if err := initTableIndices(&t.tableCommon); err != nil {


Is this change relevant to the PR?

tiancaiamao · 2018-08-10T06:24:05Z

ddl/db_test.go

+			c.Assert(err, IsNil, Commentf("err:%v", errors.ErrorStack(err)))
+		case <-ticker.C:
+			step := 10
+			for i := num; i < num+step; i++ {


The random data will always locate in the first partition...

ciscoxll · 2018-08-12T04:34:42Z

@tiancaiamao @winkyao PTAL.

tiancaiamao · 2018-08-13T02:42:33Z

LGTM

ciscoxll · 2018-08-13T03:49:14Z

@winkyao @zimulala PTAL.

shenli · 2018-08-13T09:22:41Z

ddl/delete_range.go

+				}
+			}
+			return nil
+		}


if len(partitionIDs) > 0 {
} else {
}
This could make code more clear.

shenli · 2018-08-13T09:23:39Z

ddl/delete_range.go

@@ -279,9 +279,20 @@ func insertJobIntoDeleteRangeTable(ctx sessionctx.Context, job *model.Job) error
 		tableID := job.TableID
 		var indexName interface{}
 		var indexID int64
-		if err := job.DecodeArgs(&indexName, &indexID); err != nil {
+		var partitionIDs []int64
+		if err := job.DecodeArgs(&indexName, &indexID, &partitionIDs); err != nil {


We have the job.TableID. Could we get the partition IDs from the info schema instead of job arguments?

When modifying table schema, there is a new table info and an old one.
It's easy to get the stale IDs if we get it from the info schema. @shenli

@shenli Existing function getPartitionIDs.

func getPartitionIDs(table *model.TableInfo) []int64 { if table.GetPartitionInfo() == nil { return []int64{} } partitionIDs := make([]int64, 0, len(table.Partition.Definitions)) for _, def := range table.Partition.Definitions { partitionIDs = append(partitionIDs, def.ID) } return partitionIDs }

Is it clearer.

In drop index, because the ddl of same table is serial executed, so get partition IDs from info schema is ok.
But I remain neutral, get the partition IDs from the info schema or use job arguments are both ok for me. >v<

"It's easy to get the stale IDs if we get it from the info schema." Can not understand this. We should guarantee that we will get the correct schema when doing DDL.

You can ignore the comment @shenli , I just give a hint on how to achieve this...

We should guarantee that we will get the correct schema when doing DDL.

@ciscoxll Please address the comment. We do not need to put partition IDs in the job argument.

Do agree with shenli. please address the comment.

@shenli @winkyao
I think it's OK. The processing here is unified with other places.

crazycs520 · 2018-08-13T11:21:18Z

ddl/db_test.go

+			for i := num; i < num+step; i++ {
+				rnd := rand.Intn(num)
+				s.mustExec(c, "update partition_drop_idx set c2 = 1 where c1 = ?", rnd)
+				s.mustExec(c, "insert into partition_drop_idx values (?, ?, ?)", i, i, i)


i may more than 20, then, insert into partition_drop_idx values (20,20,20)" should return error: (1604, 'locate partition failed')

ciscoxll · 2018-08-14T10:01:23Z

@winkyao @zimulala PTAL.

winkyao · 2018-08-15T02:24:55Z

You can use interface to get InfoSchema to solve the cycle import problem.

ciscoxll · 2018-08-15T09:52:41Z

@shenli @winkyao @crazycs520 PTAL.

crazycs520

LGTM

zimulala

LGTM

zimulala · 2018-08-17T05:41:50Z

/run-all-tests

zhexuany

LGTM

ciscoxll added component/DDL-need-LGT3 labels Aug 7, 2018

tiancaiamao reviewed Aug 8, 2018

View reviewed changes

ciscoxll force-pushed the drop-partition-index branch from f8f0298 to 95355b8 Compare August 10, 2018 02:14

ciscoxll removed the status/WIP label Aug 10, 2018

tiancaiamao reviewed Aug 10, 2018

View reviewed changes

address comment

56b719e

ciscoxll force-pushed the drop-partition-index branch from 95355b8 to 56b719e Compare August 10, 2018 09:37

tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 13, 2018

shenli reviewed Aug 13, 2018

View reviewed changes

crazycs520 reviewed Aug 13, 2018

View reviewed changes

ciscoxll added 2 commits August 14, 2018 15:54

address comment

40ab8c2

fix ci error

5285650

address comment

75d4e6c

fix ci error

22068b8

crazycs520 reviewed Aug 16, 2018

View reviewed changes

ciscoxll added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 16, 2018

zimulala approved these changes Aug 17, 2018

View reviewed changes

zimulala added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Aug 17, 2018

Merge branch 'master' into drop-partition-index

fbc0db6

zhexuany approved these changes Aug 17, 2018

View reviewed changes

zhexuany merged commit 8199f60 into pingcap:master Aug 17, 2018

zhexuany added the status/all tests passed label Aug 17, 2018

ciscoxll deleted the drop-partition-index branch August 17, 2018 06:41

ciscoxll mentioned this pull request Aug 20, 2018

ddl:support add index rollback of partitioned table #7437

Merged

tiancaiamao mentioned this pull request Nov 21, 2018

Drop index operation on hash partitioned table #8378

Closed

sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019

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:support drop index for the partitioned table. #7306

ddl:support drop index for the partitioned table. #7306

ciscoxll commented Aug 7, 2018 •

edited

Loading

tiancaiamao Aug 8, 2018

tiancaiamao Aug 8, 2018

tiancaiamao Aug 8, 2018

ciscoxll Aug 8, 2018

tiancaiamao Aug 10, 2018

tiancaiamao Aug 10, 2018

ciscoxll commented Aug 12, 2018

tiancaiamao commented Aug 13, 2018

ciscoxll commented Aug 13, 2018

shenli Aug 13, 2018

shenli Aug 13, 2018

tiancaiamao Aug 13, 2018

ciscoxll Aug 13, 2018

crazycs520 Aug 13, 2018

shenli Aug 13, 2018

tiancaiamao Aug 14, 2018

shenli Aug 14, 2018

winkyao Aug 14, 2018

zimulala Aug 16, 2018

crazycs520 Aug 13, 2018

ciscoxll commented Aug 14, 2018

winkyao commented Aug 15, 2018

ciscoxll commented Aug 15, 2018

crazycs520 left a comment

zimulala left a comment

zimulala commented Aug 17, 2018

zhexuany left a comment

ddl:support drop index for the partitioned table. #7306

ddl:support drop index for the partitioned table. #7306

Conversation

ciscoxll commented Aug 7, 2018 • edited Loading

What have you changed? (mandatory)

What is the type of the changes? (mandatory)

How has this PR been tested? (mandatory)

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

Does this PR affect tidb-ansible update? (mandatory)

Does this PR need to be added to the release notes? (mandatory)

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

Choose a reason for hiding this comment

ciscoxll commented Aug 12, 2018

tiancaiamao commented Aug 13, 2018

ciscoxll commented Aug 13, 2018

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

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

Choose a reason for hiding this comment

ciscoxll commented Aug 14, 2018

winkyao commented Aug 15, 2018

ciscoxll commented Aug 15, 2018

crazycs520 left a comment

Choose a reason for hiding this comment

zimulala left a comment

Choose a reason for hiding this comment

zimulala commented Aug 17, 2018

zhexuany left a comment

Choose a reason for hiding this comment

ciscoxll commented Aug 7, 2018 •

edited

Loading