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: support alter algorithm INPLACE/INSTANT #8811

Merged
merged 17 commits into from Feb 20, 2019

Conversation

@winkyao
Copy link
Member

commented Dec 25, 2018

What problem does this PR solve?

For #8428 , this is the first step to support EXPLAIN DDL(#8429).

MySQL support COPY/INPLACE/INSTANT/DEFAULT alter algorithm, you can get the details in https://dev.mysql.com/doc/refman/8.0/en/alter-table.html#alter-table-performance

In TiDB, we only support INPLACE/INSTANT, and the most algorithm of the alter operations is the INSTANT, which means

Operations only modify metadata in the data dictionary. No exclusive metadata locks are taken on the table during preparation and execution, and table data is unaffected, making operations instantaneous. Concurrent DML is permitted.

The add index using INPLACE algorithm, but not exactly the same with MySQL, the difference with MySQL is that TiDB never blocks the DML and never have a metadata lock on the table:

Operations avoid copying table data but may rebuild the table in place. An exclusive metadata lock on the table may be taken briefly during the preparation and execution phases of the operation. Typically, concurrent DML is supported.

If user specifies the unsupported algorithm of the alter operation, a wanrning ERROR 1846 (0A000): ALGORITHM=COPY is not supported. Reason: Cannot alter table by COPY. Try ALGORITHM=INSTANT. will be returned:

mysql> create table t(a int);
Query OK, 0 rows affected (0.00 sec)

mysql> alter table t comment = 'test', ALGORITHM=COPY;
Query OK, 0 rows affected, 1 warning (0.00 sec)

mysql> show warnings;
+-------+------+---------------------------------------------------------------------------------------------+
| Level | Code | Message                                                                                     |
+-------+------+---------------------------------------------------------------------------------------------+
| Error | 1846 | ALGORITHM=COPY is not supported. Reason: Cannot alter table by COPY. Try ALGORITHM=INSTANT. |
+-------+------+---------------------------------------------------------------------------------------------+
1 row in set (0.00 sec)

Need to merge pingcap/parser#93 first.

Tests

  • Unit test

Code changes

  • Has exported function/method change

Side effects

  • Increased code complexity
  • Breaking backward compatibility

Related changes

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

This change is Reviewable

@zimulala @crazycs520 @morgo PTAL

@winkyao

This comment has been minimized.

Copy link
Member Author

commented Dec 25, 2018

The circleci will be fixed after the pingcap/parser#93 merged.

@@ -0,0 +1,107 @@
// Copyright 2015 PingCAP, Inc.

This comment has been minimized.

Copy link
@shenli

shenli Dec 26, 2018

Member

2015 -> 2018

// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSES/QL-LICENSE file.

// Copyright 2015 PingCAP, Inc.

This comment has been minimized.

Copy link
@shenli

shenli Dec 26, 2018

Member

2015 -> 2018

go.mod Outdated
@@ -86,3 +86,5 @@ require (
sourcegraph.com/sourcegraph/appdash v0.0.0-20180531100431-4c381bd170b4
sourcegraph.com/sourcegraph/appdash-data v0.0.0-20151005221446-73f23eafcf67
)

replace github.com/pingcap/parser => github.com/winkyao/parser v0.0.0-20181221090435-0eaad9528419

This comment has been minimized.

Copy link
@shenli

shenli Dec 26, 2018

Member

Should we remove this line?

This comment has been minimized.

Copy link
@winkyao

winkyao Dec 26, 2018

Author Member

this line will be removed after the pingcap/parser#93 is merged, but now we need to replace the parser to pass the ci.

winkyao added some commits Dec 27, 2018

@morgo

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2019

@winkyao I spoke to MySQL users about the error on ALGORITHM=COPY behavior, and they reminded me that TiDB might be replicating from a MySQL system. A warning is better behavior, since it won't break replication if an upstream MySQL server sends this DDL event.

@GregoryIan

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2019

@winkyao what do you think?if you are worried about warning messages are easy to ignore, we can also transform it(CPOY=>INSTANT) in replication app. But a warning message is definitely simpler if no other affects

@winkyao

This comment has been minimized.

Copy link
Member Author

commented Jan 4, 2019

@morgo Well, return warning is also acceptable for me. I'll change this PR later.

@winkyao

This comment has been minimized.

Copy link
Member Author

commented Jan 4, 2019

@morgo Done.

Show resolved Hide resolved ddl/ddl_api.go Outdated

winkyao added some commits Jan 4, 2019

@crazycs520
Copy link
Contributor

left a comment

LGTM

@tiancaiamao

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

LGTM

@@ -1407,10 +1407,16 @@ func getCharsetAndCollateInTableOption(startIdx int, options []*ast.TableOption)
return
}

func (d *ddl) AlterTable(ctx sessionctx.Context, ident ast.Ident, specs []*ast.AlterTableSpec) (err error) {
// Only handle valid specs.
// resolveAlterTableSpec resolve alter table algorithm and remove ignore table spec in specs.

This comment has been minimized.

Copy link
@zimulala

zimulala Feb 19, 2019

Member

s/resolve/resolves
s/remove/removes

func (d *ddl) AlterTable(ctx sessionctx.Context, ident ast.Ident, specs []*ast.AlterTableSpec) (err error) {
// Only handle valid specs.
// resolveAlterTableSpec resolve alter table algorithm and remove ignore table spec in specs.
// returns valied specs, and the occured error.

This comment has been minimized.

Copy link
@zimulala

zimulala Feb 19, 2019

Member

s/returns/Return

for _, spec := range specs {
if spec.Tp == ast.AlterTableAlgorithm {
// find the last AlterTableAlgorithm.

This comment has been minimized.

Copy link
@zimulala

zimulala Feb 19, 2019

Member

s/find/Find

@@ -1210,3 +1210,75 @@ func (s *testIntegrationSuite) TestAlterColumn(c *C) {
_, err = s.tk.Exec("alter table mc modify column a bigint auto_increment") // Adds auto_increment should throw error
c.Assert(err, NotNil)
}

func (s *testIntegrationSuite) assertWarningExec(c *C, sql string) {

This comment has been minimized.

Copy link
@zimulala

zimulala Feb 19, 2019

Member

I think adding the warning message as an argument can be beneficial for calls to other functions.

@winkyao

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2019

@zimulala Done, PTAL

@zimulala
Copy link
Member

left a comment

LGTM

@zimulala zimulala added status/LGT3 and removed status/LGT2 labels Feb 19, 2019

@winkyao

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2019

/run-all-tests

@winkyao

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2019

/run-common-test

@winkyao

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2019

/run-common-test tidb-test=pr/741

1 similar comment
@winkyao

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2019

/run-common-test tidb-test=pr/741

@winkyao

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2019

/run-integration-common-test tidb-test=pr/741

@codecov-io

This comment has been minimized.

Copy link

commented Feb 20, 2019

Codecov Report

Merging #8811 into master will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8811      +/-   ##
==========================================
+ Coverage   67.07%   67.15%   +0.08%     
==========================================
  Files         372      373       +1     
  Lines       78085    78113      +28     
==========================================
+ Hits        52373    52459      +86     
+ Misses      21043    20966      -77     
- Partials     4669     4688      +19
Impacted Files Coverage Δ
ddl/ddl_algorithm.go 100% <100%> (ø)
ddl/ddl_api.go 77.24% <100%> (+2.41%) ⬆️
ddl/ddl.go 89.58% <100%> (+0.04%) ⬆️
util/systimemon/systime_mon.go 80% <0%> (-20%) ⬇️
ddl/delete_range.go 75.13% <0%> (-4.24%) ⬇️
store/tikv/lock_resolver.go 41.7% <0%> (-0.95%) ⬇️
executor/distsql.go 72.24% <0%> (+0.45%) ⬆️
executor/join.go 79.42% <0%> (+0.52%) ⬆️
expression/schema.go 94.53% <0%> (+0.78%) ⬆️
... and 4 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 f7dc915...b6e2b28. Read the comment docs.

@winkyao winkyao merged commit dec3ab6 into pingcap:master Feb 20, 2019

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@winkyao winkyao deleted the winkyao:ddl_algorithm_support branch Feb 20, 2019

yu34po added a commit to yu34po/tidb that referenced this pull request Feb 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.