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

server: return error for new statement after opening cursor #40095

Conversation

YangKeao
Copy link
Member

Signed-off-by: YangKeao yangkeao@chunibyo.icu

What problem does this PR solve?

Issue Number: close #40094, close #39148, close #38116

Problem Summary:

Run statement after opening a cursor will make the sate (session variable, etc) hard to predict. The current implementation of cursor fetch in TiDB doesn't allow creating multiple cursor at the same time.

What is changed and how it works?

This PR add a check to return error when the command other than fetching/closing the active cursor arrives. The user will have to close the existing statement before running another commands.

The user forgot to close the statement may fail to run existing program ()

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

If the user forgot to close the statement explicitly before running other queries, the program will not continue to work with this patch. However, the former behavior is both unsafe (can cause panic in TiDB) and unpredictable (can read unpredictable data).

Release note

Previously, executing queries after opening an cursor will cause unpredictable behavior when reading the cursor. From now on, TiDB will not allow executing queries other than fetch and close after opening an cursor.

Signed-off-by: YangKeao <yangkeao@chunibyo.icu>
@YangKeao YangKeao requested a review from a team as a code owner December 21, 2022 12:10
@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has not been approved.

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.

@ti-chi-bot ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/needs-triage-completed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 21, 2022
@ti-chi-bot ti-chi-bot added needs-cherry-pick-release-5.3 Type: Need cherry pick to release-5.3 needs-cherry-pick-release-5.4 Should cherry pick this PR to release-5.4 branch. needs-cherry-pick-release-6.1 Should cherry pick this PR to release-6.1 branch. needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. and removed do-not-merge/needs-triage-completed labels Dec 21, 2022
@YangKeao YangKeao changed the title server: return error for new statement with cursor server: return error for new statement after opening cursor Dec 22, 2022
@YangKeao
Copy link
Member Author

/run-build
/run-unit-test

@YangKeao
Copy link
Member Author

As discussed, we should fix these problems rather than returning an error 🤔 , though it may take some effort. Let me try 🤖 .

@YangKeao YangKeao closed this Dec 22, 2022
@YangKeao YangKeao added the compatibility-breaker Violation of forwards/backwards compatibility in a design-time piece. label Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility-breaker Violation of forwards/backwards compatibility in a design-time piece. needs-cherry-pick-release-5.3 Type: Need cherry pick to release-5.3 needs-cherry-pick-release-5.4 Should cherry pick this PR to release-5.4 branch. needs-cherry-pick-release-6.1 Should cherry pick this PR to release-6.1 branch. needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
2 participants