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.

Sign up for GitHub

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

Jump to bottom

*: use CLUSTERED and NONCLUSTERED to control primary key type #22409

Merged
merged 13 commits into from
Feb 5, 2021
Merged

*: use CLUSTERED and NONCLUSTERED to control primary key type #22409

merged 13 commits into from
Feb 5, 2021

Conversation

tangenta
Copy link
Contributor

@tangenta tangenta commented Jan 15, 2021 •
edited
Loading

What problem does this PR solve?

Issue Number: close #22346

Problem Summary:

As mentioned in #22346, it is not intuitive to set the session variable before creating a table. What’s more, it is also inconvenient to change the variable back and forth in the case of mix usage.

What is changed and how it works?

What's Changed:

  • Update dependency: bump the parser to the latest version(which supports CLUSTERED and NONCLUSTERED keywords).
  • Apply new keywords: during creating a table, TiDB decides whether the primary key of a table is clustered according to the syntax, global variable tidb_enable_clustered_index and the config option alter-primary-key.
  • Add special comment: the TiDB-specific executable comment /*!T[clustered_index] xxx */ is added in the output from binlog and SHOW CREATE TABLE.
  • Others:
    • Change the available values of tidb_pk_type in information_schema.tables to {CLUSTERED, NON_CLUSTERED}.

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:

Check List

Tests

  • Unit test
  • Integration test

Side effects

N/A

Release note

  • Support CLUSTERED and NONCLUSTERED keywords to specify the primary key type in CREATE TABLE statements.

@sre-bot
Copy link
Contributor

sre-bot commented Jan 15, 2021

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

@github-actions github-actions bot added sig/execution SIG execution sig/sql-infra SIG: SQL Infra labels Jan 15, 2021
@sre-bot
Copy link
Contributor

sre-bot commented Jan 15, 2021

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

2 similar comments
@sre-bot
Copy link
Contributor

sre-bot commented Jan 15, 2021

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

@sre-bot
Copy link
Contributor

sre-bot commented Jan 15, 2021

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

@AilinKid AilinKid added the type/enhancement The issue or PR belongs to an enhancement. label Jan 18, 2021
@tangenta tangenta marked this pull request as ready for review January 19, 2021 02:56
@tangenta tangenta requested review from a team as code owners January 19, 2021 02:56
@tangenta tangenta requested review from qw4990 and winoros and removed request for a team January 19, 2021 02:56
jackysp
jackysp reviewed Jan 21, 2021
View reviewed changes
ddl/ddl_api.go
tbInfo.IsCommonHandle = true
}
case model.PrimaryKeyTypeDefault:
if isSingleIntPK(constr, lastCol) {
tbInfo.PKIsHandle = !alterPKConf
Copy link
Member

@jackysp jackysp Jan 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Add && ctx.GetSessionVars().EnableClusteredIndex either to make it consistent with common handle?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A lot of tests will be broken. Let's do this in another PR when it is decided.

@jackysp
Copy link
Member

jackysp commented Jan 22, 2021

Please fix CI.

@coocood
Copy link
Member

coocood commented Jan 26, 2021

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 26, 2021
lzmhhh123
lzmhhh123 reviewed Feb 4, 2021
View reviewed changes
ddl/ddl_api.go
if isSingleIntPK(constr, lastCol) {
tbInfo.PKIsHandle = !alterPKConf
} else {
tbInfo.IsCommonHandle = !alterPKConf && ctx.GetSessionVars().EnableClusteredIndex
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we return a warning if model.PrimaryKeyTypeClustered && !ctx.GetSessionVars().EnableClusteredIndex?

Copy link
Member

Choose a reason for hiding this comment

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

No, the variable has lower priority.

lzmhhh123
lzmhhh123 reviewed Feb 4, 2021
View reviewed changes
Copy link
Contributor

@lzmhhh123 lzmhhh123 left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 4, 2021
ti-srebot
ti-srebot previously approved these changes Feb 4, 2021
View reviewed changes
@coocood
Copy link
Member

coocood commented Feb 5, 2021

LGTM

@coocood
Copy link
Member

coocood commented Feb 5, 2021

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 5, 2021
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@tangenta merge failed.

@tangenta
Copy link
Contributor Author

tangenta commented Feb 5, 2021

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

@tangenta
Copy link
Contributor Author

tangenta commented Feb 5, 2021 •
edited
Loading

dial tcp: lookup goproxy.cn on 10.233.0.10:53: no such host

/run-integration-ddl-test

@tangenta
Copy link
Contributor Author

tangenta commented Feb 5, 2021

/merge

@tangenta
Copy link
Contributor Author

tangenta commented Feb 5, 2021

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@tangenta merge failed.

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 27db39b into pingcap:master Feb 5, 2021
@tangenta tangenta mentioned this pull request Mar 11, 2021
Merged
@ti-srebot ti-srebot mentioned this pull request Mar 18, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lzmhhh123 lzmhhh123 lzmhhh123 left review comments

@jackysp jackysp jackysp left review comments

@coocood coocood coocood approved these changes

@ti-srebot ti-srebot ti-srebot left review comments

@qw4990 qw4990 Awaiting requested review from qw4990

@winoros winoros Awaiting requested review from winoros

Assignees
No one assigned
Labels
component/expression sig/execution SIG execution sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

add Clustered Index option in DDL statement.
7 participants
@tangenta @sre-bot @jackysp @coocood @ti-srebot @lzmhhh123 @AilinKid