Export commitTS of KVTxn #1489

b6g · 2024-11-11T02:18:02Z

Export KVTxn.CommitTS to make it available in TiDB.

ti-chi-bot · 2024-11-11T02:18:11Z

Welcome @b6g!

It looks like this is your first PR to tikv/client-go 🎉.

I'm the bot to help you request reviewers, add labels and more, See available commands.

We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to tikv/client-go. 😃

b6g · 2024-11-17T05:40:09Z

ping

you06

Export CommitTS LGTM.

I also take a look at pingcap/tidb#57305 which guarantee txn.commit_ts > txn.start_ts and txn.commit_ts > prev_txn.commit_ts in session.

To ensure the start_ts is greater than any exist commit_ts cross sessions, we may have a field last_commit_ts in store level and set it to the latest commit_ts. Each time, we fetch start_ts like this:

last_commit_ts = store.last_commit_ts.Load()
start_ts = oracle.GetTSO()
if start_ts < last_commit_ts {
  panic(...)
}

This may be another task, @cfzjywxk what's your idea.

you06 · 2024-11-25T05:35:56Z

txnkv/transaction/txn.go

@@ -1700,6 +1700,10 @@ func (txn *KVTxn) StartTS() uint64 {
 	return txn.startTS
 }

+func (txn *KVTxn) CommitTS() uint64 {


There are still some tests using txn.GetCommitTS in 1pc_test.go.

cd integration_tests && go test ./...

@you06 Seems to be an enhancement to add a check before and after the ts allocation, how about filing an issue for it?

The integration_tests are fixed.

Sounds a good idea to check store's last_commit_ts is smaller than start_ts, because store's last_commit_ts is not directly allocated from pd.

how about filing an issue for it?

@cfzjywxk It's mentioned in pingcap/tidb#57165 IMO.

In theory, all read requests occurring after the write transaction should use a read_ts greater than the previous known transaction's commit_ts.

Signed-off-by: Bin Zhang <[email protected]>

cfzjywxk · 2024-11-26T01:50:44Z

txnkv/transaction/txn.go

@@ -1704,6 +1704,10 @@ func (txn *KVTxn) StartTS() uint64 {
 	return txn.startTS
 }

+func (txn *KVTxn) CommitTS() uint64 {


Suggested change

func (txn *KVTxn) CommitTS() uint64 {

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

func (txn *KVTxn) CommitTS() uint64 {

Exported functions need explicity comments from the ci & lint.

Signed-off-by: Bin Zhang <[email protected]>

cfzjywxk

LGTM

b6g · 2024-11-27T02:41:28Z

The Integration Test failed because of timeout. Is it expected? How to trigger it to re-run?

b6g · 2024-11-27T06:12:49Z

@you06 Can I get another approval to merge the PR? Thanks!

ti-chi-bot · 2024-11-27T07:12:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfzjywxk, you06

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cfzjywxk,you06]

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 · 2024-11-27T07:12:49Z

[LGTM Timeline notifier]

Timeline:

2024-11-27 01:46:30.714494546 +0000 UTC m=+600978.334149056: ☑️ agreed by cfzjywxk.
2024-11-27 07:12:48.185389314 +0000 UTC m=+620555.805043827: ☑️ agreed by you06.

cfzjywxk · 2024-11-27T08:19:04Z

@b6g
Thanks for the contribution!
For the tidb pr, besides the checking within the same session perhaps we could consider @you06 has proposed

last_commit_ts = store.last_commit_ts.Load()
start_ts = oracle.GetTSO()
if start_ts < last_commit_ts {
  panic(...)
}

checking the last commit timestamp after fetching a new ts.

ti-chi-bot bot added dco-signoff: no Indicates the PR's author has not signed dco. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 11, 2024

b6g force-pushed the b6g/tsoinvariant1 branch from 337f334 to fdb6200 Compare November 11, 2024 03:12

ti-chi-bot bot added dco-signoff: yes Indicates the PR's author has signed the dco. and removed dco-signoff: no Indicates the PR's author has not signed dco. labels Nov 11, 2024

b6g force-pushed the b6g/tsoinvariant1 branch from fdb6200 to be41b52 Compare November 11, 2024 09:29

ti-chi-bot bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 11, 2024

b6g force-pushed the b6g/tsoinvariant1 branch from be41b52 to da62319 Compare November 11, 2024 22:03

ti-chi-bot bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 11, 2024

b6g mentioned this pull request Nov 12, 2024

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

Open

13 tasks

you06 self-requested a review November 25, 2024 05:29

you06 reviewed Nov 25, 2024

View reviewed changes

cfzjywxk mentioned this pull request Nov 25, 2024

variable: session variable tidb_enable_commit_ts_order_check pingcap/tidb#57313

Closed

13 tasks

b6g force-pushed the b6g/tsoinvariant1 branch from f47074d to 22f2749 Compare November 25, 2024 17:36

ti-chi-bot bot added dco-signoff: yes Indicates the PR's author has signed the dco. and removed dco-signoff: no Indicates the PR's author has not signed dco. labels Nov 25, 2024

b6g added 2 commits November 25, 2024 10:45

Export commitTS of KVTxn

4101598

Signed-off-by: Bin Zhang <[email protected]>

replace GetCommitTS with CommitTS in integration_tests

e689dcf

Signed-off-by: Bin Zhang <[email protected]>

b6g force-pushed the b6g/tsoinvariant1 branch from 22f2749 to e689dcf Compare November 25, 2024 18:45

cfzjywxk reviewed Nov 26, 2024

View reviewed changes

ti-chi-bot bot added dco-signoff: no Indicates the PR's author has not signed dco. and removed dco-signoff: yes Indicates the PR's author has signed the dco. labels Nov 26, 2024

comment

0e396b2

Signed-off-by: Bin Zhang <[email protected]>

b6g force-pushed the b6g/tsoinvariant1 branch from a97868a to 0e396b2 Compare November 26, 2024 17:06

ti-chi-bot bot added dco-signoff: yes Indicates the PR's author has signed the dco. and removed dco-signoff: no Indicates the PR's author has not signed dco. labels Nov 26, 2024

cfzjywxk approved these changes Nov 27, 2024

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Nov 27, 2024

cfzjywxk requested a review from you06 November 27, 2024 01:47

you06 approved these changes Nov 27, 2024

View reviewed changes

ti-chi-bot bot added the lgtm label Nov 27, 2024

ti-chi-bot bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Nov 27, 2024

ti-chi-bot bot merged commit 89643b0 into tikv:master Nov 27, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Export commitTS of KVTxn #1489

Export commitTS of KVTxn #1489

b6g commented Nov 11, 2024

ti-chi-bot bot commented Nov 11, 2024

b6g commented Nov 17, 2024

you06 left a comment

you06 Nov 25, 2024

cfzjywxk Nov 25, 2024

b6g Nov 25, 2024

you06 Nov 26, 2024

cfzjywxk Nov 26, 2024

cfzjywxk left a comment

b6g commented Nov 27, 2024

b6g commented Nov 27, 2024

ti-chi-bot bot commented Nov 27, 2024

ti-chi-bot bot commented Nov 27, 2024

cfzjywxk commented Nov 27, 2024

	func (txn *KVTxn) CommitTS() uint64 {
	// CommitTS returns the commit timestamp of the already committed transaction, zero if it's not committed yet.
	func (txn *KVTxn) CommitTS() uint64 {

Export commitTS of KVTxn #1489

Export commitTS of KVTxn #1489

Conversation

b6g commented Nov 11, 2024

ti-chi-bot bot commented Nov 11, 2024

b6g commented Nov 17, 2024

you06 left a comment

Choose a reason for hiding this comment

you06 Nov 25, 2024

Choose a reason for hiding this comment

cfzjywxk Nov 25, 2024

Choose a reason for hiding this comment

b6g Nov 25, 2024

Choose a reason for hiding this comment

you06 Nov 26, 2024

Choose a reason for hiding this comment

cfzjywxk Nov 26, 2024

Choose a reason for hiding this comment

cfzjywxk left a comment

Choose a reason for hiding this comment

b6g commented Nov 27, 2024

b6g commented Nov 27, 2024

ti-chi-bot bot commented Nov 27, 2024

ti-chi-bot bot commented Nov 27, 2024

[LGTM Timeline notifier]

cfzjywxk commented Nov 27, 2024