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

session: annotate the previous statement to the error when transaction commit failed #12087

Merged
merged 17 commits into from
Sep 16, 2019

Conversation

jackysp
Copy link
Member

@jackysp jackysp commented Sep 9, 2019

Signed-off-by: Shuaipeng Yu jackysp@gmail.com

What problem does this PR solve?

The user may want to know the SQL statement in the transaction when commit failed.

What is changed and how it works?

Annotate the previous statement to the error when transaction commit failed.

Check List

Tests

  • Manual test (add detailed scripts or steps below)
    Make an write conflicts case, then search previous statement in log files
[2019/09/09 12:54:12.346 +08:00] [WARN] [session.go:493] ["commit failed"] [conn=3] ["finished txn"="Txn{state=invalid}"] [error="previous statement: update t set j = ? where i = ? [arguments: -3, 3]: [kv:8005]Write conflict, txnStartTS 411043064012079104 is stale [try again later]"] [errorVerbose="[kv:8005]Write conflict, txnStartTS 411043064012079104 is stale [try again later]\nprevious statement: update t set j = ? where i = ? [arguments: -3, 3]"]
[2019/09/09 12:54:12.346 +08:00] [WARN] [session.go:1010] ["run statement error"] [conn=3] [schemaVersion=29] [error="previous statement: update t set j = ? where i = ? [arguments: -3, 3]: [kv:8005]Write conflict, txnStartTS 411043064012079104 is stale [try again later]"] [errorVerbose="[kv:8005]Write conflict, txnStartTS 411043064012079104 is stale [try again later]\nprevious statement: update t set j = ? where i = ? [arguments: -3, 3]"] [session="{\n  \"currDBName\": \"test\",\n  \"id\": 3,\n  \"preparedStmtCount\": 1,\n  \"status\": 2,\n  \"strictMode\": true,\n  \"user\": {\n    \"Username\": \"root\",\n    \"Hostname\": \"127.0.0.1\",\n    \"CurrentUser\": false,\n    \"AuthUsername\": \"root\",\n    \"AuthHostname\": \"%\"\n  }\n}"]
[2019/09/09 12:54:12.346 +08:00] [WARN] [conn.go:669] ["dispatch error"] [conn=3] [connInfo="id:3, addr:127.0.0.1:61812 status:2, collation:utf8_general_ci, user:root"] [sql=COMMIT] [err="[kv:8005]Write conflict, txnStartTS 411043064012079104 is stale [try again later]\nprevious statement: update t set j = ? where i = ? [arguments: -3, 3]"]

Code changes

  • Has exported function/method change

Related changes

  • Need to cherry-pick to the release branch

…n commmit failed

Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>
@jackysp jackysp added the type/enhancement The issue or PR belongs to an enhancement. label Sep 9, 2019
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>
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.

Rest LGTM

tools/check/go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>
@jackysp
Copy link
Member Author

jackysp commented Sep 9, 2019

PTAL @crazycs520

@lysu
Copy link
Contributor

lysu commented Sep 10, 2019

/bench

@sre-bot
Copy link
Contributor

sre-bot commented Sep 10, 2019

@@                               Benchmark Diff                               @@
================================================================================
--- tidb: 660ce3f16138fac1855d3bd72378225cee6a7b53
+++ tidb: 25eaf6f9c99365a41d8afcfd5ac30e82f31fe0ea
tikv: 9bee65d186241bccf7b3e086a86c1b025eca336c
pd: 9098f21b1be28a36be9e50deef6f7216187926b8
================================================================================
test-1: < oltp_insert >
    * QPS : 21105.72 ± 0.7955% (std=109.04) delta: -0.41%
    * AvgMs : 12.12 ± 0.7918% (std=0.06) delta: 0.43%
    * PercentileMs99 : 42.92 ± 1.0903% (std=0.38) delta: 0.73%
            
test-2: < oltp_update_non_index >
    * QPS : 29334.11 ± 0.3559% (std=69.35) delta: -0.31%
    * AvgMs : 8.72 ± 0.3726% (std=0.02) delta: 0.30%
    * PercentileMs99 : 30.59 ± 2.8568% (std=0.56) delta: 0.01%
            
test-3: < oltp_read_write >
    * QPS : 36812.43 ± 0.2108% (std=51.36) delta: -0.28%
    * AvgMs : 139.64 ± 0.2005% (std=0.20) delta: 0.29%
    * PercentileMs99 : 257.95 ± 0.0000% (std=0.00) delta: 0.00%
            
test-4: < oltp_point_select >
    * QPS : 75143.30 ± 1.8531% (std=841.07) delta: -1.21%
    * AvgMs : 3.41 ± 1.9378% (std=0.04) delta: 1.19%
    * PercentileMs99 : 7.51 ± 1.0389% (std=0.06) delta: 1.05%
            
test-5: < oltp_update_index >
    * QPS : 16788.21 ± 1.4336% (std=186.56) delta: 0.14%
    * AvgMs : 15.25 ± 1.4168% (std=0.17) delta: -0.14%
    * PercentileMs99 : 48.34 ± 0.0000% (std=0.00) delta: 0.00%
            

https://perf.pingcap.com

@jackysp
Copy link
Member Author

jackysp commented Sep 10, 2019

/bench

@sre-bot
Copy link
Contributor

sre-bot commented Sep 10, 2019

@@                               Benchmark Diff                               @@
================================================================================
--- tidb: e6e3e630606767cab4a9dbac77241772848bc9a8
+++ tidb: db07b1d7899c6068a5183d36273a8dbc996f6afd
tikv: 96220b40a30fcf29043736d89fd1590039ec209b
pd: e678a1f5c022fed729fb79397fe02b6c9f54ff4a
================================================================================
test-1: < oltp_insert >
    * QPS : 21401.26 ± 0.0867% (std=13.82) delta: 0.48%
    * AvgMs : 11.96 ± 0.0558% (std=0.00) delta: -0.46%
    * PercentileMs99 : 42.61 ± 0.0000% (std=0.00) delta: 0.00%
            
test-2: < oltp_update_non_index >
    * QPS : 29490.97 ± 0.4168% (std=74.13) delta: 0.13%
    * AvgMs : 8.68 ± 0.3687% (std=0.02) delta: -0.12%
    * PercentileMs99 : 30.59 ± 1.0788% (std=0.27) delta: 0.00%
            
test-3: < oltp_read_write >
    * QPS : 37050.74 ± 0.4773% (std=117.64) delta: 0.37%
    * AvgMs : 138.74 ± 0.4598% (std=0.43) delta: -0.36%
    * PercentileMs99 : 257.95 ± 0.0000% (std=0.00) delta: 0.00%
            
test-4: < oltp_point_select >
    * QPS : 74165.37 ± 0.3771% (std=200.00) delta: -1.41%
    * AvgMs : 3.45 ± 0.2899% (std=0.01) delta: 1.41%
    * PercentileMs99 : 7.43 ± 0.0000% (std=0.00) delta: -1.04%
            
test-5: < oltp_update_index >
    * QPS : 16931.01 ± 0.0183% (std=2.23) delta: 0.09%
    * AvgMs : 15.12 ± 0.0000% (std=0.00) delta: -0.07%
    * PercentileMs99 : 48.34 ± 0.0000% (std=0.00) delta: 0.00%
            

https://perf.pingcap.com

Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>
@jackysp
Copy link
Member Author

jackysp commented Sep 11, 2019

PTAL @lysu

Copy link
Contributor

@lysu lysu left a comment

Choose a reason for hiding this comment

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

LGTM

@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao added the status/LGT2 Indicates that a PR has LGTM 2. label Sep 12, 2019
tiancaiamao
tiancaiamao previously approved these changes Sep 12, 2019
@tiancaiamao tiancaiamao added the status/can-merge Indicates a PR has been approved by a committer. label Sep 12, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Sep 12, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Sep 12, 2019

@jackysp merge failed.

@jackysp
Copy link
Member Author

jackysp commented Sep 12, 2019

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Sep 12, 2019

/run-all-tests

@jackysp
Copy link
Member Author

jackysp commented Sep 16, 2019

/run-all-tests

1 similar comment
@jackysp
Copy link
Member Author

jackysp commented Sep 16, 2019

/run-all-tests

@jackysp jackysp added status/all tests passed and removed status/can-merge Indicates a PR has been approved by a committer. labels Sep 16, 2019
Copy link
Contributor

@lysu lysu left a comment

Choose a reason for hiding this comment

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

LGTM

@lysu lysu added the status/can-merge Indicates a PR has been approved by a committer. label Sep 16, 2019
@codecov
Copy link

codecov bot commented Sep 16, 2019

Codecov Report

Merging #12087 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #12087   +/-   ##
===========================================
  Coverage   81.2533%   81.2533%           
===========================================
  Files           454        454           
  Lines         98167      98167           
===========================================
  Hits          79764      79764           
  Misses        12683      12683           
  Partials       5720       5720

@sre-bot
Copy link
Contributor

sre-bot commented Sep 16, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Sep 16, 2019

@jackysp merge failed.

@jackysp jackysp added status/can-merge Indicates a PR has been approved by a committer. and removed status/can-merge Indicates a PR has been approved by a committer. labels Sep 16, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Sep 16, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Sep 16, 2019

@jackysp merge failed.

@jackysp jackysp merged commit 04292a0 into pingcap:master Sep 16, 2019
jackysp added a commit to jackysp/tidb that referenced this pull request Sep 23, 2019
…n commit failed (pingcap#12087)

 Conflicts:
	ddl/db_test.go
	session/tidb.go
sre-bot pushed a commit that referenced this pull request Sep 24, 2019
jackysp added a commit to jackysp/tidb that referenced this pull request Oct 16, 2019
sre-bot pushed a commit that referenced this pull request Oct 16, 2019
jackysp added a commit to jackysp/tidb that referenced this pull request Nov 7, 2019
sre-bot pushed a commit that referenced this pull request Nov 7, 2019
@jackysp jackysp deleted the prev_commit_err branch February 27, 2020 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants