server: add protocol support for lazy cursor fetch #54527

YangKeao · 2024-07-09T09:57:38Z

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?

Add a new system variable tidb_enable_lazy_cursor_fetch.
Add a new implementation of ResultSet to give similar interface like the one for eager cursor fetch.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

Add a new system variable `tidb_enable_lazy_cursor_fetch` to indicate whether to try to fetch data lazily for cursor fetch requests.

codecov · 2024-07-09T10:23:44Z

Codecov Report

Attention: Patch coverage is 80.21390% with 37 lines in your changes missing coverage. Please review.

Project coverage is 56.2016%. Comparing base (2362c9c) to head (5379dbf).
Report is 77 commits behind head on master.

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

Flag	Coverage Δ
integration	`22.1644% <33.3333%> (?)`
unit	`71.7200% <80.2139%> (-0.0510%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`52.9656% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`51.9930% <ø> (+6.1455%)`	⬆️

YangKeao · 2024-07-10T03:02:14Z

/retest

YangKeao · 2024-07-10T03:10:44Z

pkg/server/tests/cursor/cursor_test.go

@@ -226,3 +226,66 @@ outerLoop:
 		}
 	}
 }
+
+func TestSerialLazyExecuteAndFetch(t *testing.T) {


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.

pkg/server/conn_stmt.go

pkg/server/internal/resultset/cursor.go

pkg/server/internal/resultset/resultset.go

pkg/server/conn_stmt.go

pkg/server/internal/resultset/resultset.go

YangKeao · 2024-07-17T04:16:09Z

pkg/server/conn_stmt.go

-			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 {


Most of the following logic in this function is copied from the previous deleted lines. The only difference is this branch.

ti-chi-bot · 2024-07-17T06:57:58Z

[LGTM Timeline notifier]

Timeline:

2024-07-17 03:28:12.743679427 +0000 UTC m=+410914.734620897: ☑️ agreed by lcwangchao.
2024-07-17 06:57:57.346361282 +0000 UTC m=+423499.337302752: ☑️ agreed by xhebox.

easonn7 · 2024-07-18T08:59:37Z

/approve

easonn7 · 2024-07-18T09:07:24Z

/approve

qw4990 · 2024-07-18T09:24:00Z

pkg/server/conn_stmt.go

+// 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 {


Should we use "!ok || err!=nil", which seems safer?

You are right!

ti-chi-bot · 2024-07-18T09:47:00Z

[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:

~~OWNERS~~ [lcwangchao,qw4990,xhebox]
~~pkg/sessionctx/variable/OWNERS~~ [easonn7]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

YangKeao · 2024-07-18T09:47:31Z

/hold A tiny fix!

Signed-off-by: Yang Keao <yangkeao@chunibyo.icu>

YangKeao · 2024-07-18T09:48:54Z

/unhold

tiprow · 2024-07-18T09:50:16Z

@YangKeao: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
fast_test_tiprow	`5379dbf`	link	true	`/test fast_test_tiprow`

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.

ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 9, 2024

YangKeao force-pushed the cursor-fetch-protocol branch from 2358bff to c524f18 Compare July 9, 2024 10:18

YangKeao commented Jul 10, 2024

View reviewed changes

lcwangchao reviewed Jul 10, 2024

View reviewed changes

pkg/server/conn_stmt.go Outdated Show resolved Hide resolved

pkg/server/internal/resultset/cursor.go Show resolved Hide resolved

pkg/server/internal/resultset/resultset.go Outdated Show resolved Hide resolved

pkg/server/conn_stmt.go Outdated Show resolved Hide resolved

YangKeao force-pushed the cursor-fetch-protocol branch 3 times, most recently from 3729c64 to b423468 Compare July 11, 2024 08:59

lcwangchao reviewed Jul 12, 2024

View reviewed changes

pkg/server/internal/resultset/resultset.go Outdated Show resolved Hide resolved

YangKeao force-pushed the cursor-fetch-protocol branch 2 times, most recently from 5ce08db to 5529e53 Compare July 12, 2024 06:04

lcwangchao approved these changes Jul 17, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jul 17, 2024

YangKeao requested a review from xhebox July 17, 2024 04:07

YangKeao commented Jul 17, 2024

View reviewed changes

xhebox approved these changes Jul 17, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jul 17, 2024

ti-chi-bot bot added the approved label Jul 18, 2024

qw4990 reviewed Jul 18, 2024

View reviewed changes

qw4990 approved these changes Jul 18, 2024

View reviewed changes

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 18, 2024

add protocol support for lazy cursor fetch

5379dbf

Signed-off-by: Yang Keao <yangkeao@chunibyo.icu>

YangKeao force-pushed the cursor-fetch-protocol branch from 5529e53 to 5379dbf Compare July 18, 2024 09:48

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 18, 2024

ti-chi-bot bot merged commit afd6d6a into pingcap:master Jul 18, 2024
22 of 23 checks passed

YangKeao mentioned this pull request Jul 22, 2024

Add test for concurrently executing normal statements with background cursor #54801

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: add protocol support for lazy cursor fetch #54527

server: add protocol support for lazy cursor fetch #54527

YangKeao commented Jul 9, 2024

codecov bot commented Jul 9, 2024 •

edited

Loading

YangKeao commented Jul 10, 2024

YangKeao Jul 10, 2024

YangKeao Jul 17, 2024

ti-chi-bot bot commented Jul 17, 2024

easonn7 commented Jul 18, 2024

easonn7 commented Jul 18, 2024

qw4990 Jul 18, 2024

YangKeao Jul 18, 2024

ti-chi-bot bot commented Jul 18, 2024

YangKeao commented Jul 18, 2024

YangKeao commented Jul 18, 2024

tiprow bot commented Jul 18, 2024

server: add protocol support for lazy cursor fetch #54527

server: add protocol support for lazy cursor fetch #54527

Conversation

YangKeao commented Jul 9, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

codecov bot commented Jul 9, 2024 • edited Loading

Codecov Report

YangKeao commented Jul 10, 2024

YangKeao Jul 10, 2024

Choose a reason for hiding this comment

YangKeao Jul 17, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented Jul 17, 2024

[LGTM Timeline notifier]

easonn7 commented Jul 18, 2024

easonn7 commented Jul 18, 2024

qw4990 Jul 18, 2024

Choose a reason for hiding this comment

YangKeao Jul 18, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented Jul 18, 2024

YangKeao commented Jul 18, 2024

YangKeao commented Jul 18, 2024

tiprow bot commented Jul 18, 2024

codecov bot commented Jul 9, 2024 •

edited

Loading