Skip to content

executor: fix log desensitization bug in prestmt#19392

Merged
crazycs520 merged 8 commits intopingcap:masterfrom
crazycs520:fix-prestmt
Aug 24, 2020
Merged

executor: fix log desensitization bug in prestmt#19392
crazycs520 merged 8 commits intopingcap:masterfrom
crazycs520:fix-prestmt

Conversation

@crazycs520
Copy link
Contributor

@crazycs520 crazycs520 commented Aug 24, 2020 •
edited
Loading

Signed-off-by: crazycs520 crazycs520@gmail.com

What problem does this PR solve?

Close #18566 (comment)

When tidb log enables desensitization (set @@global.tidb_log_desensitization = 1;), the slow-log and log may still out put the previous statement that doesn't desensitization:

before

...
# Prev_stmt: insert into t values (1,1),(2,2),(3,3),(4,4),(5,5)
commit;

this PR

...
# Prev_stmt: insert into t values ( ... ) , ( ... ) , ( ... ) , ( ... ) , ( ... )
commit;

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Manual test
set @@tidb_slow_log_threshold=0;
begin;insert into t values (1,1),(2,2),(3,3),(4,4),(5,5); 
commit;
set @@tidb_slow_log_threshold=100;
-- Then check the tidb-slow-log file.

Side effects

  • Performance regression
    • Consumes more CPU

Release note

  • Fix log desensitization bug in prestmt

@crazycs520
executor: fix log desensitization bug in prestmt
8ed9838 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@crazycs520 crazycs520 requested a review from a team as a code owner August 24, 2020 07:15
@crazycs520 crazycs520 requested review from SunRunAway and removed request for a team August 24, 2020 07:15
@crazycs520 crazycs520 added security Everything related with security sig/execution SIG execution labels Aug 24, 2020
@crazycs520 crazycs520 mentioned this pull request Aug 24, 2020
Closed
@crazycs520
add unit test
bdf1cbe 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@crazycs520
Merge branch 'fix-prestmt' of https://github.com/crazycs520/tidb into…
2a6fd8c 
… fix-prestmt
@bb7133
Copy link
Member

bb7133 commented Aug 24, 2020

LGTM

@ti-srebot
Copy link
Contributor

ti-srebot commented Aug 24, 2020

@bb7133,Thanks for your review. However, LGTM is restricted to Reviewers or higher roles.See the corresponding SIG page for more information. Related SIGs: execution(slack).

tangenta
tangenta reviewed Aug 24, 2020
View reviewed changes
Copy link
Contributor

@tangenta tangenta 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 the status/LGT1 Indicates that a PR has LGTM 1. label Aug 24, 2020
jackysp
jackysp previously approved these changes Aug 24, 2020
View reviewed changes
Copy link
Contributor

@jackysp jackysp 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 removed the status/LGT1 Indicates that a PR has LGTM 1. label Aug 24, 2020
ti-srebot
ti-srebot previously approved these changes Aug 24, 2020
View reviewed changes
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Aug 24, 2020
@crazycs520
fix retry log
bc46872 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@crazycs520 crazycs520 dismissed stale reviews from ti-srebot and jackysp via bc46872 August 24, 2020 08:06
@bb7133
Copy link
Member

bb7133 commented Aug 24, 2020

LGTM

@ti-srebot
Copy link
Contributor

ti-srebot commented Aug 24, 2020

@bb7133,Thanks for your review. However, LGTM is restricted to Reviewers or higher roles.See the corresponding SIG page for more information. Related SIGs: execution(slack).

@jackysp
Copy link
Contributor

jackysp commented Aug 24, 2020

Please resolve conflicts. @crazycs520

@crazycs520
Copy link
Contributor Author

crazycs520 commented Aug 24, 2020

/run-all-tests

@ti-srebot
Copy link
Contributor

ti-srebot commented Aug 24, 2020

@bb7133,Thanks for your review. However, LGTM is restricted to Reviewers or higher roles.See the corresponding SIG page for more information. Related SIGs: execution(slack).

@crazycs520
Copy link
Contributor Author

crazycs520 commented Aug 24, 2020

/run-check_dev_2

@crazycs520 crazycs520 merged commit 9620b71 into pingcap:master Aug 24, 2020
@crazycs520 crazycs520 deleted the fix-prestmt branch August 24, 2020 12:28
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Sep 17, 2020
@crazycs520 @ti-srebot
cherry pick pingcap#19392 to release-4.0
5e3901f 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Sep 17, 2020
Merged
@ti-srebot
Copy link
Contributor

ti-srebot commented Sep 17, 2020

cherry pick to release-4.0 in PR #20048

ti-srebot added a commit that referenced this pull request Sep 17, 2020
@ti-srebot
executor: fix log desensitization bug in prestmt (#19392) (#20048)
4ae5f54 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
Signed-off-by: crazycs520 <crazycs520@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tangenta tangenta tangenta left review comments

@bb7133 bb7133 bb7133 approved these changes

@jackysp jackysp jackysp left review comments

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

@SunRunAway SunRunAway Awaiting requested review from SunRunAway

Assignees

No one assigned

Labels

security Everything related with security sig/execution SIG execution 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.

Support error/info log desensitization

5 participants

@crazycs520 @bb7133 @ti-srebot @jackysp @tangenta