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.

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

executor: fix logging format of prepared statements (#16062) #22984

Merged
merged 35 commits into from
Apr 14, 2021

Conversation

nayuta-yanagisawa
Copy link
Contributor

What problem does this PR solve?

Issue Number: close #16062

Problem Summary: With a slow prepared statement, the slow query log records it in the form of exec ... rather than the underlying SQL with placeholders.

What is changed and how it works?

What's Changed: Records original queries of prepared statements.

How it Works:

On master branch (53a68b5)

mysql> PREPARE mystmt FROM 'SELECT SLEEP(?)';
Query OK, 0 rows affected (0.00 sec)

mysql> SET @num = 1;
Query OK, 0 rows affected (0.00 sec)

mysql> EXECUTE mystmt USING @num;
+----------+
| SLEEP(?) |
+----------+
|        0 |
+----------+
1 row in set (1.00 sec)

mysql> SELECT Query FROM information_schema.SLOW_QUERY LIMIT 1;
+-------------------------------------------+
| Query                                     |
+-------------------------------------------+
| EXECUTE mystmt USING @num [arguments: 1]; |
+-------------------------------------------+
1 row in set (0.00 sec)

On PR branch

mysql> PREPARE mystmt FROM 'SELECT SLEEP(?)';
Query OK, 0 rows affected (0.00 sec)

mysql> SET @num = 1;
Query OK, 0 rows affected (0.00 sec)

mysql> EXECUTE mystmt USING @num;
+----------+
| SLEEP(?) |
+----------+
|        0 |
+----------+
1 row in set (1.00 sec)

mysql> SELECT Query FROM information_schema.SLOW_QUERY LIMIT 1;
+---------------------------------+
| Query                           |
+---------------------------------+
| SELECT SLEEP(?) [arguments: 1]; |
+---------------------------------+
1 row in set (0.00 sec)

Related changes

N/A

Check List

Tests

  • Integration test

Side effects

  • Breaking backward compatibility

Release note

  • Slow query log records original queries with placeholders for prepared statements.

@nayuta-yanagisawa nayuta-yanagisawa requested a review from a team as a code owner February 27, 2021 16:10
@nayuta-yanagisawa nayuta-yanagisawa requested review from qw4990 and removed request for a team February 27, 2021 16:10
@ti-srebot ti-srebot added the first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. label Feb 27, 2021
@ti-challenge-bot
Copy link

There is no reward for this challenge pull request, so you can request a reward from @qw4990.

Details

Tip : About reward you can refs to reward-command.

Warning: None

1 similar comment
@ti-challenge-bot
Copy link

There is no reward for this challenge pull request, so you can request a reward from @qw4990.

Details

Tip : About reward you can refs to reward-command.

Warning: None

@ti-chi-bot ti-chi-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 27, 2021
@ichn-hu ichn-hu mentioned this pull request Feb 27, 2021
@nayuta-yanagisawa
Copy link
Contributor Author

/run-all-test

@github-actions github-actions bot added the sig/execution SIG execution label Feb 27, 2021
@nayuta-yanagisawa
Copy link
Contributor Author

All tests now pass. Could you take a look at the present pull request? @qw4990

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 9, 2021
} else {
sql = FormatSQL(a.Text, sessVars.PreparedParams)
}
sql := FormatSQL(a.GetTextToLog())
Copy link
Contributor

Choose a reason for hiding this comment

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

I am afraid of that the new code broke the original semantic, which is introduced in #18578 by @crazycs520 , PTAL at this PR.

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 24, 2021
@qw4990
Copy link
Contributor

qw4990 commented Apr 9, 2021

Please resolve conflicts at your convenience @nayuta-yanagisawa

@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 9, 2021
@nayuta-yanagisawa
Copy link
Contributor Author

The CI fails continuously. I think that this is possibly fixed by #23967.

@Rustin170506
Copy link
Member

/run-unit-test

2 similar comments
@Rustin170506
Copy link
Member

/run-unit-test

@Rustin170506
Copy link
Member

/run-unit-test

@Rustin170506
Copy link
Member

/run-check_dev_2

@Rustin170506
Copy link
Member

/run-unit-test

@Rustin170506
Copy link
Member

/run-integration-copr-test

/run-sqllogic-test-2

@ti-chi-bot ti-chi-bot merged commit 7d1a6a3 into pingcap:master Apr 14, 2021
@nayuta-yanagisawa nayuta-yanagisawa deleted the issue-16062_pr4 branch April 14, 2021 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. sig/execution SIG execution sig/sql-infra SIG: SQL Infra size/M Denotes a PR that changes 30-99 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.

The slow log of Prepare statements don't record the real SQL
7 participants