ddl, planner: support `ALTER TABLE` without use db #18784

xhebox · 2020-07-25T11:04:22Z

What problem does this PR solve?

Issue Number: close #18756

Problem Summary: alter table db.t1 add constraint fk foreign key (c2) references t2(c1) without first executing use db, TiDB will report No database selected. That is because c2 has no schema(dbName), nor do we have a global CurrentDB. But mysql will try to use dbName from db.t1. Manually tested.

Check List

Tests

Unit test
Integration test

Release note

allow alter table db.t1 add constraint fk foreign key (c2) references t2(c1) like statements without first executing use db

codecov · 2020-07-25T11:16:18Z

Codecov Report

Merging #18784 into master will decrease coverage by 0.1866%.
The diff coverage is 100.0000%.

@@               Coverage Diff                @@
##             master     #18784        +/-   ##
================================================
- Coverage   79.3692%   79.1825%   -0.1867%     
================================================
  Files           552        549         -3     
  Lines        149369     148222      -1147     
================================================
- Hits         118553     117366      -1187     
- Misses        21343      21376        +33     
- Partials       9473       9480         +7

planner/core/preprocess.go

djshow832 · 2020-07-27T03:58:27Z

planner/core/preprocess.go

+		if currentDB := p.ctx.GetSessionVars().CurrentDB; currentDB != "" {
+			tn.Schema = model.NewCIStr(currentDB)
+		} else if p.tmpTs != "" {
+			tn.Schema = model.NewCIStr(p.tmpTs)
+		} else {


It doesn't check whether it's the renamed table name. It will affect all the ALTER TABLE statements. Does they all conform to this rule?

I have verified the case of rename, and the case in the original issue. I do not know other cases.

I just checked the source code of mysql, it is of great possibility. The related code is here, specifically the function mysql_opt_change_db.

@djshow832 Confirmed. It is at this line https://github.com/mysql/mysql-server/blob/7d10c82196c8e45554f27c00681474a9fb86d137/sql/sql_alter.cc#L142. So it conforms to all rules.

zimulala

I try to rename the table on MySQL 8.0.18 without executing the use test. It will return the error of No database selected. What is the version of MySQL you tested?

mysql> alter table test.t1 rename to t2;
ERROR 1046 (3D000): No database selected

xhebox · 2020-08-04T04:49:59Z

I do not remember that. I now retest it with mysql 8.0.20. And I can not reproduce that again. Maybe there was a mistake :(.

Foreign key works, rename/exchange partition do not work. For now, I modified it to only take effects on foreign key references.

`alter table db.t1 .... table t2` without `use db`, mysql will try to use dbName from `db.t1`.

AilinKid

LGTM

planner/core/preprocess.go

Co-authored-by: Lynn <zimu_xia@126.com>

planner/core/preprocess.go

xhebox · 2020-08-20T09:06:34Z

@ti-srebot /run-unit-test

ti-srebot · 2020-08-20T09:06:35Z

/run-unit-test

djshow832

It seems the code has nothing to do with rename to as the the description says.

xhebox · 2020-08-25T03:01:56Z

The description is updated.

planner/core/preprocess_test.go

djshow832 · 2020-08-25T07:31:12Z

planner/core/preprocess_test.go

+
+	s.runSQL(c, "ALTER TABLE test.t ADD CONSTRAINT fk FOREIGN KEY (c3) REFERENCES t (c1)", false, nil)
+
+	s.runSQL(c, "ALTER TABLE test.t ADD CONSTRAINT fk FOREIGN KEY t3(c3) REFERENCES t (c1)", false, nil)


I mean, REFERENCES test2.t(c1), where test2 is another schema.

Oh, that is a mistake.

Are you sure test2.t exists?

It seems that the underlying store is read only. Since the code here is not actually executed, but only parsed/resolved. I think it is ok to just use test.t.

So you can't judge from the test case that whether test is read from ALTER TABLE test.t or REFERENCES test.t(c1).

xhebox · 2020-08-26T03:47:13Z

@zz-jason Sounds good. I will do it later sometime.

djshow832 · 2020-08-26T04:31:14Z

@zz-jason Sounds good. I will do it later sometime.

You can do it simply by adding a needs-cherry-pick-4.0 or needs-cherry-pick-3.0 label. The robot will cherry-pick and commit a PR.

xhebox · 2020-08-26T04:47:46Z

You can do it simply by adding a label

I thought that I do not have the permission to add a label?

xhebox · 2020-08-26T05:13:43Z

@tangenta I think that should be tested in DDL package.

Here, this PR is just a change in preprocess. The tests here are enough.

djshow832

LGTM

djshow832 · 2020-08-26T07:16:49Z

/merge

ti-srebot · 2020-08-26T07:16:52Z

Your auto merge job has been accepted, waiting for:

19447

ti-srebot · 2020-08-26T07:23:22Z

/run-all-tests

ti-srebot · 2020-08-26T07:27:54Z

/run-all-tests

ti-srebot · 2020-08-26T07:29:55Z

@xhebox merge failed.

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2020-08-26T07:34:47Z

cherry pick to release-4.0 in PR #19471

xhebox requested a review from a team as a code owner July 25, 2020 11:04

xhebox requested review from lzmhhh123 and removed request for a team July 25, 2020 11:04

ti-srebot added the contribution This PR is from a community contributor. label Jul 25, 2020

djshow832 reviewed Jul 27, 2020

View reviewed changes

djshow832 requested review from zimulala and AilinKid July 31, 2020 03:37

zimulala reviewed Aug 4, 2020

View reviewed changes

xhebox added 2 commits August 4, 2020 12:52

ddl, planner: support ALTER TABLE without use db

0429778

`alter table db.t1 .... table t2` without `use db`, mysql will try to use dbName from `db.t1`.

ddl,planner: only effect foreign key reference

c9ddd3a

xhebox force-pushed the 18756 branch from 8fc9b76 to c9ddd3a Compare August 4, 2020 04:52

zimulala added the sig/sql-infra SIG: SQL Infra label Aug 4, 2020

Merge branch 'master' into 18756

46b6aa4

AilinKid reviewed Aug 20, 2020

View reviewed changes

ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 20, 2020

Merge branch 'master' into 18756

c407473

zimulala reviewed Aug 20, 2020

View reviewed changes

planner/core/preprocess.go Outdated Show resolved Hide resolved

ddl: merge condition

b0a31dd

Co-authored-by: Lynn <zimu_xia@126.com>

zimulala reviewed Aug 20, 2020

View reviewed changes

planner/core/preprocess.go Outdated Show resolved Hide resolved

planner/core/preprocess.go Show resolved Hide resolved

ddl: fix

1489867

djshow832 reviewed Aug 25, 2020

View reviewed changes

planner/core/preprocess_test.go Outdated Show resolved Hide resolved

xhebox added 2 commits August 25, 2020 13:16

planner: refactor the test

5d33e8d

planner: format error

d743364

djshow832 reviewed Aug 25, 2020

View reviewed changes

xhebox changed the title ~~ddl, planner: support ALTER TABLE without use db~~ [DNM]ddl, planner: support ALTER TABLE without use db Aug 26, 2020

djshow832 added the needs-cherry-pick-4.0 label Aug 26, 2020

planner: fix test

0b1ca55

xhebox dismissed ti-srebot’s stale review via 0b1ca55 August 26, 2020 05:11

planner: fix

9b8f9f4

xhebox changed the title ~~[DNM]ddl, planner: support ALTER TABLE without use db~~ ddl, planner: support ALTER TABLE without use db Aug 26, 2020

planner: improve test

40839e7

djshow832 approved these changes Aug 26, 2020

View reviewed changes

ti-srebot added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Aug 26, 2020

ti-srebot approved these changes Aug 26, 2020

View reviewed changes

ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 26, 2020

Merge branch 'master' into 18756

e67398f

Merge branch 'master' into 18756

16876d8

djshow832 merged commit c6b80ea into pingcap:master Aug 26, 2020

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Aug 26, 2020

cherry pick pingcap#18784 to release-4.0

533d027

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot mentioned this pull request Aug 26, 2020

ddl, planner: support ALTER TABLE without use db (#18784) #19471

Merged

zz-jason pushed a commit that referenced this pull request Sep 2, 2020

ddl, planner: support ALTER TABLE without use db (#18784) (#19471)

54840da

xhebox deleted the 18756 branch September 15, 2020 00:31

xhebox mentioned this pull request Jul 7, 2021

Define the placement of data by SQL statements #18030

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl, planner: support `ALTER TABLE` without use db #18784

ddl, planner: support `ALTER TABLE` without use db #18784

xhebox commented Jul 25, 2020 •

edited

Loading

codecov bot commented Jul 25, 2020 •

edited

Loading

djshow832 Jul 27, 2020

xhebox Jul 27, 2020

xhebox Jul 28, 2020

zimulala left a comment

xhebox commented Aug 4, 2020 •

edited

Loading

AilinKid left a comment

xhebox commented Aug 20, 2020

ti-srebot commented Aug 20, 2020

djshow832 left a comment

xhebox commented Aug 25, 2020

djshow832 Aug 25, 2020

xhebox Aug 25, 2020

djshow832 Aug 25, 2020

xhebox Aug 25, 2020

djshow832 Aug 26, 2020

xhebox commented Aug 26, 2020

djshow832 commented Aug 26, 2020

xhebox commented Aug 26, 2020 •

edited

Loading

xhebox commented Aug 26, 2020

djshow832 left a comment

djshow832 commented Aug 26, 2020

ti-srebot commented Aug 26, 2020

ti-srebot commented Aug 26, 2020

ti-srebot commented Aug 26, 2020

ti-srebot commented Aug 26, 2020

ti-srebot commented Aug 26, 2020


		s.runSQL(c, "ALTER TABLE test.t ADD CONSTRAINT fk FOREIGN KEY (c3) REFERENCES t (c1)", false, nil)

		s.runSQL(c, "ALTER TABLE test.t ADD CONSTRAINT fk FOREIGN KEY t3(c3) REFERENCES t (c1)", false, nil)

ddl, planner: support ALTER TABLE without use db #18784

ddl, planner: support ALTER TABLE without use db #18784

Conversation

xhebox commented Jul 25, 2020 • edited Loading

What problem does this PR solve?

Check List

Release note

codecov bot commented Jul 25, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zimulala left a comment

Choose a reason for hiding this comment

xhebox commented Aug 4, 2020 • edited Loading

AilinKid left a comment

Choose a reason for hiding this comment

xhebox commented Aug 20, 2020

ti-srebot commented Aug 20, 2020

djshow832 left a comment

Choose a reason for hiding this comment

xhebox commented Aug 25, 2020

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

xhebox commented Aug 26, 2020

djshow832 commented Aug 26, 2020

xhebox commented Aug 26, 2020 • edited Loading

xhebox commented Aug 26, 2020

djshow832 left a comment

Choose a reason for hiding this comment

djshow832 commented Aug 26, 2020

ti-srebot commented Aug 26, 2020

ti-srebot commented Aug 26, 2020

ti-srebot commented Aug 26, 2020

ti-srebot commented Aug 26, 2020

ti-srebot commented Aug 26, 2020

ddl, planner: support `ALTER TABLE` without use db #18784

ddl, planner: support `ALTER TABLE` without use db #18784

xhebox commented Jul 25, 2020 •

edited

Loading

codecov bot commented Jul 25, 2020 •

edited

Loading

xhebox commented Aug 4, 2020 •

edited

Loading

xhebox commented Aug 26, 2020 •

edited

Loading