executor: query user and role, hostname should be lowercase #28880

7yyo · 2021-10-15T12:50:51Z

What problem does this PR solve?

Issue Number: close #28841

Problem Summary:

When creating a new user, the hostname will be converted to lowercase. So when querying, also use lowercase.

What is changed and how it works?

What's Changed:

Use lowercase hostname for query.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

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

Documentation

Release note

Use lowercase `hostname` for query.

ti-chi-bot · 2021-10-15T12:50:52Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

djshow832
morgo

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.

7yyo · 2021-10-15T13:07:35Z

PTAL @morgo

morgo · 2021-10-15T18:57:17Z

executor/simple_test.go

+	tk.MustExec("CREATE ROLE `engineering`@`US`;")
+	tk.MustExec("create role `engineering`@`INDIA`;")


Can you also verify the persisted value in mysql.user?

It should match the following:

mysql [localhost:8024] {root} ((none)) > select user,host from mysql.user where user='engineering'\G *************************** 1. row *************************** user: engineering host: % *************************** 2. row *************************** user: engineering host: india *************************** 3. row *************************** user: engineering host: us 3 rows in set (0.00 sec)

Otherwise LGTM

The case you mentioned should be ok, but after your reminder, I found such a case，I think we should fix it：

In MySQL

mysql> select user,host from mysql.user; +------------------+-----------+ | user | host | +------------------+-----------+ | sqwe | % | | test_user | % | | tidb_security | % | | tidb_system | % | | u1 | % | | mysql.infoschema | localhost | | mysql.session | localhost | | mysql.sys | localhost | | root | localhost | +------------------+-----------+ 9 rows in set (0.00 sec) mysql> select host from mysql.user where host = 'LOCALHOST'\G *************************** 1. row *************************** host: localhost *************************** 2. row *************************** host: localhost *************************** 3. row *************************** host: localhost *************************** 4. row *************************** host: localhost 4 rows in set (0.00 sec)

In TiDB

mysql> select host from mysql.user; +-------+ | host | +-------+ | % | | % | | % | | % | | % | | % | | india | | us | +-------+ 8 rows in set (0.00 sec) mysql> select host from mysql.user where host = 'INDIA'\G Empty set (0.00 sec) mysql> select host from mysql.user where host = 'india'\G *************************** 1. row *************************** host: india 1 row in set (0.00 sec)

morgo · 2021-10-16T02:38:47Z

This difference is because MySQL uses a case insensitive collation, while tidb uses binary.

…

On Fri., Oct. 15, 2021, 8:30 p.m. #7, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In executor/simple_test.go <#28880 (comment)>: > + tk.MustExec("CREATE ROLE `engineering`@`US`;") + tk.MustExec("create role `engineering`@`INDIA`;") The case you mentioned should be ok, but after your reminder, I found such a case，I think we should fix it： In MySQL mysql> select user,host from mysql.user; +------------------+-----------+ | user | host | +------------------+-----------+ | sqwe | % | | test_user | % | | tidb_security | % | | tidb_system | % | | u1 | % | | mysql.infoschema | localhost | | mysql.session | localhost | | mysql.sys | localhost | | root | localhost | +------------------+-----------+ 9 rows in set (0.00 sec) mysql> select host from mysql.user where host = 'LOCALHOST'\G *************************** 1. row *************************** host: localhost *************************** 2. row *************************** host: localhost *************************** 3. row *************************** host: localhost *************************** 4. row *************************** host: localhost 4 rows in set (0.00 sec) In TiDB mysql> select host from mysql.user; +-------+ | host | +-------+ | % | | % | | % | | % | | % | | % | | india | | us | +-------+ 8 rows in set (0.00 sec) mysql> select host from mysql.user where host = 'INDIA'\G Empty set (0.00 sec) mysql> select host from mysql.user where host = 'india'\G *************************** 1. row *************************** host: india 1 row in set (0.00 sec) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28880 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAOE7T5P5XRNMLFT3VBAO3UHDPTTANCNFSM5GB464KA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

7yyo · 2021-10-16T02:49:01Z

But if use user as the query condition, it will be case-sensitive

mysql> select User, host from mysql.user where host = 'LOCALHOST';
+------------------+-----------+
| User             | host      |
+------------------+-----------+
| mysql.infoschema | localhost |
| mysql.session    | localhost |
| mysql.sys        | localhost |
| root             | localhost |
+------------------+-----------+
4 rows in set (0.00 sec)

mysql> select User, host from mysql.user where user = 'ROOT';
Empty set (0.00 sec)

mysql> select User, host from mysql.user where user = 'root';
+------+-----------+
| User | host      |
+------+-----------+
| root | localhost |
+------+-----------+
1 row in set (0.00 sec)

mysql> select version();
+-----------+
| version() |
+-----------+
| 8.0.20    |
+-----------+
1 row in set (0.00 sec)

morgo · 2021-10-16T03:17:45Z

Here is the definition from MySQL 8.0:

CREATE TABLE `user` (
  `Host` char(255) CHARACTER SET ascii COLLATE ascii_general_ci NOT NULL DEFAULT '', <-- insensitive
  `User` char(32) COLLATE utf8_bin NOT NULL DEFAULT '', <-- sensitive

Versus TiDB:

  `Host` char(255) NOT NULL, <-- binary (tidb disables collations by default)
  `User` char(32) NOT NULL, <-- binary (tidb disables collations by default)

7yyo · 2021-10-16T03:19:51Z

Here is the definition from MySQL 8.0:

CREATE TABLE `user` (
  `Host` char(255) CHARACTER SET ascii COLLATE ascii_general_ci NOT NULL DEFAULT '', <-- insensitive
  `User` char(32) COLLATE utf8_bin NOT NULL DEFAULT '', <-- sensitive

Versus TiDB:

  `Host` char(255) NOT NULL, <-- binary (tidb disables collations by default)
  `User` char(32) NOT NULL, <-- binary (tidb disables collations by default)

OK, I understand, thank you!

7yyo · 2021-10-16T08:16:02Z

@morgo PTAL , thank you.

By the way, I think if the user uses uppercase to store the hostname, but can only query through lowercase, it will make the user a little confused

djshow832 · 2021-10-18T11:25:08Z

executor/simple.go

@@ -982,7 +982,7 @@ func (e *SimpleExec) executeGrantRole(ctx context.Context, s *ast.GrantRoleStmt)
 	}

 	for _, role := range s.Roles {
-		exists, err := userExists(ctx, e.ctx, role.Username, role.Hostname)
+		exists, err := userExists(ctx, e.ctx, role.Username, strings.ToLower(role.Hostname))


I noticed that there is a similar function userExistsInternal. Should it be modified, too?
Besides, I prefer to put strings.ToLower in userExists so the callers don't have to care about the cases.

7yyo · 2021-10-20T03:50:44Z

PTAL @morgo @djshow832

morgo

LGTM, thanks for making the changes.

djshow832 · 2021-10-27T02:34:16Z

/merge

ti-chi-bot · 2021-10-27T02:34:18Z

@djshow832: /merge in this pull request requires 2 approval(s).

In response to this:

/merge

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.

djshow832 · 2021-10-27T03:11:37Z

/merge

ti-chi-bot · 2021-10-27T03:11:40Z

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

Commit hash: a4dae6d

ti-chi-bot · 2021-10-27T03:14:59Z

@7yyo: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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.

morgo · 2021-10-27T03:24:45Z

/run-check_dev_2

hostname to lower

d6d493f

ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 15, 2021

morgo requested review from morgo and djshow832 October 15, 2021 18:53

morgo reviewed Oct 15, 2021

View reviewed changes

add test case

362654c

djshow832 reviewed Oct 18, 2021

View reviewed changes

fix

e09435b

morgo approved these changes Oct 20, 2021

View reviewed changes

morgo requested a review from djshow832 October 27, 2021 01:36

djshow832 added the compatibility-breaker Violation of forwards/backwards compatibility in a design-time piece. label Oct 27, 2021

djshow832 approved these changes Oct 27, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 27, 2021

AilinKid changed the title ~~executer: query user and role, hostname should be lowercase~~ executor: query user and role, hostname should be lowercase Oct 27, 2021

Merge branch 'master' into lower

a4dae6d

morgo self-requested a review October 27, 2021 03:11

morgo approved these changes Oct 27, 2021

View reviewed changes

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 Oct 27, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Oct 27, 2021

Merge branch 'master' into lower

8ecbce7

ti-chi-bot merged commit ad75d08 into pingcap:master Oct 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: query user and role, hostname should be lowercase #28880

executor: query user and role, hostname should be lowercase #28880

7yyo commented Oct 15, 2021 •

edited by djshow832

Loading

ti-chi-bot commented Oct 15, 2021 •

edited

Loading

7yyo commented Oct 15, 2021

morgo Oct 15, 2021

7yyo Oct 16, 2021

morgo commented Oct 16, 2021 via email

7yyo commented Oct 16, 2021

morgo commented Oct 16, 2021

7yyo commented Oct 16, 2021

7yyo commented Oct 16, 2021

djshow832 Oct 18, 2021

7yyo commented Oct 20, 2021

morgo left a comment

djshow832 commented Oct 27, 2021

ti-chi-bot commented Oct 27, 2021

djshow832 commented Oct 27, 2021

ti-chi-bot commented Oct 27, 2021

ti-chi-bot commented Oct 27, 2021

morgo commented Oct 27, 2021

		tk.MustExec("CREATE ROLE `engineering`@`US`;")
		tk.MustExec("create role `engineering`@`INDIA`;")

executor: query user and role, hostname should be lowercase #28880

executor: query user and role, hostname should be lowercase #28880

Conversation

7yyo commented Oct 15, 2021 • edited by djshow832 Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Oct 15, 2021 • edited Loading

7yyo commented Oct 15, 2021

morgo Oct 15, 2021

Choose a reason for hiding this comment

7yyo Oct 16, 2021

Choose a reason for hiding this comment

morgo commented Oct 16, 2021 via email

7yyo commented Oct 16, 2021

morgo commented Oct 16, 2021

7yyo commented Oct 16, 2021

7yyo commented Oct 16, 2021

djshow832 Oct 18, 2021

Choose a reason for hiding this comment

7yyo commented Oct 20, 2021

morgo left a comment

Choose a reason for hiding this comment

djshow832 commented Oct 27, 2021

ti-chi-bot commented Oct 27, 2021

djshow832 commented Oct 27, 2021

ti-chi-bot commented Oct 27, 2021

ti-chi-bot commented Oct 27, 2021

morgo commented Oct 27, 2021

7yyo commented Oct 15, 2021 •

edited by djshow832

Loading

ti-chi-bot commented Oct 15, 2021 •

edited

Loading