server, session, variable: normalize tidb_txn_mode values #25009
Conversation
ti-chi-bot
added
the
size/M
| if v.Name == variable.TiDBTxnMode && config.GetGlobalConfig().Store == "tikv" { | ||
| vVal = "pessimistic" | ||
| if v.Name == variable.TiDBTxnMode && config.GetGlobalConfig().Store != "tikv" { | ||
| vVal = "OPTIMISTIC" |
There was a problem hiding this comment.
Maybe use DefTiDBTxnMode directly, is it possible?
There was a problem hiding this comment.
DefTiDBTxnMode is now pessimistic, but here we are saying if the store is not tikv use OPTIMISTIC. I can use ast.Optimistic if that helps?
There was a problem hiding this comment.
Oops, I did not notice that... No, just keep it unchanged. Do we need to change the document or inform the transaction guys?
There was a problem hiding this comment.
Yes, the user documents should change. But I am hoping we can merge the automated docs generation and incorporate it here: pingcap/docs#5720
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
ti-chi-bot
added
the
needs-rebase
ti-chi-bot
added
needs-rebase
ti-chi-bot
removed
the
needs-rebase
To clarify: it's not a default change unless the user upgrades directly from This PR is trying to reduce the confusion which is caused by having 3 states; at the cost of an unlikely (and should not be supported) upgrade path. |
1 similar comment
To clarify: it's not a default change unless the user upgrades directly from This PR is trying to reduce the confusion which is caused by having 3 states; at the cost of an unlikely (and should not be supported) upgrade path. |
@morgo |
OK let me know :-) |
|
@morgo: PR needs rebase. 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
added
the
needs-rebase
What problem does this PR solve?
Problem Summary:
Previously the sysvar
tidb_txn_modepermitted three values:This improves the normalization of system variables to be in upper case (consistent with most other ENUM variables), and the validation function converts "" to
OPTIMISTIC.Because the state is cleaned up internally, the helper function
GetReadableTxnMode()is no longer required.Additionally, the default is changed to
PESSIMISTICso that it can support the use case ofSET tidb_txn_mode = DEFAULT(users effectively think pessmistic is the default, but from a code point of view it's not). To support this, the bootstrap logic of changing the value of tidb_txn_mode toPESSIMISTICif the store is tikv is inverted to be if the store!=tikv.Note: This means there is a slight behavior change: Upgrading from 2.1 directly to master will change the txn-mode from optimistic to pessimistic. But if you step through any version in between it will stay as optimistic. I think this is easy enough to describe in the docs, but long term it might be better to say that direct upgrades that skip more than 1 major digit are not supported. That way we can avoid the excessive language in docs describing edge-cases (which hurt readability).
What is changed and how it works?
What's Changed:
Code cleanup, mostly function neutral to users. Improves usability because the behavior of "" is confusing to me, and likely to other users.
Related changes
pingcap/docs/pingcap/docs-cn:Check List
Tests
Side effects
Release note
tidb_txn_modesystem variable has been improved. It is recommended to set the value toOPTIMISTICorPESSIMISTIC, and no longer the special empty string ('') value.