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

fix bug: should throw err when prepare multi stmt (#14112) #14124

Merged
merged 2 commits into from
Jan 11, 2024

Conversation

ouyuanning
Copy link
Contributor

参看这个comment:
#14068 (comment)

Approved by: @qingxinhome

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #14068

What this PR does / why we need it:

参看这个comment:
#14068 (comment)

@matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Jan 11, 2024
@mergify mergify bot requested a review from sukki37 January 11, 2024 06:04
@mergify mergify bot added the kind/bug Something isn't working label Jan 11, 2024
@matrix-meow
Copy link
Contributor

@ouyuanning Thanks for your contributions!

Title: fix bug: should throw err when prepare multi stmt (#14112)

Body: This pull request fixes a bug related to preparing multiple statements. It is based on the comment provided in issue #14068.

Changes:

  • In mysql_cmd_executor.go, the doComQuery function has been modified to check if the command is COM_STMT_PREPARE and there are multiple statements. If so, it throws an error using moerr.NewNotSupported.
  • In the handleSetOption function, the code related to CLIENT_MULTI_STATEMENTS has been commented out.

Review:

  1. In the doComQuery function, the check for COM_STMT_PREPARE and multiple statements is necessary to ensure that only a single statement is prepared. However, the error message provided in moerr.NewNotSupported is not clear. It would be helpful to provide a more descriptive error message explaining why preparing multiple statements is not supported.

  2. In the handleSetOption function, the code related to CLIENT_MULTI_STATEMENTS has been commented out. It is unclear why this change was made and if it is necessary. If it is not necessary, the commented code should be removed to avoid confusion.

Suggestions:

  1. In the doComQuery function, update the error message in moerr.NewNotSupported to provide a clear explanation of why preparing multiple statements is not supported. For example, "Preparing multiple statements is not supported. Please prepare one statement at a time."

  2. In the handleSetOption function, if the commented code is not necessary, it should be removed to avoid confusion. If it is necessary, add a comment explaining why it is commented out and provide a clear explanation of the implications of this change.

Optimizations:

No optimizations are needed for this pull request.

@mergify mergify bot merged commit 9d120d4 into matrixorigin:1.1-dev Jan 11, 2024
17 of 18 checks passed
@ouyuanning ouyuanning deleted the cp-14112-to-1.1 branch February 22, 2024 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working size/S Denotes a PR that changes [10,99] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants