ddl: support drop sequence in TiDB #14442

AilinKid · 2020-01-10T08:59:48Z

support drop sequence in TiDB.

DROP [TEMPORARY] SEQUENCE [IF EXISTS] sequence_name [, sequence_name] ...

What problem does this PR solve?

What is changed and how it works?

drop sequence share many logic code with dropTableOrView, so I merge them together.
drop sequence will also delete the sequence value kv pair (drop table and drop table will also do this. But don't worry, HDel() won't take effect if the sequence kv-pair is not exist in case of drop table/view)

Check List

Tests

Unit test
Integration test

Code changes

Has exported function/method change
Has interface methods change

Related changes

Need to update the documentation

Release note

ddl: Support drop sequence in TiDB.

ddl/ddl_api.go

bb7133 · 2020-01-10T10:53:00Z

ddl/sequence_test.go

@@ -84,3 +84,37 @@ func (s *testSequenceSuite) TestCreateSequence(c *C) {
 	c.Assert(err, NotNil)
 	c.Assert(err.Error(), Equals, "[planner:1142]CREATE command denied to user 'localhost'@'myuser' for table 'my_seq'")
 }
+
+func (s *testSequenceSuite) TestDropSequence(c *C) {
+	s.tk = testkit.NewTestKit(c, s.store)


Please add the test case for drop table SEQUCNE_NAME, 'drop view SEQUCNE_NAME`.

Also, we need to test the behavior for drop sequence TABLE_NAME, drop sequence VIEW_NAME, but not in this function. You can find a suitable place.

good catch.

wwar · 2020-01-12T06:03:39Z

It does not look like privilege checks are being done here. I assume it will be handled in a followup PR?

AilinKid · 2020-01-15T07:48:22Z

It does not look like privilege checks are being done here. I assume it will be handled in a followup PR?

Good catch! thanks for reminding me.

bb7133 · 2020-01-16T02:18:17Z

executor/ddl.go

+
+}
+
+func (e *DDLExec) dropTableViewSequence(objects []*ast.TableName, obt objectType, ifExists bool) error {


The naming is weird. How about renaming it to dropTableObject and add some comments?

nice, addressed.

bb7133 · 2020-01-16T02:32:57Z

executor/ddl.go

+	viewObject
+	sequenceObject
+)
+
 func (e *DDLExec) executeDropTableOrView(s *ast.DropTableStmt) error {


Please separate this function to executeDropTable and executeDropView(just what you did for Sequences).

Both of them can simply call dropTableViewSequence

Got it, addressed.

AilinKid · 2020-01-17T05:03:41Z

It does not look like privilege checks are being done here. I assume it will be handled in a followup PR?

I've added the check, please take a look~

AilinKid · 2020-01-17T05:53:39Z

/build

executor/ddl.go

tangenta

Rest LGTM

ddl/sequence_test.go

AilinKid · 2020-01-19T12:12:05Z

In MariaDB,

mariadb root@172.17.0.3:test> show create table t
1 row in set
Time: 0.006s
+-------+----------------------------------------+
| Table | Create Table                           |
+-------+----------------------------------------+
| t     | CREATE TABLE `t` (                     |
|       |   `a` int(11) DEFAULT NULL             |
|       | ) ENGINE=InnoDB DEFAULT CHARSET=latin1 |
+-------+----------------------------------------+
mariadb root@172.17.0.3:test> drop sequence if exists t
Query OK, 0 rows affected
Time: 0.001s

Perhaps skip ErrWrongObject if if exists is specified.

Exactly right, I've fixed it, PTAL

tangenta

LGTM

meta/meta.go

ddl/ddl_worker.go

AilinKid · 2020-01-20T06:22:54Z

/run-unit-test

AilinKid · 2020-01-20T06:29:47Z

/run-unit-test

AilinKid · 2020-01-20T06:36:15Z

/run-all-tests

AilinKid · 2020-01-20T06:45:52Z

/rebuild

zimulala

LGTM

bb7133 · 2020-01-20T07:45:28Z

/merge

sre-bot · 2020-01-20T07:46:02Z

/run-all-tests

drop sequence

45c60da

AilinKid requested a review from a team as a code owner January 10, 2020 08:59

AilinKid added 2 commits January 10, 2020 17:21

.

e63cde8

.

b1d85ad

ghost requested review from francis0407 and alivxxx and removed request for a team January 10, 2020 09:28

AilinKid requested review from zimulala, bb7133 and djshow832 and removed request for francis0407 and alivxxx January 10, 2020 10:16

Merge branch 'master' into d_sequence

9583e3d

bb7133 reviewed Jan 10, 2020

View reviewed changes

bb7133 changed the title ~~ddl: Support drop sequence in TiDB.~~ ddl: support drop sequence in TiDB Jan 11, 2020

bb7133 added this to the v4.0.0 milestone Jan 16, 2020

bb7133 reviewed Jan 16, 2020

View reviewed changes

AilinKid and others added 2 commits January 17, 2020 13:00

Merge branch 'master' into d_sequence

06011fa

add drop privilege check

b7d0eae

fix test

8b6f928

zimulala added component/DDL-need-LGT3 type/new-feature labels Jan 17, 2020

bb7133 reviewed Jan 19, 2020

View reviewed changes

executor/ddl.go Show resolved Hide resolved

tangenta reviewed Jan 19, 2020

View reviewed changes

ddl/sequence_test.go Outdated Show resolved Hide resolved

ddl/sequence_test.go Show resolved Hide resolved

AilinKid and others added 2 commits January 19, 2020 20:15

drop ifExists

63288d8

Merge branch 'master' into d_sequence

e81f201

AilinKid requested review from tangenta and zimulala and removed request for zimulala January 19, 2020 12:31

tangenta reviewed Jan 19, 2020

View reviewed changes

tangenta added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jan 19, 2020

add drop sequence to convertJob2RollbackJob

b3243be

zimulala reviewed Jan 20, 2020

View reviewed changes

meta/meta.go Outdated Show resolved Hide resolved

zimulala reviewed Jan 20, 2020

View reviewed changes

ddl/ddl_worker.go Show resolved Hide resolved

address comment

90038b7

Merge branch 'master' into d_sequence

acf8056

zimulala approved these changes Jan 20, 2020

View reviewed changes

Merge branch 'master' into d_sequence

297555b

AilinKid added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Jan 20, 2020

Merge branch 'master' into d_sequence

8f432ef

sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 20, 2020

sre-bot merged commit b25c824 into pingcap:master Jan 20, 2020

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 sequence in TiDB #14442

ddl: support drop sequence in TiDB #14442

AilinKid commented Jan 10, 2020

bb7133 Jan 10, 2020

AilinKid Jan 11, 2020

wwar commented Jan 12, 2020

AilinKid commented Jan 15, 2020

bb7133 Jan 16, 2020

AilinKid Jan 17, 2020

bb7133 Jan 16, 2020

AilinKid Jan 17, 2020

AilinKid commented Jan 17, 2020

AilinKid commented Jan 17, 2020

tangenta left a comment

AilinKid commented Jan 19, 2020

tangenta left a comment

AilinKid commented Jan 20, 2020

AilinKid commented Jan 20, 2020

AilinKid commented Jan 20, 2020

AilinKid commented Jan 20, 2020

zimulala left a comment

bb7133 commented Jan 20, 2020

sre-bot commented Jan 20, 2020


		}

		func (e DDLExec) dropTableViewSequence(objects []ast.TableName, obt objectType, ifExists bool) error {

ddl: support drop sequence in TiDB #14442

ddl: support drop sequence in TiDB #14442

Conversation

AilinKid commented Jan 10, 2020

What problem does this PR solve?

What is changed and how it works?

Check List

bb7133 Jan 10, 2020

Choose a reason for hiding this comment

AilinKid Jan 11, 2020

Choose a reason for hiding this comment

wwar commented Jan 12, 2020

AilinKid commented Jan 15, 2020

bb7133 Jan 16, 2020

Choose a reason for hiding this comment

AilinKid Jan 17, 2020

Choose a reason for hiding this comment

bb7133 Jan 16, 2020

Choose a reason for hiding this comment

AilinKid Jan 17, 2020

Choose a reason for hiding this comment

AilinKid commented Jan 17, 2020

AilinKid commented Jan 17, 2020

tangenta left a comment

Choose a reason for hiding this comment

AilinKid commented Jan 19, 2020

tangenta left a comment

Choose a reason for hiding this comment

AilinKid commented Jan 20, 2020

AilinKid commented Jan 20, 2020

AilinKid commented Jan 20, 2020

AilinKid commented Jan 20, 2020

zimulala left a comment

Choose a reason for hiding this comment

bb7133 commented Jan 20, 2020

sre-bot commented Jan 20, 2020