Skip to content

Conversation

e1ijah1
Copy link
Contributor

@e1ijah1 e1ijah1 commented May 27, 2022

Signed-off-by: elijah [email protected]

What problem does this PR solve?

Issue Number: close #34931

Problem Summary: the 'max-index-length' check does not respect non-restricted sql_mode

What is changed and how it works?

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
mysql> set @@sql_mode='';
Query OK, 0 rows affected (0.00 sec)

mysql> create table t (id int, name varchar(2048), index(name)) charset=utf8;
Query OK, 0 rows affected, 2 warnings (0.02 sec)

mysql> show warnings;
+---------+------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Level   | Code | Message                                                                                                                                                                     |
+---------+------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Warning | 3719 | 'utf8' is currently an alias for the character set UTF8MB3, but will be an alias for UTF8MB4 in a future release. Please consider using UTF8MB4 in order to be unambiguous. |
| Warning | 1071 | Specified key was too long; max key length is 3072 bytes                                                                                                                    |
+---------+------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
2 rows in set (0.00 sec)

mysql> show create table t;
+-------+-------------------------------------------------------------------------------------------------------------------------------------------------------+
| Table | Create Table                                                                                                                                          |
+-------+-------------------------------------------------------------------------------------------------------------------------------------------------------+
| t     | CREATE TABLE `t` (
  `id` int(11) DEFAULT NULL,
  `name` varchar(2048) DEFAULT NULL,
  KEY `name` (`name`(1024))
) ENGINE=InnoDB DEFAULT CHARSET=utf8 |
+-------+-------------------------------------------------------------------------------------------------------------------------------------------------------+
1 row in set (0.00 sec)
  • No code

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

converted the error to warning in non-strict SQL mode with `max-index-length` check

@ti-chi-bot
Copy link
Member

ti-chi-bot commented May 27, 2022

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • bb7133
  • tangenta

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 27, 2022
@sre-bot
Copy link
Contributor

sre-bot commented May 27, 2022

@e1ijah1
Copy link
Contributor Author

e1ijah1 commented May 27, 2022

/run-all-tests

@e1ijah1
Copy link
Contributor Author

e1ijah1 commented May 27, 2022

/run-realtikv-test

@e1ijah1 e1ijah1 changed the title ddl: fix the 'max-index-length' check result in non-restricted sql mod [WIP]ddl: fix the 'max-index-length' check result in non-restricted sql mod May 27, 2022
@ti-chi-bot ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 27, 2022
@bb7133
Copy link
Member

bb7133 commented May 28, 2022

@e11jah , just a kind reminder:

The `max-index-length' in MySQL(with InnoDB) is hard-coded to 3072, but it is configurable in TiDB because of some histroical reason:

https://docs.pingcap.com/tidb/dev/tidb-configuration-file#max-index-length

@e1ijah1 e1ijah1 changed the title [WIP]ddl: fix the 'max-index-length' check result in non-restricted sql mod ddl: fix the 'max-index-length' check result in non-restricted sql mod May 28, 2022
@ti-chi-bot ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 28, 2022
@e1ijah1
Copy link
Contributor Author

e1ijah1 commented May 29, 2022

/run-all-tests

@e1ijah1
Copy link
Contributor Author

e1ijah1 commented May 29, 2022

/cc @bb7133 @CbcWestwolf @hawkingrei @tangenta PTAL

@ti-chi-bot ti-chi-bot requested review from bb7133 and CbcWestwolf May 29, 2022 23:55
@ti-chi-bot
Copy link
Member

@e11jah: GitHub didn't allow me to request PR reviews from the following users: PTAL.

Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @bb7133 @CbcWestwolf PTAL

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@e1ijah1 e1ijah1 requested a review from CbcWestwolf May 30, 2022 09:42
@ti-chi-bot
Copy link
Member

@CbcWestwolf: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

Signed-off-by: elijah <[email protected]>
@e1ijah1 e1ijah1 requested a review from tangenta May 30, 2022 14:35
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label May 31, 2022
@ti-chi-bot ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 31, 2022
@e1ijah1 e1ijah1 changed the title ddl: fix the 'max-index-length' check result in non-restricted sql mod WIP: ddl: fix the 'max-index-length' check result in non-restricted sql mod May 31, 2022
@ti-chi-bot ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 31, 2022
@bb7133
Copy link
Member

bb7133 commented Jun 1, 2022

Is there any reason this PR is changed back to 'WIP'?

@e1ijah1
Copy link
Contributor Author

e1ijah1 commented Jun 1, 2022

Is there any reason this PR is changed back to 'WIP'?

Sorry, I found that the error occurs when building index with exceeds max length text type column, I will fix it later.

@e1ijah1 e1ijah1 changed the title WIP: ddl: fix the 'max-index-length' check result in non-restricted sql mod ddl: fix the 'max-index-length' check result in non-restricted sql mod Jun 1, 2022
@ti-chi-bot ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 1, 2022
@e1ijah1 e1ijah1 requested a review from bb7133 June 1, 2022 12:20
@bb7133
Copy link
Member

bb7133 commented Jun 1, 2022

Sorry, I found that the error occurs

@e11jah You don't need to say 'sorry' :)

@bb7133
Copy link
Member

bb7133 commented Jun 1, 2022

Thank you!

Signed-off-by: elijah <[email protected]>
@e1ijah1 e1ijah1 requested a review from bb7133 June 2, 2022 06:41
@bb7133
Copy link
Member

bb7133 commented Jun 3, 2022

The rest LGTM.

@e1ijah1 e1ijah1 requested a review from bb7133 June 4, 2022 13:00
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

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 6, 2022
@bb7133
Copy link
Member

bb7133 commented Jun 6, 2022

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: eb69948

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 6, 2022
@ti-chi-bot ti-chi-bot merged commit 45a6758 into pingcap:master Jun 6, 2022
@sre-bot
Copy link
Contributor

sre-bot commented Jun 6, 2022

TiDB MergeCI notify

🔴 Bad News! [2] CI still failing after this pr merged.
These failed integration tests don't seem to be introduced by the current PR.

CI Name Result Duration Compare with Parent commit
idc-jenkins-ci-tidb/integration-common-test 🔴 failed 1, success 10, total 11 17 min Existing failure
idc-jenkins-ci-tidb/common-test 🔴 failed 1, success 11, total 12 6 min 53 sec Existing failure
idc-jenkins-ci/integration-cdc-test 🟢 all 35 tests passed 27 min Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2 🟢 all 28 tests passed 7 min 15 sec Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1 🟢 all 26 tests passed 7 min 14 sec Existing passed
idc-jenkins-ci-tidb/integration-ddl-test 🟢 all 6 tests passed 7 min 3 sec Existing passed
idc-jenkins-ci-tidb/tics-test 🟢 all 1 tests passed 5 min 54 sec Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test 🟢 all 1 tests passed 4 min 34 sec Existing passed
idc-jenkins-ci-tidb/mybatis-test 🟢 all 1 tests passed 3 min 15 sec Existing passed
idc-jenkins-ci-tidb/plugin-test 🟢 build success, plugin test success 4min Existing passed

@e1ijah1
Copy link
Contributor Author

e1ijah1 commented Jun 6, 2022

/run-all-test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ddl: the 'max-index-length' check does not respect non-restricted sql_mode
6 participants