executor, session: Add Detach() method to detach the record set #54091
Conversation
ti-chi-bot
bot
added
release-note-none
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #54091 +/- ##
=================================================
- Coverage 74.5222% 57.4713% -17.0510%
=================================================
Files 1508 1646 +138
Lines 358470 621228 +262758
=================================================
+ Hits 267140 357028 +89888
- Misses 71953 240476 +168523
- Partials 19377 23724 +4347
Flags with carried forward coverage won't be shown. Click here to find out more.
|
YangKeao
force-pushed
the
static-record-set
branch
2 times, most recently
from
4a820cb
to
6a6f561
Compare
|
/retest |
2 similar comments
|
/retest |
|
/retest |
YangKeao
force-pushed
the
static-record-set
branch
from
6a6f561
to
6583c38
Compare
|
/retest |
YangKeao
force-pushed
the
static-record-set
branch
5 times, most recently
from
8c62fcf
to
2688b85
Compare
| // FIXME: if the following code meets error, the `detachedRS` will be leaked. | ||
| // The error may occur during the begin/commit of the newly created transaction on the given `startTS``. | ||
|
|
||
| // FIXME: block the min-start-ts. Now the `min-start-ts` will advance and exceed the `startTS` used by |
There was a problem hiding this comment.
This FIXME will be fixed in #54114
YangKeao
force-pushed
the
static-record-set
branch
from
2688b85
to
25e4b92
Compare
| RecordSet | ||
|
|
||
| // Detach detaches the record set from the current session context. | ||
| Detach() (RecordSet, error) |
There was a problem hiding this comment.
Could we make this function to TryDetach() (RecordSet, bool) because when recordSet cannot be detached, it may be a normal case
There was a problem hiding this comment.
I'll return TryDetach() (RecordSet, bool, error) to indicate whether the statement is suitable to detach, and whether it detaches successfully. I sounds strange but I didn't come up a better way 🤦.
If the caller receives nil, false, _, it means the original record set can still be used. If the caller receives _, _, err, it means the state of session/recordSet is in a chaos, and usually the connection will be halted.
YangKeao
force-pushed
the
static-record-set
branch
3 times, most recently
from
94c73b6
to
70904b6
Compare
Signed-off-by: Yang Keao <[email protected]>
YangKeao
force-pushed
the
static-record-set
branch
from
70904b6
to
a5b23d4
Compare
|
/retest |
ti-chi-bot
bot
added
needs-1-more-lgtm
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lcwangchao, 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 |
[LGTM Timeline notifier]Timeline:
|
What problem does this PR solve?
Issue Number: close #54090
Problem Summary:
The original
recordSetandexecStmtResultare too complicated to implement the lazy cursor fetch feature. We do need a much simpler one: without explicit transaction, without metrics/summaries and only supports read-only statements.What changed and how does it work?
This PR adds a method
Detachto create a new record set from existingrecordSetorexecStmtResult. The new record set can run without depending on the session context.However, currently the executor still cannot be detached, so this PR specially handled the
TableReaderExecutorand left aTODOhere.Check List
Tests
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.