Skip to content

executor: fix drop role not work as expected in the show result. #29667

Merged
ti-chi-bot merged 3 commits intopingcap:masterfrom
7yyo:drop_role
Nov 15, 2021
Merged

executor: fix drop role not work as expected in the show result. #29667
ti-chi-bot merged 3 commits intopingcap:masterfrom
7yyo:drop_role

Conversation

@7yyo
Copy link
Contributor

@7yyo 7yyo commented Nov 10, 2021

What problem does this PR solve?

Issue Number: close #29655

Problem Summary:

fix drop role not work as expected in the show result.

What is changed and how it works?

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

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

Release note

fix drop role not work as expected in the show result.

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Nov 10, 2021 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • mjonss
  • 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.

Details

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/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 10, 2021
@7yyo 7yyo changed the title executer: fix drop role not work as expected in the show result. executor: fix drop role not work as expected in the show result. Nov 10, 2021
@7yyo
Copy link
Contributor Author

7yyo commented Nov 10, 2021

/run-all-tests

@7yyo
Copy link
Contributor Author

7yyo commented Nov 10, 2021

PTAL @winoros @morgo @jackysp

tangenta
tangenta reviewed Nov 11, 2021
View reviewed changes
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 11, 2021
mjonss
mjonss approved these changes Nov 11, 2021
View reviewed changes
Copy link
Contributor

@mjonss mjonss left a comment

Choose a reason for hiding this comment

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

LGTM but could you also check if there are more bugs like this?

executor/simple.go
// begin a transaction to delete a user.
sql.Reset()
sqlexec.MustFormatSQL(sql, `DELETE FROM %n.%n WHERE Host = %? and User = %?;`, mysql.SystemDB, mysql.UserTable, user.Hostname, user.Username)
sqlexec.MustFormatSQL(sql, `DELETE FROM %n.%n WHERE Host = %? and User = %?;`, mysql.SystemDB, mysql.UserTable, strings.ToLower(user.Hostname), user.Username)
Copy link
Contributor

@mjonss mjonss Nov 11, 2021

Choose a reason for hiding this comment

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

I think there might be another case in line 680:

sqlexec.MustFormatSQL(sql, `DELETE IGNORE FROM %n.%n WHERE FROM_HOST=%? and FROM_USER=%? and TO_HOST=%? and TO_USER=%?`, mysql.SystemDB, mysql.RoleEdgeTable, role.Hostname, role.Username, user.Hostname, user.Username)

Copy link
Contributor Author

@7yyo 7yyo Nov 11, 2021

Choose a reason for hiding this comment

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

I'm not sure if it needs to be modified. Because this is not the mysql.user table, currently, create user will lowercase hostname, so I think all SQL involving mysql.user hostname should be changed to lowercase. I'm not sure about the other tables.

I added some other modifications, please help double confirm whether it is correct, thank you.

@mjonss

@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 Nov 11, 2021
@ti-chi-bot ti-chi-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 11, 2021
@morgo morgo mentioned this pull request Nov 14, 2021
Merged
12 tasks
@morgo
Copy link
Contributor

morgo commented Nov 14, 2021

/run-integration-tests

@morgo
Copy link
Contributor

morgo commented Nov 14, 2021

/merge

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Nov 14, 2021

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

DetailsCommit hash: 2df1e20

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 14, 2021
@ti-chi-bot ti-chi-bot merged commit 8f7ed14 into pingcap:master Nov 15, 2021
@7yyo 7yyo deleted the drop_role branch November 15, 2021 02:25
@winoros
Copy link
Member

winoros commented Nov 29, 2021

/run-cherry-picker release-5.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mjonss mjonss mjonss approved these changes

@tangenta tangenta tangenta approved these changes

Assignees

No one assigned

Labels

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. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

DROP ROLE not work as expected in the SHOW result.

6 participants

@7yyo @ti-chi-bot @morgo @winoros @mjonss @tangenta