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.

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.5884%. Comparing base (0c22a2d) to head (60a0008).
Report is 79 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #57305        +/-   ##
================================================
+ Coverage   73.5156%   73.5884%   +0.0728%     
================================================
  Files          1681       1681                
  Lines        464346     464373        +27     
================================================
+ Hits         341367     341725       +358     
+ Misses       102149     101830       -319     
+ Partials      20830      20818        -12

Flag	Coverage Δ
integration	`43.3034% <35.7142%> (?)`
unit	`72.3138% <75.0000%> (+0.0406%)`	⬆️

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

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

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

br/pkg/backup/BUILD.bazel

b6g · 2024-12-20T23:41:40Z

Merge conflicts are resolved. PTAL

ekexium · 2024-12-23T11:01:23Z

pkg/session/session.go

+			logutil.BgLogger().Panic("check lastCommitTS failed",
+				zap.Uint64("sessionLastCommitTS", s.sessionVars.LastCommitTS),
+				zap.Uint64("txnLastCommitTS", s.txn.lastCommitTS),
+				zap.String("sql", s.sessionVars.StmtCtx.OriginalSQL),


Suggested change

zap.String("sql", s.sessionVars.StmtCtx.OriginalSQL),

zap.String("sql", redact.String(s.sessionVars.EnableRedactLog, s.sessionVars.StmtCtx.OriginalSQL)),

Also for the other log

ekexium · 2024-12-23T11:10:22Z

pkg/kv/kv.go

@@ -251,6 +251,8 @@ type Transaction interface {
 	IsReadOnly() bool
 	// StartTS returns the transaction start timestamp.
 	StartTS() uint64
+	// CommitTS returns the transaction commit timestamp.


Suggested change

// CommitTS returns the transaction commit timestamp.

// CommitTS returns the commit timestamp of the already committed transaction, or zero if it's not committed yet.

ekexium · 2024-12-23T11:21:39Z

pkg/sessiontxn/isolation/base.go

@@ -268,6 +270,10 @@ func (p *baseTxnContextProvider) getTxnStartTS() (uint64, error) {
 	return txn.StartTS(), nil
 }

+func (p *baseTxnContextProvider) usePresetStartTS() bool {
+	return p.constStartTS != 0 || p.sctx.GetSessionVars().SnapshotTS != 0


I'm not sure about this: SnapshotTS != 0 should imply a staleness txn ctx provider? It won't hurt though

I am not sure either. That's why I named it Preset. The point is we don't check StartTS if it is preset.

@ekexium @b6g

Here, considering various historical features (e.g., tidb_snapshot, stale read, etc.), careful attention is required to ensure accurate and comprehensive condition checks. Any oversight could lead to unexpected tidb-server panics.

An alternative approach is to reverse the logic—confirming that the current transaction is activated by a PD-allocated timestamp before proceeding with the check. This seems to be a more reliable method.

like

- if !usePresetStartTS + if activiatedByPDAllocatedTS

ti-chi-bot · 2024-12-24T05:30:56Z

@b6g: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-br-integration-test	`e1ee91c`	link	true	`/test pull-br-integration-test`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

… is larger

ti-chi-bot · 2024-12-25T01:39:13Z

[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 · 2025-01-05T07:37:19Z

ping

ti-chi-bot · 2025-01-08T04:38:08Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ekexium
Once this PR has been reviewed and has the lgtm label, please assign benmeadowcroft for approval. 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~~ [ekexium]
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

ti-chi-bot · 2025-01-08T04:38:11Z

[LGTM Timeline notifier]

Timeline:

2025-01-08 04:38:11.286280732 +0000 UTC m=+328434.575112437: ☑️ agreed by ekexium.

ekexium · 2025-01-08T04:44:27Z

The PR is good to merge as is. We could improve the lastCommitTs design to avoid the confusion.

b6g · 2025-01-10T02:19:24Z

I think I need another approval

cfzjywxk · 2025-01-10T03:08:49Z

@Benjamin2037 @yudongusa
The session variable file is modified, PTAL for the approvement.

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

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/planner SIG: Planner labels Dec 20, 2024

b6g force-pushed the b6g/tsinvariant1 branch from d0c5266 to e1ee91c Compare December 20, 2024 07:54

b6g commented Dec 20, 2024

View reviewed changes

br/pkg/backup/BUILD.bazel Outdated Show resolved Hide resolved

b6g force-pushed the b6g/tsinvariant1 branch from e1ee91c to 9a2b35a Compare December 20, 2024 18:19

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 20, 2024

cfzjywxk requested review from you06, MyonKeminta, ekexium and crazycs520 December 21, 2024 15:36

ekexium reviewed Dec 23, 2024

View reviewed changes

b6g added 6 commits December 24, 2024 17:38

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

ba7df89

… is larger

test

db72d08

dont check startts if it is preset

abf573d

bazel_prepare

6f87b7f

redact sql string

1f8567a

bazel_prepare

60a0008

b6g force-pushed the b6g/tsinvariant1 branch from 708731b to 60a0008 Compare December 25, 2024 01:39

ekexium approved these changes Jan 8, 2025

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jan 8, 2025

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

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

b6g commented Dec 20, 2024

ekexium Dec 23, 2024

ekexium Dec 23, 2024

ekexium Dec 23, 2024

ekexium Dec 23, 2024

b6g Dec 24, 2024

cfzjywxk Jan 14, 2025

ti-chi-bot bot commented Dec 24, 2024 •

edited

Loading

ti-chi-bot bot commented Dec 25, 2024

b6g commented Jan 5, 2025

ti-chi-bot bot commented Jan 8, 2025

ti-chi-bot bot commented Jan 8, 2025

ekexium commented Jan 8, 2025

b6g commented Jan 10, 2025

cfzjywxk commented Jan 10, 2025

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

	zap.String("sql", s.sessionVars.StmtCtx.OriginalSQL),
	zap.String("sql", redact.String(s.sessionVars.EnableRedactLog, s.sessionVars.StmtCtx.OriginalSQL)),

	// CommitTS returns the transaction commit timestamp.
	// CommitTS returns the commit timestamp of the already committed transaction, or zero if it's not committed yet.

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

tiprow bot commented Nov 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

b6g commented Nov 27, 2024

b6g commented Nov 27, 2024

b6g commented Nov 28, 2024

b6g commented Dec 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Dec 24, 2024 • edited Loading

ti-chi-bot bot commented Dec 25, 2024

b6g commented Jan 5, 2025

ti-chi-bot bot commented Jan 8, 2025

ti-chi-bot bot commented Jan 8, 2025

[LGTM Timeline notifier]

ekexium commented Jan 8, 2025

b6g commented Jan 10, 2025

cfzjywxk commented Jan 10, 2025

codecov bot commented Nov 25, 2024 •

edited

Loading

ti-chi-bot bot commented Dec 24, 2024 •

edited

Loading