variable: session variable tidb_enable_commit_ts_order_check #57313

b6g · 2024-11-12T07:18:06Z

What problem does this PR solve?

Issue Number: ref #57165

Problem Summary:

Add a session variable tidb_enable_commit_ts_order_check to enable checking commit_ts in #57305.

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-12T07:18:16Z

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-12T07:18:27Z

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.

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

/ok-to-test

ti-chi-bot · 2024-11-18T06:41:48Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dveeden
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~~ [dveeden]
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 · 2024-11-18T06:41:50Z

[LGTM Timeline notifier]

Timeline:

2024-11-18 06:41:49.933464172 +0000 UTC m=+856872.124333162: ☑️ agreed by dveeden.

codecov · 2024-11-18T06:51:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.4246%. Comparing base (058d947) to head (02c28aa).
Report is 62 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #57313        +/-   ##
================================================
+ Coverage   72.8577%   73.4246%   +0.5668%     
================================================
  Files          1672       1672                
  Lines        462711     462756        +45     
================================================
+ Hits         337121     339777      +2656     
+ Misses       104809     102274      -2535     
+ Partials      20781      20705        -76

Flag	Coverage Δ
integration	`43.3628% <100.0000%> (?)`
unit	`72.2396% <100.0000%> (-0.0055%)`	⬇️

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

Components	Coverage Δ
dumpling	`52.7673% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`45.0969% <ø> (+0.0153%)`	⬆️

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

cfzjywxk · 2024-11-18T08:48:11Z

pkg/sessionctx/variable/sysvar.go

@@ -1915,6 +1915,10 @@ var defaultSysVars = []*SysVar{
 		s.allowTiFlashCop = TiDBOptOn(val)
 		return nil
 	}},
+	{Scope: ScopeSession, Name: TiDBEnableCommitTSOrderCheck, Value: Off, Type: TypeBool, SetSession: func(s *SessionVars, val string) error {
+		s.EnableCommitTSOrderCheck = TiDBOptOn(val)


The already exists tidb_last_txn_info could be used to track the last transaction information in current session.

It stores the json encoded TxnInfo. We have to json decode to use the last commit_ts, which seems hacky.

#57305 implements the check.

Does this make sense?

Since this check condition should always hold, it can always be verified without introducing a dedicated parameter. Introducing system variables or configuration items typically increases the using complexity for users.

From an implementation perspective, introducing a separate interface to expose the commit_ts of the previous transaction is also costly. The last_txn_info cannot be directly used as it requires deserialization.

Another possible way is to modify the txn commit hook function interface, allowing the KV client to set the commit_ts into a specific field within the session variable's statement context during the commit process like sessVars.stmtContext.commitTS = commitTS.

I will try txn commit hook function. Thanks!

@cfzjywxk Is this what you suggested 571c73e#diff-3749ef84f018df1c4d5541271a230702b5c8200c8f44268ffdff63886842f268R521?

I feel commitTS should be stored in the SessionVars.LastCommitTS instead of StmtCtx, because StmtCtx is for the current txn, while LastCommitTS is from the last txn.

I can refactor CommitHook to take *TxnInfo in a separate PR, which involves client-go.

It seems that using the commit hook approach would still require modifying the client-go interface if we want to avoid deserializing txn info.

Perhaps the original approach in [PR #1489 of client-go] is better, by adding a commit_ts interface to the transaction in client-go for TiDB to use. It would avoid the performance impact caused by deserializing txn info.

If there are future scenarios requiring access to the last transaction info, we could consider extending the newly exported commit_ts interface into a last_txn_info interface. This would allow it to return structured info instead of a string.

sgtm

As you pointed out, this session variable is not necessary, as we should always check commit_s. I will close this PR.

ti-chi-bot bot removed the do-not-merge/needs-tests-checked label Nov 12, 2024

b6g mentioned this pull request Nov 12, 2024

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

Open

13 tasks

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 approved these changes Nov 18, 2024

View reviewed changes

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

cfzjywxk reviewed Nov 18, 2024

View reviewed changes

variable: session variable tidb_enable_commit_ts_order_check

02c28aa

b6g force-pushed the b6g/tsvar1 branch from 4408668 to 02c28aa Compare November 19, 2024 03:26

b6g closed this Nov 25, 2024

b6g deleted the b6g/tsvar1 branch November 25, 2024 17:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

variable: session variable tidb_enable_commit_ts_order_check #57313

variable: session variable tidb_enable_commit_ts_order_check #57313

b6g commented Nov 12, 2024 •

edited

Loading

ti-chi-bot bot commented Nov 12, 2024

tiprow bot commented Nov 12, 2024

dveeden commented Nov 18, 2024

ti-chi-bot bot commented Nov 18, 2024

ti-chi-bot bot commented Nov 18, 2024

codecov bot commented Nov 18, 2024 •

edited

Loading

cfzjywxk Nov 18, 2024

b6g Nov 19, 2024

b6g Nov 21, 2024

cfzjywxk Nov 21, 2024 •

edited

Loading

b6g Nov 22, 2024

b6g Nov 25, 2024

cfzjywxk Nov 25, 2024

b6g Nov 25, 2024

variable: session variable tidb_enable_commit_ts_order_check #57313

variable: session variable tidb_enable_commit_ts_order_check #57313

Conversation

b6g commented Nov 12, 2024 • edited Loading

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

dveeden commented Nov 18, 2024

ti-chi-bot bot commented Nov 18, 2024

ti-chi-bot bot commented Nov 18, 2024

[LGTM Timeline notifier]

codecov bot commented Nov 18, 2024 • edited Loading

Codecov Report

cfzjywxk Nov 18, 2024

Choose a reason for hiding this comment

b6g Nov 19, 2024

Choose a reason for hiding this comment

b6g Nov 21, 2024

Choose a reason for hiding this comment

cfzjywxk Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

b6g Nov 22, 2024

Choose a reason for hiding this comment

b6g 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

b6g commented Nov 12, 2024 •

edited

Loading

codecov bot commented Nov 18, 2024 •

edited

Loading

cfzjywxk Nov 21, 2024 •

edited

Loading