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: error or skip unsupported partition-related DDLs #10672

Merged
merged 5 commits into from
Jun 11, 2019

Conversation

kennytm
Copy link
Contributor

@kennytm kennytm commented Jun 1, 2019

What problem does this PR solve?

Update parser to include pingcap/parser#345, which involved some changes in the DDL component.

What is changed and how it works?

  1. Make sure code which checks PartitionType recognizes all 5 partition types:

    • In ALTER TABLE ... ADD PARTITION, reject all types ("unsupported") except RANGE
  2. In a HASH partition, if the partitions are explicitly named (and maybe contains comments), they will be saved into the model. The comments are kept even after ALTER TABLE ... TRUNCATE PARTITION.

  3. ALTER TABLE ... PARTITION BY will raise an error (instead of silently ignoring).

  4. ALTER TABLE ... DROP PARTITION and ALTER TABLE ... TRUNCATE PARTITION will raise "multiple schema changes" error if multiple partition names are supplied.

Check List

Tests

  • Unit test

Code changes

  • Has persistent data change
    • HASH PARTITIONS are no longer just always named p0, p1, p2 etc and may have comments.

Side effects

Related changes

  • Need to update the documentation (?)

@codecov

This comment has been minimized.

@kennytm kennytm force-pushed the partitions branch 3 times, most recently from e52446c to fb1db96 Compare June 2, 2019 10:34
@kennytm
Copy link
Contributor Author

kennytm commented Jun 2, 2019

/run-integration-tests

@tiancaiamao
Copy link
Contributor

/run-all-tests

@@ -1747,8 +1747,7 @@ func resolveAlterTableSpec(ctx sessionctx.Context, specs []*ast.AlterTableSpec)
validSpecs = append(validSpecs, spec)
}

if len(validSpecs) != 1 {
// TODO: Hanlde len(validSpecs) == 0.
if len(validSpecs) > 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will len(validSpecs) == 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, ALTER TABLE t; is len(validSpecs) == 0.

ddl/ddl_api.go Show resolved Hide resolved
ddl/partition.go Show resolved Hide resolved
ddl/partition.go Show resolved Hide resolved
ddl/partition.go Show resolved Hide resolved
@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 11, 2019
@tiancaiamao
Copy link
Contributor

PTAL @crazycs520 @winkyao @zimulala

@kennytm kennytm marked this pull request as ready for review June 11, 2019 09:37
@kennytm
Copy link
Contributor Author

kennytm commented Jun 11, 2019

/run-all-tests

Copy link
Member

@bb7133 bb7133 left a comment

Choose a reason for hiding this comment

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

LGTM

@kennytm
Copy link
Contributor Author

kennytm commented Jun 11, 2019

/run-integration-common-test

@kennytm kennytm added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 11, 2019
@kennytm
Copy link
Contributor Author

kennytm commented Jun 11, 2019

/run-all-tests

Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

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

LGTM

@kennytm kennytm added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Jun 11, 2019
@kennytm kennytm merged commit fb7fd47 into pingcap:master Jun 11, 2019
@kennytm kennytm deleted the partitions branch June 11, 2019 15:40
@crazycs520
Copy link
Contributor

crazycs520 commented Jul 22, 2019

@kennytm Can you cherry pick this pr to v3.0.0, I found there was bug fix in this PR:
Or maybe you can only cherry-pick some part of this PR: the truncateTableByReassignPartitionIDs function.

create table t (a int,b int) partition by hash(a) partitions 10;
truncate table t;
select * from t partition (p0)
(1735, u"Unknown partition 'p0' in table 't'")
mysql>select tidb_version();
+--------------------------------------------------------------------------+
| tidb_version()                                                           |
+--------------------------------------------------------------------------+
| Release Version: v3.0.1-19-g8dd4a277d                                    |
| Git Commit Hash: 8dd4a277d2a844aaaf03b6d90792baade8adea70                |
| Git Branch: release-3.0                                                  |
| UTC Build Time: 2019-07-22 01:04:07                                      |
| GoVersion: go version go1.12 darwin/amd64                                |
| Race Enabled: false                                                      |
| TiKV Min Version: 2.1.0-alpha.1-ff3dd160846b7d1aed9079c389fc188f7f5ea13e |
| Check Table Before Drop: false                                           |
+--------------------------------------------------------------------------+

kennytm added a commit to kennytm/tidb that referenced this pull request Jul 22, 2019
* ddl: error or skip unsupported partition-related DDLs

* go.mod: stop replacing parser since the PR is merged

* ddl: addressed comment
@you06 you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/parser require-LGT3 Indicates that the PR requires three LGTM. sig/sql-infra SIG: SQL Infra status/LGT3 The PR has already had 3 LGTM. type/new-feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants