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.

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

Jump to bottom

executor: error processing refactor for load data #12465

Merged
merged 13 commits into from Oct 11, 2019
Merged

executor: error processing refactor for load data #12465

merged 13 commits into from Oct 11, 2019

Conversation

cfzjywxk
Copy link
Contributor

What problem does this PR solve?

there are two different modifications(maybe they should not be considered together?)

  1. remove old logic in load data error processing, commitWork function will commit or rollback txn. Former implementation will make query processing panic since commit fail and txn is invalid.
    Ealier tidb version still has this problem(2.1 3.0), you will get lost connection error if doing sth like ddl between load data

  2. doCommit function defer will make txn changeToInvalid, so if commit failed and stmtRollBack is called, txnState has nil transaction field, which will cause ConfirmAssertions operation report panic error "invalid memory nil"

some unit-test or tidb-test will be added for this situation

enable commit fail point in `commitOneTask` function
do load data query

client expected like:
ERROR 1105 (HY000): Information schema is changed. [try again later]
not `lost connection` and no panic error log

What is changed and how it works?

Check List

Tests

  • Manual test (add detailed scripts or steps below)
enable commit fail point in `commitOneTask` function
do load data query

client expected like:
ERROR 1105 (HY000): Information schema is changed. [try again later]
not `lost connection` and no panic error log

Code changes

Side effects

Related changes

Release note

@cfzjywxk cfzjywxk added type/bug-fix This PR fixes a bug. sig/execution SIG execution sig/transaction SIG:Transaction labels Sep 28, 2019
@codecov
Copy link

codecov bot commented Sep 28, 2019 •
edited

Codecov Report

Merging #12465 into master will increase coverage by 0.0146%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             master    #12465        +/-   ##
===============================================
+ Coverage   79.9213%   79.936%   +0.0146%     
===============================================
  Files           460       460                
  Lines        103732    103803        +71     
===============================================
+ Hits          82904     82976        +72     
- Misses        14760     14761         +1     
+ Partials       6068      6066         -2

lysu
lysu reviewed Sep 29, 2019
View reviewed changes
server/conn.go
}
return cc.ctx.CommitTxn(sessionctx.SetCommitCtx(ctx, loadDataInfo.Ctx))
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove this snippet will remove function added by #9444?

maybe some reason we need txn.Rollback here if txn.Valid and have err, PTAL @crazycs520

Copy link
Contributor Author

Choose a reason for hiding this comment

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

former implementation will leave one batch/txn undecided out for loop, so the main logic need to process the last txn

Copy link
Member

Choose a reason for hiding this comment

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

PTAL @crazycs520

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jackysp PTAL

cfzjywxk
cfzjywxk commented Sep 29, 2019
View reviewed changes
session/txn.go
@@ -461,7 +461,9 @@ func (s *session) StmtCommit() error {
// StmtRollback implements the sessionctx.Context interface.
func (s *session) StmtRollback() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tiancaiamao PTAL if we can change StmtRollback like this

Copy link
Contributor

Choose a reason for hiding this comment

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

That's ok.
B.T.W, I'm thinking of removing the assertion proto which is introduced to add an additional check for write consistency, but I find it's hard to achieve that goal.

lonng
lonng reviewed Sep 29, 2019
View reviewed changes
server/server_test.go Outdated Show resolved Hide resolved
@tiancaiamao
Copy link
Contributor

LGTM @crazycs520

crazycs520
crazycs520 reviewed Oct 9, 2019
View reviewed changes
Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

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

LGTM

@crazycs520 crazycs520 added the status/LGT2 Indicates that a PR has LGTM 2. label Oct 9, 2019
@crazycs520
Copy link
Contributor

/run-all-tests

@cfzjywxk
Copy link
Contributor Author

/run-all-tests

@cfzjywxk
Copy link
Contributor Author

/run-all-tests

@cfzjywxk cfzjywxk requested a review from lonng October 10, 2019 07:53
@cfzjywxk cfzjywxk requested a review from lysu October 10, 2019 07:53
jackysp
jackysp approved these changes Oct 11, 2019
View reviewed changes
Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@jackysp jackysp added the status/can-merge Indicates a PR has been approved by a committer. label Oct 11, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Oct 11, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Oct 11, 2019

@cfzjywxk merge failed.

@cfzjywxk
Copy link
Contributor Author

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Oct 11, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Oct 11, 2019

@cfzjywxk merge failed.

@cfzjywxk
Copy link
Contributor Author

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Oct 11, 2019

Your auto merge job has been accepted, waiting for 12458

@cfzjywxk
Copy link
Contributor Author

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Oct 11, 2019

/run-all-tests

@sre-bot sre-bot merged commit fbf0d90 into pingcap:master Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crazycs520 crazycs520 crazycs520 left review comments

@jackysp jackysp jackysp approved these changes

@coocood coocood Awaiting requested review from coocood

@tiancaiamao tiancaiamao Awaiting requested review from tiancaiamao

@imtbkcat imtbkcat Awaiting requested review from imtbkcat

@lonng lonng Awaiting requested review from lonng

@lysu lysu Awaiting requested review from lysu

Assignees
No one assigned
Labels
sig/execution SIG execution sig/transaction SIG:Transaction status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bug-fix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@cfzjywxk @tiancaiamao @crazycs520 @sre-bot @lysu @lonng @jackysp