context, executor: add a function to detach the TableReaderExecutor
#54456
Conversation
ti-chi-bot
bot
added
release-note-none
YangKeao
changed the title
TableReaderExecutor
ti-chi-bot
bot
added
the
size/XL
ti-chi-bot
bot
added
size/XXL
YangKeao
force-pushed
the
detach-executor
branch
5 times, most recently
from
0e754c7
to
28e5899
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #54456 +/- ##
=================================================
- Coverage 72.7971% 56.3085% -16.4887%
=================================================
Files 1553 1671 +118
Lines 437399 605844 +168445
=================================================
+ Hits 318414 341142 +22728
- Misses 99376 241346 +141970
- Partials 19609 23356 +3747
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| newCtx := treCtx | ||
|
|
||
| if ctx, ok := treCtx.ectx.(*contextsession.SessionExprContext); ok { | ||
| staticExprCtx := ctx.IntoStatic() |
There was a problem hiding this comment.
Is it possible to make IntoStatic a static function and receives exprctx.BuildContext as input? So that we will need to try to cast the expr ctx to SessionExprContext
There was a problem hiding this comment.
It's a little weird. I think it'd be better to announce that it can only handle the SessionExprContext, as we may have many other implementations for exprctx.BuildContext.
For example, the NullRejectCheckExprContext { *SessionExprContext } also implements the exprctx.BuildContext. If we are going to implement a standalone static IntoStatic function, it'll have to handle this case. It's not easy to maintain all possible implementations in that function (though the only possible branch is the *contextsession.SessionExprContext).
|
/retest |
|
/retest |
YangKeao
force-pushed
the
detach-executor
branch
from
54209e3
to
efb3dd9
Compare
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
@XuHuaiyu PTAL, thanks. |
YangKeao
force-pushed
the
detach-executor
branch
2 times, most recently
from
641b42e
to
5ff6ee8
Compare
|
/retest |
|
/retest |
1 similar comment
|
/retest |
pkg/distsql/context/context.go
Outdated
| // A simple way to fix it is to use the origianl SQLKiller, and wait for all cursor to be closed after | ||
| // receiving a kill signal and before resetting it. For now, it uses a newly created `SQLKiller` to avoid | ||
| // affecting the original one and keep safety. | ||
| newCtx.SQLKiller = &sqlkiller.SQLKiller{} |
There was a problem hiding this comment.
Do we need to set the vars.SQLKiller instead of making a new one？
pkg/executor/detach.go
Outdated
| } | ||
| newChildren[i] = detached | ||
| } | ||
| copy(newExecutor.AllChildren(), newChildren) |
There was a problem hiding this comment.
It's a little wired, can we use newExecutor.SetAllChildren(newChildren)
YangKeao
force-pushed
the
detach-executor
branch
2 times, most recently
from
8b7c6de
to
c65147c
Compare
Signed-off-by: Yang Keao <[email protected]>
YangKeao
force-pushed
the
detach-executor
branch
from
c65147c
to
b3047b6
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lcwangchao, qw4990, tangenta, XuHuaiyu 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 |
|
/retest |
What problem does this PR solve?
Issue Number: close #53336
Problem Summary:
It's not safe to call
Next(or other method) of an executor after the session is executing the next statement. We have split out a lot ofContextand it's clear that the context depended by theTableReaderExecutoris safe to be re-used (after proper configuration) while executing another statement is running on the session.What changed and how does it work?
This PR adds a
Detachfunction to the executor package. After callingDetach, the session can be used to run other statements. It depends on the following assumption:StmtCtxwill not be re-used. (For cursor-fetch scenario, it guaranteed by the logic insideResetContextOfStmt).Check List
Tests
Release note