Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 9 commits into from

Conversation

bb7133
Copy link
Member

@bb7133 bb7133 commented Sep 26, 2018

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 Reviewable

@bb7133 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 zz-jason added contribution This PR is from a community contributor. component/DDL-need-LGT3 labels Sep 27, 2018
@winkyao
Copy link
Contributor

winkyao commented Sep 27, 2018

@bb7133 Great work! thanks

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

bb7133 commented Sep 27, 2018

@bb7133 Great work! thanks

Thank you for all the helps and advices!

@winoros
Copy link
Member

winoros commented Sep 27, 2018

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

@bb7133
Copy link
Member Author

bb7133 commented Sep 28, 2018

@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!

executor/ddl.go Outdated Show resolved Hide resolved
ddl/ddl_api.go Outdated
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ddl/ddl_api.go Outdated Show resolved Hide resolved
ddl/table.go Show resolved Hide resolved
ddl/table.go Outdated Show resolved Hide resolved
ddl/table.go Show resolved Hide resolved
ddl/table.go Outdated Show resolved Hide resolved
ddl/table.go Show resolved Hide resolved
@bb7133
Copy link
Member Author

bb7133 commented Oct 21, 2018

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 Outdated Show resolved Hide resolved
ddl/table.go Outdated Show resolved Hide resolved
ddl/table.go Outdated Show resolved Hide resolved
ddl/table.go Show resolved Hide resolved
ddl/table.go Show resolved Hide resolved
@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, thanks!

// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@winkyao winkyao Dec 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. You're right, I didn't treat here carefully, the current codes just work because Allocator.Rebase prevented base id rolling back.
  2. There is a test case already, but now I think it can be enhanced.

Thanks for the review

@winkyao
Copy link
Contributor

winkyao commented Dec 8, 2018

@winoros @zz-jason PTAL

@bb7133
Copy link
Member Author

bb7133 commented Dec 16, 2018

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

@bb7133 bb7133 force-pushed the bb7133/create_select_impl branch 2 times, most recently from c1f90a5 to 3b40c98 Compare December 24, 2018 01:54
ddl/ddl.go Outdated
@@ -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()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, thank you!

@@ -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:
Copy link
Contributor

@crazycs520 crazycs520 Jan 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tips, let me refine the code

ddl/table.go Outdated
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

%s/handle/handles

mockTablePlan := LogicalTableDual{}.Init(b.ctx)
mockTablePlan.SetSchema(expression.NewSchema(mockExprs...))

genCols, err := b.resolveGeneratedColumns(mockColumns, nil, mockTablePlan)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed, thanks

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
Copy link

codecov-io commented Jan 17, 2019

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Member Author

bb7133 commented Jan 17, 2019

@crazycs520 PTAL, thank you!

@bb7133
Copy link
Member Author

bb7133 commented Aug 2, 2019

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 bb7133 closed this Aug 2, 2019
@you06 you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020
@bb7133 bb7133 deleted the bb7133/create_select_impl branch December 29, 2023 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor. sig/sql-infra SIG: SQL Infra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

create table select from not supported. Create table select from not supported
7 participants