*: fix resource leak in select for update when 'tidb_low_resolution_tso' is set #57012
Conversation
ti-chi-bot
bot
added
do-not-merge/needs-triage-completed
release-note-none
ti-chi-bot
bot
added
the
size/XS
|
Hi @tiancaiamao. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #57012 +/- ##
=================================================
- Coverage 73.2316% 58.2548% -14.9768%
=================================================
Files 1650 1812 +162
Lines 455646 659689 +204043
=================================================
+ Hits 333677 384301 +50624
- Misses 101479 250300 +148821
- Partials 20490 25088 +4598
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| @@ -598,6 +598,7 @@ func (a *ExecStmt) Exec(ctx context.Context) (_ sqlexec.RecordSet, err error) { | |||
|
|
|||
| if a.isSelectForUpdate { | |||
| if sctx.GetSessionVars().UseLowResolutionTSO() { | |||
| terror.Log(exec.Close(e)) | |||
There was a problem hiding this comment.
Should the exec.Close(e) also be called at L618 before returning errors?
There was a problem hiding this comment.
Yes, it should be.
Updated.
There was a problem hiding this comment.
Or if the exec.Close(e) is idempotent, is it better to add the logic in the defer block of the function, if the executor is opened?
There was a problem hiding this comment.
Let's just do the ad-hoc fix for the bug first here.
I'm afraid of introducing bugs when doing too much changes, although it may reduce potential bug and reduce the maintance burden.
@lcwangchao is doing the refactoring work, I'd like hear his idea
There was a problem hiding this comment.
Seems there is no better way to make the close more elegant and keep a minimum change of the code. The following function handlePessimisticSelectForUpdate and handleNoDelay may close or replace the executor internally. If we want to make the code more clear, we should redesign such implementations, introducing more assumption or make executor.Close idempotent...
There was a problem hiding this comment.
@tiancaiamao @lcwangchao
So we merge this fix first and file an refactor or enhancment issue for the more elegant solution?
ti-chi-bot
bot
added
approved
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfzjywxk, lcwangchao 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 |
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
/test build |
|
@tiancaiamao: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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. |
ti-chi-bot
bot
added
the
needs-cherry-pick-release-8.5
|
In response to a cherrypick label: new pull request created to branch |
What problem does this PR solve?
Issue Number: close #55468
Problem Summary:
The cause of that DATA RACE is a resource leak.
After executing "select * from low_resolution_tso for update" when
tidb_low_resolution_tsois on,error is return but the executor is not closed, so the background goroutine leak.
The leaked goroutine still referencing session ctx.
So when the next SQL is executed, it races visiting the session ctx.
What changed and how does it work?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.