ddl: add check when create table with foreign key. #7885

crazycs520 · 2018-10-12T12:45:17Z

What problem does this PR solve?

below sql will cause tidb panic:

create table t1 (a int, b int);
create table t2 (c int, foreign key (d) references t1(a));  
select * from information_schema.key_column_usage;

create table t2 (c int, foreign key (a) references t1(a)); should return error:
key column d doesn't exist in table,
but currently tidb will create table t2 successfully. then next select will cause panic.

What is changed and how it works?

Add check when create table with foreign key.

Check List

Tests

Integration test

Code changes

Has exported function/method change

Related changes

eurekaka · 2018-10-12T13:31:51Z

infoschema/tables.go

@@ -1266,6 +1266,9 @@ func keyColumnUsageInTable(schema *model.DBInfo, table *model.TableInfo) [][]typ
 		}
 		for i, key := range fk.Cols {
 			col := nameToCol[key.L]
+			if col == nil {


I prefer not to add this check, this should act as something like assert to expose inconsistency problems.

alivxxx · 2018-10-15T03:06:25Z

table/column.go

+// FindColInfo finds column in cols by name.
+func FindColInfo(cols []*model.ColumnInfo, name string) bool {
+	for _, col := range cols {
+		if strings.EqualFold(col.Name.O, name) {


You pass the name by key.Column.Name.L, but here check with col.Name.O.

jackysp · 2018-10-15T03:26:31Z

ddl/ddl_api.go

@@ -727,6 +727,9 @@ func buildTableInfo(ctx sessionctx.Context, d *ddl, tableName model.CIStr, cols
 			fk.RefTable = constr.Refer.Table.Name
 			fk.State = model.StatePublic
 			for _, key := range constr.Keys {
+				if !table.FindColInfo(tbInfo.Columns, key.Column.Name.L) {
+					return nil, errKeyColumnDoesNotExits.GenWithStack("key column %s doesn't exist in table", key.Column.Name)


Why not use GenByArgs here?

There's no GenByArgs method now.

emmm... GenWithStackByArgs

jackysp · 2018-10-15T03:26:52Z

executor/executor_test.go

+	tk.MustExec("use test")
+	tk.MustExec("create table t1 (a int, b int);")
+	_, err := tk.Exec("create table t2 (c int, foreign key (a) references t1(a));")
+	c.Assert(err, NotNil)


Should we compare the error message?

jackysp

LGTM

zimulala · 2018-10-16T10:54:56Z

ddl/ddl_api.go

@@ -727,6 +727,9 @@ func buildTableInfo(ctx sessionctx.Context, d *ddl, tableName model.CIStr, cols
 			fk.RefTable = constr.Refer.Table.Name
 			fk.State = model.StatePublic
 			for _, key := range constr.Keys {
+				if !table.FindColInfo(tbInfo.Columns, key.Column.Name.O) {


Can we use "table.FindCol" directly?

…eign-key-bug

crazycs520 · 2018-10-16T13:57:59Z

/run-all-tests

eurekaka

LGTM

zimulala

LGTM

bugfix fixed pingcap#7518 expression: MySQL compatible current_user function (pingcap#7801) plan: propagate constant over outer join (pingcap#7794) - extract `outerCol = const` from join conditions and filter conditions, substitute `outerCol` in join conditions with `const`; - extract `outerCol = innerCol` from join conditions, derive new join conditions based on this column equal condition and `outerCol` related expressions in join conditions and filter conditions; util/timeutil: fix data race caused by forgetting set stats lease to 0 (pingcap#7901) stats: handle ddl event for partition table (pingcap#7903) plan: implement Operand and Pattern of cascades planner. (pingcap#7910) planner: not convert to TableDual if empty range is derived from deferred constants (pingcap#7808) plan: move projEliminate behind aggEliminate (pingcap#7909) admin: fix admin check table bug of byte compare (pingcap#7887) * admin: remove reflect deepEqual stats: fix panic caused by empty histogram (pingcap#7912) plan: fix panic caused by empty schema of LogicalTableDual (pingcap#7906) * fix drop view if exist error (pingcap#7833) executor: refine `explain analyze` (pingcap#7888) executor: add an variable to compatible with MySQL insert for OGG (pingcap#7863) expression: maintain `DeferredExpr` in aggressive constant folding. (pingcap#7915) stats: fix histogram boundaries overflow error (pingcap#7883) ddl:support the definition of `null` change to `not null` using `alter table` (pingcap#7771) * ddl:support the definition of null change to not null using alter table ddl: add check when create table with foreign key. (pingcap#7885) * ddl: add check when create table with foreign key planner: eliminate if null on non null column (pingcap#7924) executor: fix a bug in point get (pingcap#7934) planner, executor: refine ColumnPrune for LogicalUnionAll (pingcap#7930) executor: fix panic when limit is too large (pingcap#7936) ddl: add TiDB version to metrics (pingcap#7902) stats: limit the length of sample values (pingcap#7931) vendor: update tipb (pingcap#7893) planner: support the Group and GroupExpr for the cascades planner (pingcap#7917) store/tikv: log more information when other err occurs (pingcap#7948) types: fix date time parse (pingcap#7933) ddl: just print error message when ddl job is normal to calcel, to eliminate noisy log (pingcap#7875) stats: update delta info for partition table (pingcap#7947) explaintest: add explain test for partition pruning (pingcap#7505) util: move disjoint set to util package (pingcap#7950) util: add PreAlloc4Row and Insert for Chunk and List (pingcap#7916) executor: add the slow log for commit (pingcap#7951) expression: add builtin json_keys (pingcap#7776) privilege: add USAGE in `show grants` for mysql compatibility (pingcap#7955) ddl: fix invailid ddl job panic (pingcap#7940) *: move ast.NewValueExpr to standalone parser_driver package (pingcap#7952) Make the ast package get rid of the dependency of types.Datum server: allow cors http request (pingcap#7939) *: move `Statement` and `RecordSet` from ast to sqlexec package (pingcap#7970) pr suggestion update executor/aggfuncs: split unit tests to corresponding file (pingcap#7993) store/tikv: fix typo (pingcap#7990) executor, planner: clone proj schema for different children in buildProj4Union (pingcap#7999) executor: let information_schema be the first database in ShowDatabases (pingcap#7938) stats: use local feedback for partition table (pingcap#7963) executor: add unit test for aggfuncs (pingcap#7966) server: add log for binary execute statement (pingcap#7987) admin: refine admin check decoder (pingcap#7862) executor: improve wide table insert & update performance (pingcap#7935) ddl: fix reassigned partition id in `truncate table` does not take effect (pingcap#7919) fix reassigned partition id in truncate table does not take effect add changelog for 2.1.0 rc4 (pingcap#8020) *: make parser package dependency as small as possible (pingcap#7989) parser: support `:=` in the `set` syntax (pingcap#8018) According to MySQL document, `set` use the = assignment operator, but the := assignment operator is also permitted stats: garbage collect stats for partition table (pingcap#7962) docs: add the proposal for the column pool (pingcap#7988) expression: refine built-in func truncate to support uint arg (pingcap#8000) stats: support show stats for partition table (pingcap#8023) stats: update error rate for partition table (pingcap#8022) stats: fix estimation for out of range point queries (pingcap#8015) *: move parser to a separate repository (pingcap#8036) executor: fix wrong result when index join on union scan. (pingcap#8031) Do not modify Plan of dataReaderBuilder directly, because it would impact next batch of outer rows, as well as other concurrent inner workers. Instead, build a local child builder to store the child plan. planner: fix a panic of a cached prepared statement with IndexScan (pingcap#8017) *: fix the issue of executing DDL after executing SQL failure in txn (pingcap#8044) * ddl, executor: fix the issue of executing DDL after executing SQL failure in txn add unit test remove debug info add like evaluator case sensitive test ddl, domain: make schema correct after canceling jobs (pingcap#7997) unit test fix code format proposal: maintaining histograms in plan. (pingcap#7605) support _tidb_rowid for table scan range (pingcap#8047) var rename fix

* ddl: add check when create table with foreign key

crazycs520 added component/DDL-need-LGT3 type/bugfix This PR fixes a bug. labels Oct 12, 2018

eurekaka reviewed Oct 12, 2018

View reviewed changes

crazycs520 dismissed a stale review via 88a2f13 October 12, 2018 16:25

crazycs520 force-pushed the fix-foreign-key-bug branch from 51d598b to 88a2f13 Compare October 12, 2018 16:25

alivxxx reviewed Oct 15, 2018

View reviewed changes

jackysp reviewed Oct 15, 2018

View reviewed changes

crazycs520 force-pushed the fix-foreign-key-bug branch from 56e8146 to aeb7f19 Compare October 15, 2018 04:24

jackysp reviewed Oct 15, 2018

View reviewed changes

crazycs520 added 4 commits October 15, 2018 13:14

ddl: add check when create table with foreign key

c2b0871

address comment

2a3ab76

address comment

f67aeeb

address comment

88106f5

crazycs520 force-pushed the fix-foreign-key-bug branch from c848f74 to 88106f5 Compare October 15, 2018 05:15

zimulala reviewed Oct 16, 2018

View reviewed changes

crazycs520 added 4 commits October 16, 2018 19:20

address comment

79f38c8

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

eb05fed

…eign-key-bug

refine test

64d0ae6

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

34132cd

…eign-key-bug

eurekaka reviewed Oct 17, 2018

View reviewed changes

eurekaka added status/all tests passed status/LGT2 Indicates that a PR has LGTM 2. labels Oct 17, 2018

zimulala approved these changes Oct 17, 2018

View reviewed changes

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

crazycs520 added 2 commits October 17, 2018 11:34

Merge branch 'master' into fix-foreign-key-bug

80b4d70

Merge branch 'master' into fix-foreign-key-bug

6e593c0

Merge branch 'master' into fix-foreign-key-bug

d200d8b

ciscoxll merged commit 732233a into pingcap:master Oct 17, 2018

crazycs520 added a commit to crazycs520/tidb that referenced this pull request Nov 22, 2018

ddl: add check when create table with foreign key. (pingcap#7885)

4b4c443

* ddl: add check when create table with foreign key

crazycs520 added a commit to crazycs520/tidb that referenced this pull request Nov 22, 2018

ddl: add check when create table with foreign key. (pingcap#7885)

8293831

* ddl: add check when create table with foreign key

This was referenced Nov 22, 2018

ddl: add check when create table with foreign key. (#7885) #8410

Merged

ddl: add check when create table with foreign key. (#7885) #8407

Merged

zz-jason pushed a commit that referenced this pull request Nov 23, 2018

ddl: add check when create table with foreign key. (#7885) (#8407)

830c013

winkyao pushed a commit that referenced this pull request Nov 23, 2018

ddl: add check when create table with foreign key. (#7885) (#8410)

3f03b30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: add check when create table with foreign key. #7885

ddl: add check when create table with foreign key. #7885

crazycs520 commented Oct 12, 2018 •

edited

Loading

eurekaka Oct 12, 2018

alivxxx Oct 15, 2018

crazycs520 Oct 15, 2018

jackysp Oct 15, 2018

crazycs520 Oct 15, 2018

jackysp Oct 15, 2018

crazycs520 Oct 15, 2018

jackysp Oct 15, 2018

jackysp left a comment

zimulala Oct 16, 2018 •

edited

Loading

crazycs520 commented Oct 16, 2018

eurekaka left a comment

zimulala left a comment

ddl: add check when create table with foreign key. #7885

ddl: add check when create table with foreign key. #7885

Conversation

crazycs520 commented Oct 12, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackysp left a comment

Choose a reason for hiding this comment

zimulala Oct 16, 2018 • edited Loading

Choose a reason for hiding this comment

crazycs520 commented Oct 16, 2018

eurekaka left a comment

Choose a reason for hiding this comment

zimulala left a comment

Choose a reason for hiding this comment

crazycs520 commented Oct 12, 2018 •

edited

Loading

zimulala Oct 16, 2018 •

edited

Loading