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

*: print the statement causing write conflicts #14626

Closed
wants to merge 1 commit into from

Conversation

sticnarf
Copy link
Contributor

@sticnarf sticnarf commented Feb 4, 2020

What problem does this PR solve?

Fixes #14593

What is changed and how it works?

MySQL [test]> select * from t;
+----+------+
| pk | v    |
+----+------+
|  1 |    1 |
|  2 |    2 |
+----+------+
2 rows in set (0.002 sec)
Client 1 Client2
begin optimistic;
update t set v=2 where pk=1;
update t set v=3 where pk=2;
update t set v=0 where pk=1;
commit;

The result will be:

MySQL [test]> commit;
ERROR 9007 (HY000): Write conflict, txnStartTS=414420145598627841, conflictStartTS=414420147931709441, conflictCommitTS=414420147931709442, key={tableID=5262, handle=1} primary={tableID=5262, handle=1}, conflict cause: update t set v=2 where pk=1 [try again later]

In order to know which statement causes the conflict, we need to store the statement index together with the kv pair. So MemBuffer is modified to support storing *extra bytes associated with keys. Here, the extra bytes are used to store the statement index. We might also enlarge the space and store something else.

Because ticlient does not have access to the statement history, we need to return the conflicting statement index to session. However, errors returned from ticlient don't carry data (only messages), so the conflicting statement index is just stored in tikvTxn and is fetched by the caller.

To add the statement text to error message, I have no other idea but use a dirty way. parser/terror is changed to expose the inner arguments. So I can change the messages in ErrWriteConflict.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change
  • Has exported variable/fields change
  • Has interface methods change

Side effects

@sticnarf
Copy link
Contributor Author

sticnarf commented Feb 4, 2020

/bench +sysbench

@sre-bot
Copy link
Contributor

sre-bot commented Feb 4, 2020

Benchmark Report

Run Sysbench Performance Test on VMs

@@                               Benchmark Diff                               @@
================================================================================
--- tidb: 7ba002e83642c2cfed90de0d6ddbf98376a201f6
+++ tidb: b322dca1dc60d87ea81e95ef7df49215160f774d
tikv: 0f08b6beafe17825329d4a1c00a22db6b6699e6c
pd: 4686d4622ad3ebc7fc267b5c64437129cee356fd
================================================================================
oltp_insert:
    * QPS: 7273.31 ± 0.93% (std=42.47) delta: 0.14% (p=0.517)
    * Latency p50: 17.59 ± 0.92% (std=0.10) delta: -0.16%
    * Latency p99: 30.55 ± 4.56% (std=0.92) delta: 2.78%
            
oltp_point_select:
    * QPS: 42620.12 ± 0.45% (std=134.47) delta: 0.98% (p=0.055)
    * Latency p50: 3.00 ± 0.50% (std=0.01) delta: -0.61%
    * Latency p99: 10.09 ± 0.00% (std=0.00) delta: -1.18%
            
oltp_read_write:
    * QPS: 14508.33 ± 0.25% (std=25.82) delta: -0.25% (p=0.148)
    * Latency p50: 176.86 ± 0.20% (std=0.25) delta: 0.28%
    * Latency p99: 327.60 ± 5.03% (std=9.81) delta: 0.48%
            
oltp_update_index:
    * QPS: 4233.11 ± 0.15% (std=5.24) delta: 0.39% (p=0.411)
    * Latency p50: 30.24 ± 0.16% (std=0.04) delta: -0.38%
    * Latency p99: 52.57 ± 1.20% (std=0.45) delta: -3.31%
            
oltp_update_non_index:
    * QPS: 4702.72 ± 1.03% (std=31.78) delta: -0.30% (p=0.505)
    * Latency p50: 27.21 ± 1.05% (std=0.19) delta: 0.29%
    * Latency p99: 42.25 ± 2.71% (std=1.14) delta: 1.55%
            

@sticnarf sticnarf force-pushed the conflict-statement branch 5 times, most recently from f8fb379 to c16caa6 Compare February 5, 2020 07:13
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf
Copy link
Contributor Author

sticnarf commented Feb 5, 2020

/run-integration-tests

@sticnarf sticnarf marked this pull request as ready for review February 5, 2020 07:41
@sticnarf sticnarf requested a review from a team as a code owner February 5, 2020 07:41
@ghost ghost requested review from eurekaka and winoros and removed request for a team February 5, 2020 07:41
@sticnarf sticnarf changed the title [WIP] *: print the statement causing write conflicts *: print the statement causing write conflicts Feb 5, 2020
@@ -760,43 +760,6 @@ func (s *seqTestSuite) TestAdminShowNextID(c *C) {
r.Check(testkit.Rows("test1 tt id 41"))
}

func (s *seqTestSuite) TestNoHistoryWhenDisableRetry(c *C) {
Copy link
Member

Choose a reason for hiding this comment

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

removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This feature requires history to be always saved. As mentioned in the PR description, #11192 is basically reverted.

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.

After discussions with @sticnarf , we feel that the implementation cost of this pr is a bit high, and the benefits brought by it are not too large for the master and release-3.0, because the new environment is now pessimistic by default. It makes sense for 2.1, but there are a lot of changes and it is not suitable to backport to release 2.1. @coocood and @siddontang PTAL.

@sticnarf
Copy link
Contributor Author

sticnarf commented Feb 7, 2020

Close this PR as the benefit does not match its cost.

@sticnarf sticnarf closed this Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Print the SQL causing write conflict in logs
3 participants