server: add protocol support for lazy cursor fetch #54527
Conversation
ti-chi-bot
bot
added
release-note
size/XXL
YangKeao
force-pushed
the
cursor-fetch-protocol
branch
from
2358bff
to
c524f18
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #54527 +/- ##
=================================================
- Coverage 72.7618% 56.2016% -16.5603%
=================================================
Files 1546 1669 +123
Lines 436182 616936 +180754
=================================================
+ Hits 317374 346728 +29354
- Misses 99197 246357 +147160
- Partials 19611 23851 +4240
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
/retest |
| @@ -226,3 +226,66 @@ outerLoop: | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| func TestSerialLazyExecuteAndFetch(t *testing.T) { | |||
There was a problem hiding this comment.
Now we only have a serial test here. This PR won't test concurrent cursor fetch with normal execution (or another cursor fetch). After both #54456 and this one are merged, I'll add more tests about the concurrent lazy cursor fetch.
YangKeao
force-pushed
the
cursor-fetch-protocol
branch
3 times, most recently
from
3729c64
to
b423468
Compare
YangKeao
force-pushed
the
cursor-fetch-protocol
branch
2 times, most recently
from
5ce08db
to
5529e53
Compare
| chk := crs.NewChunk(nil) | ||
| func (cc *clientConn) executeWithCursor(ctx context.Context, stmt PreparedStatement, rs resultset.ResultSet) (lazy bool, err error) { | ||
| vars := (&cc.ctx).GetSessionVars() | ||
| if vars.EnableLazyCursorFetch { |
There was a problem hiding this comment.
Most of the following logic in this function is copied from the previous deleted lines. The only difference is this branch.
[LGTM Timeline notifier]Timeline:
|
|
/approve |
|
/approve |
pkg/server/conn_stmt.go
Outdated
| // first return value is `true` and `err` is not nil, the `rs` cannot be used anymore and should return the error to the upper layer. | ||
| func (cc *clientConn) executeWithLazyCursor(ctx context.Context, stmt PreparedStatement, rs resultset.ResultSet) (ok bool, err error) { | ||
| drs, ok, err := rs.TryDetach() | ||
| if !ok { |
There was a problem hiding this comment.
Should we use "!ok || err!=nil", which seems safer?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: easonn7, lcwangchao, qw4990, xhebox The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold A tiny fix! |
ti-chi-bot
bot
added
the
do-not-merge/hold
Signed-off-by: Yang Keao <yangkeao@chunibyo.icu>
YangKeao
force-pushed
the
cursor-fetch-protocol
branch
from
5529e53
to
5379dbf
Compare
|
/unhold |
ti-chi-bot
bot
removed
the
do-not-merge/hold
|
@YangKeao: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
What problem does this PR solve?
Issue Number: close #54526
Problem Summary:
Previously, all cursor fetch requests will load the data to the TiDB. Now, we can support lazily fetch results in a limited scope.
What changed and how does it work?
tidb_enable_lazy_cursor_fetch.ResultSetto give similar interface like the one for eager cursor fetch.Check List
Tests
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.