ddl, planner, executor: implement CREATE TABLE ... SELECT #7787

bb7133 · 2018-09-26T11:14:52Z

What problem does this PR solve?

Implement 'CREATE TALBE ... SELECT' syntax

What is changed and how it works?

Please check the this doc for the design of this PR: https://docs.google.com/document/d/116QWf7AOQKBCcB-oMXV6mxB8sfdo5NOnEulwr_8_yJQ/edit?usp=sharing

Check List

Tests

Unit test
Integration test

Code changes

Has exported function/method change
Has exported variable/fields change
Has interface methods change

Side effects

Increased code complexity

Related changes

Need to update the documentation
Need to be included in the release note

Related Issues/PR

Others

The design/implementation in this PR is with flaws: 'CREATE TABLE ... SELECT' is not actually one transaction. Please evaluate whether this is acceptable: if not, I will try to implement the 'alternative solution'(described in the design doc) in the future.

This change is

winkyao · 2018-09-27T01:41:18Z

@bb7133 Great work! thanks

bb7133 · 2018-09-27T09:51:15Z

@bb7133 Great work! thanks

Thank you for all the helps and advices!

winoros · 2018-09-27T12:09:34Z

@bb7133 Friendly reminding. You can use fix + issue link grammar since this pr will totally solve that two issues.

bb7133 · 2018-09-28T15:23:26Z

@bb7133 Friendly reminding. You can use fix + issue link grammar since this pr will totally solve that two issues.

I didn't know this feature before, thank you!

planner/core/planbuilder.go

executor/ddl.go

winkyao · 2018-10-15T11:48:51Z

ddl/ddl_api.go

@@ -965,10 +966,10 @@ func (d *ddl) CreateTable(ctx sessionctx.Context, s *ast.CreateTableStmt) (err e

 	// table exists, but if_not_exists flags is true, so we ignore this error.
 	if infoschema.ErrTableExists.Equal(err) && s.IfNotExists {
-		return nil
+		return tbInfo.ID, nil


If table exists, we need to execute the insertion of create-select?

Of course we shouldn't, I will fix this, thanks!

ddl/ddl_api.go

ddl/table.go

bb7133 · 2018-10-21T12:56:40Z

Hi @winkyao, thanks for all the reviews! I was just too busy to deal with all of them in the last few days. Hope I can address them soon.

ddl/table.go

winkyao · 2018-12-08T07:41:04Z

executor/builder.go

@@ -656,12 +660,74 @@ func (b *executorBuilder) buildRevoke(revoke *ast.RevokeStmt) Executor {
 }

 func (b *executorBuilder) buildDDL(v *plannercore.DDL) Executor {
-	e := &DDLExec{
+	if b.ctx.GetSessionVars().CreateTableInsertingID != 0 {
+		// in a 'inserting data from select' state of creating table.


Could you add more comments here, to explain why we implement create select so weird? And add a TODO.

Sure, thanks!

winkyao · 2018-12-08T08:50:44Z

executor/builder.go

+	// The operation of the minus 1 to make sure that the current value doesn't be used,
+	// the next Alloc operation will get this value.
+	// Its behavior is consistent with MySQL.
+	if err = tbl.RebaseAutoID(nil, tbInfo.AutoIncID-1, false); err != nil {


The auto increment id will be rebased after the table create finished, create select rebase it here, so if the create_select is true, we should not set auto-id in https://github.com/pingcap/tidb/blob/master/ddl/ddl_api.go#L964?

Could we add a test case like create table auto_increment= xxx select ?

You're right, I didn't treat here carefully, the current codes just work because Allocator.Rebase prevented base id rolling back.

There is a test case already, but now I think it can be enhanced.

Thanks for the review

winkyao · 2018-12-08T10:07:37Z

@winoros @zz-jason PTAL

bb7133 · 2018-12-16T14:35:01Z

@winkyao comments addressed, PTAL, thank you for your review!

crazycs520 · 2019-01-09T07:18:42Z

ddl/ddl.go

@@ -540,6 +543,7 @@ func (d *ddl) doDDLJob(ctx sessionctx.Context, job *model.Job) error {
 		// If a job is a history job, the state must be JobSynced or JobCancel.
 		if historyJob.IsSynced() {
 			log.Infof("[ddl] DDL job ID:%d is finished", jobID)
+			ctx.GetSessionVars().StmtCtx.AddAffectedRows(uint64(historyJob.GetRowCount()))


Watch out, add index also use job.RowCount. So this will make add index return xx row affected.
In Mysql:

mysql root@127.0.0.1:test> create table t1 (a int); Query OK, 0 rows affected Time: 0.010s mysql root@127.0.0.1:test> insert into t1 values (0),(1),(2),(3),(4),(5); Query OK, 6 rows affected Time: 0.001s mysql root@127.0.0.1:test> alter table t1 add index idx(a); Query OK, 0 rows affected Time: 0.008s mysql root@127.0.0.1:test> create table t2 select a from t1; Query OK, 6 rows affected Time: 0.007s

In This PR:

mysql root@127.0.0.1:test> create table t1 (a int); Query OK, 0 rows affected Time: 0.008s mysql root@127.0.0.1:test> insert into t1 values (0),(1),(2),(3),(4),(5); Query OK, 6 rows affected Time: 0.001s mysql root@127.0.0.1:test> alter table t1 add index idx(a); Query OK, 6 rows affected Time: 0.016s mysql root@127.0.0.1:test> create table t2 select a from t1; Query OK, 6 rows affected Time: 0.012s

Sure, thank you!

crazycs520 · 2019-01-09T08:19:12Z

ddl/delete_range.go

@@ -243,6 +243,25 @@ func insertJobIntoDeleteRangeTable(ctx sessionctx.Context, job *model.Job) error
 		startKey = tablecodec.EncodeTablePrefix(tableID)
 		endKey := tablecodec.EncodeTablePrefix(tableID + 1)
 		return doInsert(s, job.ID, tableID, startKey, endKey, now)
+	case model.ActionCreateTable:


Can we change the job Args to same with drop table? Then we can reuse the drop table code here. You can refer to create index rolling back. But create index does't reuse the drop index code here is for compatibility. create table rolling back doesn't have compatibility problem.

Or create a function to remove redundant code in here and drop table.

Thanks for the tips, let me refine the code

crazycs520 · 2019-01-09T08:47:07Z

ddl/table.go

@@ -477,6 +523,7 @@ func updateVersionAndTableInfo(t *meta.Meta, job *model.Job, tblInfo *model.Tabl
 	return ver, t.UpdateTable(job.SchemaID, tblInfo)
 }

+// onAddTablePartition handle ActionAddTablePartition DDL job


%s/handle/handles

crazycs520 · 2019-01-09T09:49:25Z

planner/core/planbuilder.go

+	mockTablePlan := LogicalTableDual{}.Init(b.ctx)
+	mockTablePlan.SetSchema(expression.NewSchema(mockExprs...))
+
+	genCols, err := b.resolveGeneratedColumns(mockColumns, nil, mockTablePlan)


In mysql:

mysql root@127.0.0.1:test> create table t1 (a int); Query OK, 0 rows affected Time: 0.011s mysql root@127.0.0.1:test> insert into t1 values (0),(1),(2),(3),(4),(5); Query OK, 6 rows affected Time: 0.001s mysql root@127.0.0.1:test> create table t2 (a int key, b int as (a*2)) select a,a+1 as b from t1; (3105, u"The value specified for generated column 'b' in table 't2' is not allowed.")

In TiDB this PR:

mysql root@127.0.0.1:test> create table t1 (a int); Query OK, 0 rows affected Time: 0.005s mysql root@127.0.0.1:test> insert into t1 values (0),(1),(2),(3),(4),(5); Query OK, 6 rows affected Time: 0.004s mysql root@127.0.0.1:test> create table t2 (a int key, b int as (a*2)) select a,a+1 as b from t1; Query OK, 6 rows affected Time: 0.014s

fixed, thanks

fix pingcap#4754, fix pingcap#3839

1. fixed some codestyles 2. added more comments 3. changed the mechnism to insert data(from session server to ddl owner) 4. added support for generated columns 5. added more tests

1. refactor CreateTeableInsertExec to re-use current executors 2. added more test cases

codecov-io · 2019-01-17T14:14:00Z

Codecov Report

Merging #7787 into master will decrease coverage by 0.1%.
The diff coverage is 46.31%.

@@            Coverage Diff             @@
##           master    #7787      +/-   ##
==========================================
- Coverage   67.14%   67.03%   -0.11%     
==========================================
  Files         372      373       +1     
  Lines       76974    77323     +349     
==========================================
+ Hits        51682    51837     +155     
- Misses      20661    20835     +174     
- Partials     4631     4651      +20

Impacted Files	Coverage Δ
planner/core/common_plans.go	`60.98% <ø> (ø)`	⬆️
sessionctx/variable/session.go	`29.8% <0%> (-0.2%)`	⬇️
infoschema/builder.go	`75% <0%> (-0.75%)`	⬇️
executor/executor.go	`67.44% <100%> (+0.4%)`	⬆️
executor/create_table_insert.go	`100% <100%> (ø)`
ddl/ddl.go	`89.62% <100%> (+0.08%)`	⬆️
ddl/delete_range.go	`79.36% <100%> (+4.23%)`	⬆️
planner/core/errors.go	`100% <100%> (ø)`	⬆️
types/field_type.go	`76.52% <100%> (+1.27%)`	⬆️
session/session.go	`71.76% <11.11%> (-0.59%)`	⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f8265a...ae22d31. Read the comment docs.

1. fix a bug inserting data into generated columns when creating table 2. fix a 'affected rows' count bug for ddl job 3. refined 'ActionCreateTable' code in delete_range.go 4. fix merge conflict

bb7133 · 2019-01-17T14:39:45Z

@crazycs520 PTAL, thank you!

bb7133 · 2019-08-02T06:47:03Z

Now it's very, very difficult to resolve the conflicts from a long commit list. I will close this PR and re-submit a new one.

Thanks for the help @winkyao @crazycs520

bb7133 force-pushed the bb7133/create_select_impl branch 2 times, most recently from 98bbb05 to 73afbf2 Compare September 26, 2018 11:24

zz-jason added contribution This PR is from a community contributor. component/DDL-need-LGT3 labels Sep 27, 2018

bb7133 force-pushed the bb7133/create_select_impl branch 2 times, most recently from 1b04937 to fb78958 Compare September 27, 2018 05:59

winoros mentioned this pull request Sep 27, 2018

tidb-server memory usage goes to 50G pingcap/docs-cn#862

Closed

bb7133 force-pushed the bb7133/create_select_impl branch from fb78958 to b7bddf6 Compare September 28, 2018 15:21

winkyao reviewed Oct 12, 2018

View reviewed changes

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

winkyao reviewed Oct 15, 2018

View reviewed changes

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

winkyao reviewed Oct 15, 2018

View reviewed changes

planner/core/planbuilder.go Show resolved Hide resolved

winkyao reviewed Oct 15, 2018

View reviewed changes

planner/core/planbuilder.go Show resolved Hide resolved

winkyao reviewed Oct 15, 2018

View reviewed changes

executor/ddl.go Outdated Show resolved Hide resolved

winkyao reviewed Oct 15, 2018

View reviewed changes

ddl/ddl_api.go Outdated Show resolved Hide resolved