sessionctx/variable: set DEFAULT_VALUE for 'tidb_txn_mode' #48500

highpon · 2023-11-09T18:26:59Z

What problem does this PR solve?

Issue Number: close #48492

Problem Summary:The documentation currently states that the default value for tidb_txn_mode is pessimistic, however there is no default value set for tidb_txn_mode. This PR fixes this problem and sets the default value of tidb_txn_mode to pessimistic.

What is changed and how it works?

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 · 2023-11-09T18:27:10Z

Hi @highpon. 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 · 2023-11-09T18:27:20Z

Hi @highpon. 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/test-infra repository.

pkg/infoschema/infoschema_test.go

codecov · 2023-11-10T03:00:24Z

Codecov Report

Merging #48500 (4f170b9) into master (c766530) will increase coverage by 0.2072%.
Report is 8 commits behind head on master.
The diff coverage is 11.1111%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #48500        +/-   ##
================================================
+ Coverage   70.9340%   71.1412%   +0.2072%     
================================================
  Files          1368       1433        +65     
  Lines        395342     417433     +22091     
================================================
+ Hits         280432     296967     +16535     
- Misses        95256     101680      +6424     
+ Partials      19654      18786       -868

Flag	Coverage Δ
integration	`43.8916% <11.1111%> (?)`
unit	`70.9339% <ø> (-0.0001%)`	⬇️

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

Components	Coverage Δ
dumpling	`53.9663% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`46.8968% <ø> (-6.0626%)`	⬇️

lcwangchao · 2023-11-10T08:02:35Z

@cfzjywxk PTAL

you06 · 2023-11-13T04:55:42Z

The important reason we leave the default value of variable tidb_txn_mode empty is for compatibility when upgrading.

When the value of tidb_txn_mode is optimistic or "" or there is no such variable in versions <= v3.0.7, the upgraded cluster should keep the global variable optimistic.
When the value of tidb_txn_mode is pessimistic, upgraded cluster can use pessimistic as global variable.

If we change the default value of tidb_txn_mode, the above requirement should be satisfied.

* `tidb_txn_mode` is introduced in `version32`, see https://github.com/pingcap/tidb/blob/96c6ea1ca9a89375d951d0049d108c05a95723a5/session/bootstrap.go#L333 * The v3.0.8 is `version38`, see https://github.com/pingcap/tidb/blob/v3.0.8/session/bootstrap.go#L363
So I think we need to:

Set global value of tidb_txn_mode to optimistic in upgradeToVer32

Set global value of tidb_txn_mode to optimistic if tidb_txn_mode == "" in upgradeToVer38

highpon · 2023-11-13T15:35:45Z

PR update!

you06

I tested the the suggested version upgradeToVer38, it works.

For upgradeToVer32, I cannot start v2 cluster with tiup, it's not easy to test :(

pkg/session/bootstrap.go

highpon · 2023-11-14T18:00:23Z

@you06
Thanks for you review!
PR updated

pkg/sessionctx/variable/sysvar.go

you06 · 2023-11-15T05:03:47Z

@highpon sorry for the idea of seetting variables linside upgradeToVer32 and upgradeToVer38, this won't work if we upgrade to a version without this patch before upgrading it to the latest version.

e.g.

upgrade from v3.0.0 to latest version, this editted version of upgradeToVer38 will be executed, fine.
upgrade from v3.0.0 to v4.0.0, then upgrade from v4.0.0 to latest version, in this case, old version of upgradeToVer38 is executed when upgrading to v4.0.0, then our grading logic is missing.

I'm discussing with @Defined2014 to find out a compatibility solution.

you06 · 2023-11-15T07:59:39Z

Because in early versions, tidb_txn_mode is not a global variable, if we upgrade from that version, the correspond record will be missing in mysql.global_variables table, then the new default value will be used, which breaks the upgrade compatibility.

So our strategy can be:

For versions tidb_txn_mode is not a global variable, generate "optimistic" value for it.
For versions tidb_txn_mode is a global variable, keep the global value.
For versions >= v3.0.8, the global value is set to "pessimistic" in set global variable tidb_txn_mode to pessimistic during bootstrap. #14154, keep it.

So we need to execute INSERT HIGH_PRIORITY IGNORE INTO mysql.GLOBAL_VARIABLES VALUES("tidb_txn_mode", "optimistic") in the latest upgradeToVerxxx function(you can add a new one).

highpon · 2023-11-19T20:51:18Z

Thanks for you review.
PR updated!

Defined2014 · 2023-11-20T01:59:05Z

/retest

pkg/session/bootstrap.go

you06 · 2023-12-04T01:13:59Z

The default configuration changes should also follow relevant procedures

I think we didn't change the default value, just correct it, but I'll request reviews from PM.

you06

LGTM, sorry for late review, because I missed the notice from GitHub.
Can you solve the conflict so we can merge this PR?
@benmaoer PTAL

ti-chi-bot · 2023-12-22T13:47:36Z

[LGTM Timeline notifier]

Timeline:

2023-11-20 05:15:29.743168581 +0000 UTC m=+208558.408394773: ☑️ agreed by Defined2014.
2023-12-22 13:47:35.609617103 +0000 UTC m=+1227946.646844032: ☑️ agreed by you06.

pkg/session/bootstrap.go

highpon · 2023-12-29T07:53:15Z

@Defined2014
Sorry for the late reply.
Thank you for fixing the conflict.

Defined2014 · 2023-12-29T07:54:41Z

@Defined2014 Sorry for the late reply. Thank you for fixing the conflict.

You are welcome, I will ask PM to review this PR later.

benmaoer

LGTM

ti-chi-bot · 2023-12-30T01:59:14Z

@benmaoer: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

LGTM

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.

Defined2014 · 2024-01-02T01:50:53Z

/unhold

easonn7 · 2024-01-02T01:51:00Z

/approve

The default value of the tidb_txn_mode variable has been changed from "null" to "pessimistic," enhancing the clarity of this variable's meaning and ensuring more consistent behavior.

ti-chi-bot · 2024-01-02T01:51:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benmaoer, Defined2014, easonn7, 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~~ [Defined2014,you06]
~~pkg/session/OWNERS~~ [easonn7]
~~pkg/sessionctx/variable/OWNERS~~ [easonn7]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Defined2014 · 2024-01-02T02:01:16Z

/hold

Defined2014 · 2024-01-02T02:06:26Z

/unhold

ti-chi-bot bot added do-not-merge/needs-triage-completed release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 9, 2023

ti-chi-bot bot added the needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. label Nov 9, 2023

highpon force-pushed the fix/48492 branch from f0cbd97 to 77a1f11 Compare November 9, 2023 18:37

Defined2014 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 10, 2023

Defined2014 reviewed Nov 10, 2023

View reviewed changes

pkg/infoschema/infoschema_test.go Outdated Show resolved Hide resolved

lcwangchao requested a review from cfzjywxk November 10, 2023 08:02

Defined2014 requested a review from you06 November 10, 2023 08:10

ti-chi-bot bot removed the do-not-merge/needs-triage-completed label Nov 11, 2023

highpon requested a review from Defined2014 November 11, 2023 09:47

you06 reviewed Nov 14, 2023

View reviewed changes

pkg/session/bootstrap.go Outdated Show resolved Hide resolved

pkg/session/bootstrap.go Outdated Show resolved Hide resolved

Defined2014 reviewed Nov 15, 2023

View reviewed changes

pkg/sessionctx/variable/sysvar.go Outdated Show resolved Hide resolved

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 19, 2023

highpon requested a review from you06 November 19, 2023 20:51

Defined2014 reviewed Nov 20, 2023

View reviewed changes

pkg/session/bootstrap.go Outdated Show resolved Hide resolved

ti-chi-bot bot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 20, 2023

ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 8, 2023

you06 approved these changes Dec 22, 2023

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Dec 22, 2023

Merge branch 'master' into fix/48492

6551e7a

Defined2014 reviewed Dec 29, 2023

View reviewed changes

pkg/session/bootstrap.go Outdated Show resolved Hide resolved

Apply suggestions from code review

84b24c8

Defined2014 reviewed Dec 29, 2023

View reviewed changes

pkg/session/bootstrap.go Outdated Show resolved Hide resolved

Apply suggestions from code review

edaf346

Defined2014 reviewed Dec 29, 2023

View reviewed changes

pkg/session/bootstrap.go Show resolved Hide resolved

Defined2014 added 2 commits December 29, 2023 15:02

Update pkg/session/bootstrap.go

015fa59

Update bootstrap.go

4f170b9

benmaoer approved these changes Dec 30, 2023

View reviewed changes

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 2, 2024

ti-chi-bot bot added the approved label Jan 2, 2024

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 2, 2024

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 2, 2024

ti-chi-bot bot merged commit 7af117d into pingcap:master Jan 2, 2024
17 of 18 checks passed

jiyfhust mentioned this pull request Jan 2, 2024

variable: fix information_schema.VARIABLES_INFO DEFAULT_VALUE not right problem #49524

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sessionctx/variable: set DEFAULT_VALUE for 'tidb_txn_mode' #48500

sessionctx/variable: set DEFAULT_VALUE for 'tidb_txn_mode' #48500

highpon commented Nov 9, 2023

ti-chi-bot bot commented Nov 9, 2023

tiprow bot commented Nov 9, 2023

codecov bot commented Nov 10, 2023 •

edited

Loading

lcwangchao commented Nov 10, 2023

you06 commented Nov 13, 2023 •

edited

Loading

highpon commented Nov 13, 2023

you06 left a comment

highpon commented Nov 14, 2023

you06 commented Nov 15, 2023

you06 commented Nov 15, 2023

highpon commented Nov 19, 2023 •

edited

Loading

Defined2014 commented Nov 20, 2023

you06 commented Dec 4, 2023

you06 left a comment

ti-chi-bot bot commented Dec 22, 2023

highpon commented Dec 29, 2023

Defined2014 commented Dec 29, 2023

benmaoer left a comment

ti-chi-bot bot commented Dec 30, 2023

Defined2014 commented Jan 2, 2024

easonn7 commented Jan 2, 2024

ti-chi-bot bot commented Jan 2, 2024

Defined2014 commented Jan 2, 2024

Defined2014 commented Jan 2, 2024

sessionctx/variable: set DEFAULT_VALUE for 'tidb_txn_mode' #48500

sessionctx/variable: set DEFAULT_VALUE for 'tidb_txn_mode' #48500

Conversation

highpon commented Nov 9, 2023

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot bot commented Nov 9, 2023

tiprow bot commented Nov 9, 2023

codecov bot commented Nov 10, 2023 • edited Loading

Codecov Report

lcwangchao commented Nov 10, 2023

you06 commented Nov 13, 2023 • edited Loading

highpon commented Nov 13, 2023

you06 left a comment

Choose a reason for hiding this comment

highpon commented Nov 14, 2023

you06 commented Nov 15, 2023

you06 commented Nov 15, 2023

highpon commented Nov 19, 2023 • edited Loading

Defined2014 commented Nov 20, 2023

you06 commented Dec 4, 2023

you06 left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Dec 22, 2023

[LGTM Timeline notifier]

highpon commented Dec 29, 2023

Defined2014 commented Dec 29, 2023

benmaoer left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Dec 30, 2023

Defined2014 commented Jan 2, 2024

easonn7 commented Jan 2, 2024

ti-chi-bot bot commented Jan 2, 2024

Defined2014 commented Jan 2, 2024

Defined2014 commented Jan 2, 2024

codecov bot commented Nov 10, 2023 •

edited

Loading

you06 commented Nov 13, 2023 •

edited

Loading

highpon commented Nov 19, 2023 •

edited

Loading