Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

*: fix resource leak in select for update when 'tidb_low_resolution_tso' is set #57012

Merged
merged 2 commits into from
Nov 6, 2024
Merged

*: fix resource leak in select for update when 'tidb_low_resolution_tso' is set #57012

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7590423
*: fix resource leak in select for update when 'tidb_low_resolution_t…
tiancaiamao Oct 30, 2024
b27c914
address comment
tiancaiamao Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pkg/executor/adapter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the exec.Close(e) also be called at L618 before returning errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it should be.
Updated.

Copy link
Contributor

@cfzjywxk cfzjywxk Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tiancaiamao @lcwangchao
So we merge this fix first and file an refactor or enhancment issue for the more elegant solution?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree!

return nil, errors.New("can not execute select for update statement when 'tidb_low_resolution_tso' is set")
}
// Special handle for "select for update statement" in pessimistic transaction.
Expand All @@ -614,6 +615,7 @@ func (a *ExecStmt) Exec(ctx context.Context) (_ sqlexec.RecordSet, err error) {
var txnStartTS uint64
txn, err := sctx.Txn(false)
if err != nil {
terror.Log(exec.Close(e))
return nil, err
}
if txn.Valid() {
Expand Down