-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
session, com_stmt: fetch all rows during EXECUTE command #42473
Conversation
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
a71d076
to
a231797
Compare
/retest |
cc @jackysp , the other issue of cursor read, we need to tell users that it may cost more memory. |
b5d542b
to
a80758e
Compare
a80758e
to
de833e2
Compare
@@ -524,7 +524,7 @@ func TestDispatchClientProtocol41(t *testing.T) { | |||
com: mysql.ComStmtFetch, | |||
in: []byte{0x1, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, | |||
err: nil, | |||
out: []byte{0x5, 0x0, 0x0, 0x9, 0xfe, 0x0, 0x0, 0x82, 0x0}, | |||
out: []byte{0x5, 0x0, 0x0, 0x9, 0xfe, 0x0, 0x0, 0x42, 0x0}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original expected out is wrong. The fetchsize is 0 and the resultSet is not empty, so it shouldn't return 0x82 (which includes mysql.ServerStatusLastRowSend
).
I modifed it to the correct answer here: the status is still ServerStatusCursorExists
with no answer.
/retest |
… this statement is active Signed-off-by: YangKeao <yangkeao@chunibyo.icu>
Signed-off-by: YangKeao <yangkeao@chunibyo.icu>
Signed-off-by: YangKeao <yangkeao@chunibyo.icu>
2ee5315
to
c52dc3c
Compare
rebase the master. @hawkingrei seems to have made some optimization on test speed. |
Signed-off-by: YangKeao <yangkeao@chunibyo.icu>
/merge |
This pull request has been accepted and is ready to merge. Commit hash: e613832
|
/retest |
1 similar comment
/retest |
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
In response to a cherrypick label: new pull request created to branch |
close #38116 |
What problem does this PR solve?
Issue Number: close #42424, close #41891
What is changed and how it works?
fetchedRows
in theResultSet
.EXECUTE
command, so we don't need to close it again in the following commands (FETCH
,RESET
andCLOSE
)Check List
Tests
Test with the following a little bigger table:
Side effects
It stores all rows of a cursor fetch statement locally, while execuing the
EXECUTE
command, so it will eat up more memory (related with the size of query).Release note