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.

Sign up for GitHub

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

variable: session variable tidb_enable_commit_ts_order_check #57313

Closed
wants to merge 1 commit into from
Closed

variable: session variable tidb_enable_commit_ts_order_check #57313

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/sessionctx/variable/session.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -955,6 +955,9 @@ type SessionVars struct {
// Value set to `false` means never use mpp.
allowMPPExecution bool

// EnableCommitTSOrderCheck enables checking order of commit_ts for transactions within a session.
EnableCommitTSOrderCheck bool

// allowTiFlashCop means if we must use mpp way to execute query.
// Default value is `false`, means to be determined by the optimizer.
// Value set to `true` means we may fall back to TiFlash cop if possible.
Expand Down Expand Up @@ -2252,6 +2255,7 @@ func NewSessionVars(hctx HookContext) *SessionVars {
vars.DMLBatchSize = DefDMLBatchSize
vars.AllowBatchCop = DefTiDBAllowBatchCop
vars.allowMPPExecution = DefTiDBAllowMPPExecution
vars.EnableCommitTSOrderCheck = DefTiDBEnableCommitTSOrderCheck
vars.HashExchangeWithNewCollation = DefTiDBHashExchangeWithNewCollation
vars.enforceMPPExecution = DefTiDBEnforceMPPExecution
vars.TiFlashMaxThreads = DefTiFlashMaxThreads
Expand Down
4 changes: 4 additions & 0 deletions pkg/sessionctx/variable/sysvar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1936,6 +1936,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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

#57305 implements the check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this make sense?

Copy link
Contributor

@cfzjywxk cfzjywxk Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try txn commit hook function. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sgtm

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

return nil
}},
{Scope: ScopeGlobal | ScopeSession, Name: TiFlashFastScan, Type: TypeBool, Value: BoolToOnOff(DefTiFlashFastScan), SetSession: func(s *SessionVars, val string) error {
s.TiFlashFastScan = TiDBOptOn(val)
return nil
Expand Down
13 changes: 13 additions & 0 deletions pkg/sessionctx/variable/sysvar_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -791,6 +791,19 @@ func TestDefaultMemoryDebugModeValue(t *testing.T) {
require.Equal(t, val, "0")
}

func TestEnableCommitTSOrderCheck(t *testing.T) {
sv := GetSysVar(TiDBEnableCommitTSOrderCheck)
require.True(t, sv.HasSessionScope())
require.False(t, sv.HasGlobalScope())

vars := NewSessionVars(nil)
require.Equal(t, false, vars.EnableCommitTSOrderCheck)

err := sv.SetSession(vars, On)
require.NoError(t, err)
require.Equal(t, true, vars.EnableCommitTSOrderCheck)
}

func TestSetTIDBDistributeReorg(t *testing.T) {
vars := NewSessionVars(nil)
mock := NewMockGlobalAccessor4Tests()
Expand Down
5 changes: 5 additions & 0 deletions pkg/sessionctx/variable/tidb_vars.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,10 @@ const (
// Value set to `false` means never use mpp.
TiDBAllowMPPExecution = "tidb_allow_mpp"

// TiDBEnableCommitTSOrderCheck enables checking the order of commit_ts for transactions within a session.
// Default value is `false`, means not to check the order of commit_ts within a session.
TiDBEnableCommitTSOrderCheck = "tidb_enable_commit_ts_order_check"

// TiDBAllowTiFlashCop means we only use MPP mode to query data.
// Default value is `true`, means to be determined by the optimizer.
// Value set to `false` means we may fall back to TiFlash cop plan if possible.
Expand Down Expand Up @@ -1309,6 +1313,7 @@ const (
DefPreSplitRegions = 0
DefBlockEncryptionMode = "aes-128-ecb"
DefTiDBAllowMPPExecution = true
DefTiDBEnableCommitTSOrderCheck = false
DefTiDBAllowTiFlashCop = false
DefTiDBHashExchangeWithNewCollation = true
DefTiDBEnforceMPPExecution = false
Expand Down