session: track LastCommitTS in SessionVars and check timestamps of later txns are larger #57305

b6g · 2024-11-12T00:01:10Z

What problem does this PR solve?

Issue Number: close #57165

Problem Summary:

This PR checks the invariant that timestamps of transactions in a session should increase monotonically.

It saves the timestamp of the last transaction in SessionVars, after the transaction is committed. The saved timestamp is compared with the start_ts and the commit_ts of the next transaction.

This PR depends on the TiKV client-go PR, tikv/client-go#1489, which exports commit_ts of the transaction.

What changed and how does it work?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

ti-chi-bot · 2024-11-12T00:01:20Z

Hi @b6g. Thanks for your PR.

I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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/test-infra repository.

ti-chi-bot · 2024-11-12T00:01:24Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign benmeadowcroft, djshow832 for approval, ensuring that each of them provides their approval before proceeding. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS
pkg/sessionctx/variable/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tiprow · 2024-11-12T00:01:28Z

Hi @b6g. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

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.

b6g · 2024-11-12T00:01:50Z

pkg/session/session.go

@@ -929,6 +929,21 @@ func (s *session) CommitTxn(ctx context.Context) error {
 		s.sessionVars.StmtCtx.MergeExecDetails(nil, commitDetail)
 	}

+	if err == nil {
+		// save CommitTS in sessionVars for invariant check
+		// TODO: enable LastCommitTS with a session variable


I will add a session variable.

Added in a separate PR #57313, to make review easier.

Per discussion in #57313 (comment), we don't need a session variable.

pkg/sessiontxn/isolation/base.go

dveeden · 2024-11-18T06:28:38Z

/ok-to-test

pkg/sessiontxn/isolation/base.go

go.mod

codecov · 2024-11-25T08:35:01Z

Codecov Report

Attention: Patch coverage is 75.00000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 73.1517%. Comparing base (1521bf7) to head (3ec86a7).

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #57305        +/-   ##
================================================
- Coverage   73.1843%   73.1517%   -0.0327%     
================================================
  Files          1674       1674                
  Lines        461690     461717        +27     
================================================
- Hits         337885     337754       -131     
- Misses       103042     103208       +166     
+ Partials      20763      20755         -8

Flag	Coverage Δ
unit	`72.2766% <75.0000%> (-0.0350%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`52.6910% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`46.0013% <ø> (+0.0120%)`	⬆️

b6g · 2024-11-27T08:19:27Z

The client-go PR is merged and go.mod is updated. The PR is ready for review. PTAL Thanks!

hawkingrei · 2024-11-27T08:30:02Z

go.mod

@@ -109,7 +109,7 @@ require (
 	github.com/tdakkota/asciicheck v0.2.0


Please run the make bazel_preapre and upload the dependencies.

b6g · 2024-11-27T17:34:32Z

Some tests failed with

Was it because the client-go PR was just merged and it takes time to create http://bazel-cache.pingcap.net:8080/gomod/github.com/tikv/client-go/v2/com_github_tikv_client_go_v2-v2.0.8-0.20241127071526-89643b0e8c9e.zip?

b6g · 2024-11-27T19:29:32Z

Some tests failed with

Was it because the client-go PR was just merged and it takes time to create http://bazel-cache.pingcap.net:8080/gomod/github.com/tikv/client-go/v2/com_github_tikv_client_go_v2-v2.0.8-0.20241127071526-89643b0e8c9e.zip?

The error was,

Error in download_and_extract: java.io.IOException: Error downloading [http://bazel-cache.pingcap.net:8080/gomod/github.com/tikv/client-go/v2/com_github_tikv_client_go_v2-v2.0.8-0.20241127071526-89643b0e8c9e.zip, http://ats.apps.svc/gomod/github.com/tikv/client-go/v2/com_github_tikv_client_go_v2-v2.0.8-0.20241127071526-89643b0e8c9e.zip, https://cache.hawkingrei.com/gomod/github.com/tikv/client-go/v2/com_github_tikv_client_go_v2-v2.0.8-0.20241127071526-89643b0e8c9e.zip, https://storage.googleapis.com/pingcapmirror/gomod/github.com/tikv/client-go/v2/com_github_tikv_client_go_v2-v2.0.8-0.20241127071526-89643b0e8c9e.zip] to /root/.cache/bazel/_bazel_root/37ff7b0eddea115f60ab9f853788d7f5/external/com_github_tikv_client_go_v2/temp12921244511850196136/com_github_tikv_client_go_v2-v2.0.8-0.20241127071526-89643b0e8c9e.zip: GET returned 404 Not Found

b6g · 2024-11-28T05:30:23Z

/test build

tiprow · 2024-11-28T05:30:45Z

@b6g: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test fast_test_tiprow
/test fast_test_tiprow_ddlargsv1
/test tidb_parser_test

Use /test all to run the following jobs that were automatically triggered:

fast_test_tiprow
tidb_parser_test

In response to this:

/test build

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.

b6g · 2024-11-28T18:31:33Z

/retest

b6g · 2024-11-29T04:01:11Z

Some tests failed with

Was it because the client-go PR was just merged and it takes time to create http://bazel-cache.pingcap.net:8080/gomod/github.com/tikv/client-go/v2/com_github_tikv_client_go_v2-v2.0.8-0.20241127071526-89643b0e8c9e.zip?

How can I update the client-go version? I tried,

go get github.com/tikv/client-go/v2@89643b0e8c9ee8cbd0fce4f264a8765c5665db17

But these tests failed.

hawkingrei · 2024-12-04T06:20:07Z

/retest

hawkingrei · 2024-12-04T06:20:33Z

Some tests failed with
Was it because the client-go PR was just merged and it takes time to create http://bazel-cache.pingcap.net:8080/gomod/github.com/tikv/client-go/v2/com_github_tikv_client_go_v2-v2.0.8-0.20241127071526-89643b0e8c9e.zip?

How can I update the client-go version? I tried,
go get github.com/tikv/client-go/v2@89643b0e8c9ee8cbd0fce4f264a8765c5665db17
But these tests failed.

I has uploaded it. you can try the unit test again.

hawkingrei · 2024-12-04T06:23:26Z

BTW, Can you sync your code with master ?

b6g · 2024-12-07T00:27:06Z

BTW, Can you sync your code with master ?

Will do. I am fixing the broken unittests.

… is larger

ti-chi-bot · 2024-12-07T16:37:05Z

[FORMAT CHECKER NOTIFICATION]

Notice: To remove the do-not-merge/needs-tests-checked label, please finished the tests then check the finished items in description.

For example:

Tests

Unit test

Integration test

Manual test (add detailed scripts or steps below)

No code

_{📖 For more info, you can check the "Contribute Code" section in the development guide.}

b6g · 2024-12-07T17:23:38Z

The unittests are fixed. PTAL

b6g · 2024-12-10T17:16:03Z

ping

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-tests-checked labels Nov 12, 2024

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Nov 12, 2024

b6g commented Nov 12, 2024

View reviewed changes

pkg/sessiontxn/isolation/base.go Outdated Show resolved Hide resolved

b6g force-pushed the b6g/tsinvariant1 branch from e8df58f to e053b6f Compare November 12, 2024 07:04

b6g mentioned this pull request Nov 12, 2024

variable: session variable tidb_enable_commit_ts_order_check #57313

Closed

13 tasks

b6g force-pushed the b6g/tsinvariant1 branch 2 times, most recently from 5cc54f3 to 8297ca5 Compare November 17, 2024 08:39

ti-chi-bot bot added ok-to-test Indicates a PR is ready to be tested. and removed needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Nov 18, 2024

dveeden reviewed Nov 18, 2024

View reviewed changes

pkg/sessiontxn/isolation/base.go Show resolved Hide resolved

dveeden reviewed Nov 18, 2024

View reviewed changes

go.mod Outdated Show resolved Hide resolved

b6g force-pushed the b6g/tsinvariant1 branch 2 times, most recently from 4e39c69 to 31193ac Compare November 19, 2024 03:38

you06 mentioned this pull request Nov 25, 2024

Export commitTS of KVTxn tikv/client-go#1489

Merged

b6g force-pushed the b6g/tsinvariant1 branch 3 times, most recently from fbfaf0f to 3c7fb77 Compare November 27, 2024 08:17

hawkingrei reviewed Nov 27, 2024

View reviewed changes

b6g force-pushed the b6g/tsinvariant1 branch from 1af9227 to ef63538 Compare November 27, 2024 18:50

b6g force-pushed the b6g/tsinvariant1 branch from ef63538 to d8cf419 Compare December 5, 2024 07:19

b6g added 5 commits December 7, 2024 08:36

session: track LastCommitTS in SessionVars and check StartTS of a txn…

450df2e

… is larger

test

639d83c

update client-go

32d42e9

bazel_prepare

e3c4981

dont check startts if it is preset

3ec86a7

b6g force-pushed the b6g/tsinvariant1 branch from d8cf419 to 3ec86a7 Compare December 7, 2024 16:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

session: track LastCommitTS in SessionVars and check timestamps of later txns are larger #57305

session: track LastCommitTS in SessionVars and check timestamps of later txns are larger #57305

b6g commented Nov 12, 2024

ti-chi-bot bot commented Nov 12, 2024

ti-chi-bot bot commented Nov 12, 2024

tiprow bot commented Nov 12, 2024

b6g Nov 12, 2024

b6g Nov 12, 2024

b6g Nov 25, 2024

dveeden commented Nov 18, 2024

codecov bot commented Nov 25, 2024 •

edited

Loading

b6g commented Nov 27, 2024

hawkingrei Nov 27, 2024

b6g commented Nov 27, 2024

b6g commented Nov 27, 2024

b6g commented Nov 28, 2024

tiprow bot commented Nov 28, 2024

b6g commented Nov 28, 2024

b6g commented Nov 29, 2024

hawkingrei commented Dec 4, 2024

hawkingrei commented Dec 4, 2024

hawkingrei commented Dec 4, 2024

b6g commented Dec 7, 2024

ti-chi-bot bot commented Dec 7, 2024

b6g commented Dec 7, 2024

b6g commented Dec 10, 2024

		@@ -109,7 +109,7 @@ require (
		github.com/tdakkota/asciicheck v0.2.0

session: track LastCommitTS in SessionVars and check timestamps of later txns are larger #57305

Are you sure you want to change the base?

session: track LastCommitTS in SessionVars and check timestamps of later txns are larger #57305

Conversation

b6g commented Nov 12, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Nov 12, 2024

ti-chi-bot bot commented Nov 12, 2024

tiprow bot commented Nov 12, 2024

b6g Nov 12, 2024

Choose a reason for hiding this comment

b6g Nov 12, 2024

Choose a reason for hiding this comment

b6g Nov 25, 2024

Choose a reason for hiding this comment

dveeden commented Nov 18, 2024

codecov bot commented Nov 25, 2024 • edited Loading

Codecov Report

b6g commented Nov 27, 2024

hawkingrei Nov 27, 2024

Choose a reason for hiding this comment

b6g commented Nov 27, 2024

b6g commented Nov 27, 2024

b6g commented Nov 28, 2024

tiprow bot commented Nov 28, 2024

b6g commented Nov 28, 2024

b6g commented Nov 29, 2024

hawkingrei commented Dec 4, 2024

hawkingrei commented Dec 4, 2024

hawkingrei commented Dec 4, 2024

b6g commented Dec 7, 2024

ti-chi-bot bot commented Dec 7, 2024

b6g commented Dec 7, 2024

b6g commented Dec 10, 2024

codecov bot commented Nov 25, 2024 •

edited

Loading