*: move config file option tidb_txn_total_size_limit and tidb_txn_entry_size_limit to sysvar #34448

Alkaagr81 · 2022-05-07T08:56:02Z

What problem does this PR solve?

Issue Number: ref #33769

Problem Summary:

The option tidb_txn_total_size_limit and tidb_txn_entry_size_limit has historically been a config option. But based on requirements from cloud & PM it should instead be a sysvar scope Global.

What is changed and how it works?

Remove them from the config list and add them to global sysvars.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

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.

The option `tidb_txn_total_size_limit` and `tidb_txn_entry_size_limit`  has historically been a config option. But based on requirements from cloud & PM it should instead be a sysvar scope Global.

…ze_limit

ti-chi-bot · 2022-05-07T08:56:04Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

morgo
you06

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

…ze_limit

config/config.toml.example

config/config.go

executor/seqtest/seq_executor_test.go

server/server_test.go

…ze_limit

you06

rest LGTM

sessionctx/variable/sysvar.go

morgo · 2022-05-12T19:12:57Z

/run-unit-test

server/server_test.go

executor/seqtest/seq_executor_test.go

config/config_test.go

morgo · 2022-05-13T20:27:43Z

/run-all-tests

morgo · 2022-05-16T15:57:27Z

~~For this one, we can add upgrade (import values from config) in a followup PR. We have some PRs that have already merged that we need to followup with a bootstrap task to upgrade.~~

Nevermind, upgrade task has been added

session/bootstrap.go

bb7133 · 2022-05-17T10:26:51Z

kv/kv.go

@@ -46,9 +46,9 @@ const UnCommitIndexKVFlag byte = '1'
 // Those limits is enforced to make sure the transaction can be well handled by TiKV.
 var (
 	// TxnEntrySizeLimit is limit of single entry size (len(key) + len(value)).
-	TxnEntrySizeLimit uint64 = config.DefTxnEntrySizeLimit
+	TxnEntrySizeLimit = atomic.NewUint64(10485760) //DefTiDBTxnEntrySizeLimit


Why not using DefTiDBTxnEntrySizeLimit directly? BTW 10485760 is not equal to DefTiDBTxnEntrySizeLimit((6 * 1024 * 1024))

Afaik this is due to circular dependencies since sessionctx/variable imports kv. But I agree we can change the starting value to 6M. It will be overwritten in the startup procedure though, as the sysvar cache is populated.

bb7133 · 2022-05-17T10:27:07Z

kv/kv.go

 	// TxnTotalSizeLimit is limit of the sum of all entry size.
-	TxnTotalSizeLimit uint64 = config.DefTxnTotalSizeLimit
+	TxnTotalSizeLimit = atomic.NewUint64(100 * 1024 * 1024) //DefTiDBTxnTotalSizeLimit


bb7133 · 2022-05-17T10:30:56Z

planner/core/planbuilder.go

@@ -2522,7 +2522,7 @@ func (b *PlanBuilder) buildAnalyzeAllIndex(as *ast.AnalyzeTableStmt, opts map[as
 }

 // CMSketchSizeLimit indicates the size limit of CMSketch.
-var CMSketchSizeLimit = kv.TxnEntrySizeLimit / binary.MaxVarintLen32
+var CMSketchSizeLimit = kv.TxnEntrySizeLimit.Load() / binary.MaxVarintLen32


This variable is initialized once and will not be updated with the change of TxnEntrySizeLimit, it could be a risk.

@XuHuaiyu @chrysan Do you have any idea how this should work with the new tidb_entry_size_limit variable?

bb7133 · 2022-05-17T10:37:15Z

/hold

We need to verify the change related to CMSketchSizeLimit.

ti-chi-bot · 2022-05-31T21:31:30Z

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

codecov · 2023-07-22T09:50:09Z

Codecov Report

Merging #34448 (1eaae25) into master (9f7813c) will decrease coverage by 0.0067%.
Report is 4944 commits behind head on master.
The diff coverage is 26.3157%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #34448        +/-   ##
================================================
- Coverage   62.8182%   62.8116%   -0.0067%     
================================================
  Files           850        850                
  Lines        276679     276686         +7     
================================================
- Hits         173805     173791        -14     
- Misses        89080      89098        +18     
- Partials      13794      13797         +3

ti-chi-bot · 2024-04-10T10:52:44Z

@Alkaagr81: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
idc-jenkins-ci-tidb/build	`1eaae25`	link	true	`/test build`
idc-jenkins-ci-tidb/check_dev	`1eaae25`	link	true	`/test check-dev`
idc-jenkins-ci-tidb/mysql-test	`1eaae25`	link	true	`/test mysql-test`
idc-jenkins-ci-tidb/check_dev_2	`1eaae25`	link	true	`/test check-dev2`
pull-mysql-client-test	`1eaae25`	link	true	`/test pull-mysql-client-test`
pull-integration-ddl-test	`1eaae25`	link	true	`/test pull-integration-ddl-test`
idc-jenkins-ci-tidb/unit-test	`1eaae25`	link	true	`/test unit-test`
pull-br-integration-test	`1eaae25`	link	true	`/test pull-br-integration-test`
pull-lightning-integration-test	`1eaae25`	link	true	`/test pull-lightning-integration-test`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

Alkaagr81 added 5 commits May 5, 2022 11:43

chnages done

98031ca

Merge remote-tracking branch 'upstream/master' into tidb_txn_total_si…

a3705a3

…ze_limit

Merge remote-tracking branch 'upstream/master' into tidb_txn_total_si…

2c43e9b

…ze_limit

*: move config file option tidb_txn_total_size_limit to sysvar

68b2584

txn entry size limit changes done

b30e306

Alkaagr81 requested a review from a team as a code owner May 7, 2022 08:56

ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 7, 2022

Merge remote-tracking branch 'upstream/master' into tidb_txn_total_si…

19779f5

…ze_limit

morgo self-requested a review May 10, 2022 16:47

morgo reviewed May 10, 2022

View reviewed changes

config/config.toml.example Outdated Show resolved Hide resolved

config/config.toml.example Outdated Show resolved Hide resolved

Conflict resolved

80193ad

Alkaagr81 requested a review from morgo May 10, 2022 18:35

morgo mentioned this pull request May 11, 2022

Convert several config options to Global sysvars #33769

Open

morgo requested review from you06, sticnarf and cfzjywxk May 11, 2022 16:21

morgo reviewed May 11, 2022

View reviewed changes

config/config.go Outdated Show resolved Hide resolved

executor/seqtest/seq_executor_test.go Outdated Show resolved Hide resolved

server/server_test.go Outdated Show resolved Hide resolved

Merge remote-tracking branch 'upstream/master' into tidb_txn_total_si…

461dc3b

…ze_limit

you06 reviewed May 12, 2022

View reviewed changes

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

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

Changes done for min value and max value

077891d

Alkaagr81 requested a review from morgo May 12, 2022 18:38

Default value change

0551816

morgo reviewed May 12, 2022

View reviewed changes

server/server_test.go Show resolved Hide resolved

morgo reviewed May 12, 2022

View reviewed changes

executor/seqtest/seq_executor_test.go Show resolved Hide resolved

default value set in end of the function

069cd6e

morgo reviewed May 12, 2022

View reviewed changes

config/config_test.go Outdated Show resolved Hide resolved

Changes done

997a08c

morgo approved these changes May 13, 2022

View reviewed changes

ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 13, 2022

Alkaagr81 added 2 commits May 15, 2022 09:45

Changes done

e755760

Merge

d7dae42

morgo requested a review from you06 May 16, 2022 15:52

Alkaagr81 added 2 commits May 16, 2022 10:17

Merge

a5d5714

Changes done for upgrade

2c208f5

morgo reviewed May 16, 2022

View reviewed changes

session/bootstrap.go Outdated Show resolved Hide resolved

Alkaagr81 added 2 commits May 16, 2022 15:18

Changes done for upgrade

54ecf18

Changes done

c31e315

you06 approved these changes May 17, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 17, 2022

bb7133 reviewed May 17, 2022

View reviewed changes

Alkaagr81 added 4 commits May 18, 2022 10:46

Conflict resolved

b26b43e

Conflict resolved

e846dec

Conflict resolved

c46ccf7

Conflict resolved

1eaae25

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2022

ti-chi-bot bot deleted a comment from ti-chi-bot Dec 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: move config file option tidb_txn_total_size_limit and tidb_txn_entry_size_limit to sysvar #34448

*: move config file option tidb_txn_total_size_limit and tidb_txn_entry_size_limit to sysvar #34448

Alkaagr81 commented May 7, 2022

ti-chi-bot commented May 7, 2022 •

edited

Loading

you06 left a comment

morgo commented May 12, 2022

morgo commented May 13, 2022

morgo commented May 16, 2022 •

edited

Loading

bb7133 May 17, 2022

morgo May 17, 2022

bb7133 May 17, 2022

bb7133 May 17, 2022

bb7133 commented May 17, 2022

ti-chi-bot commented May 31, 2022

codecov bot commented Jul 22, 2023 •

edited

Loading

ti-chi-bot bot commented Apr 10, 2024

*: move config file option tidb_txn_total_size_limit and tidb_txn_entry_size_limit to sysvar #34448

Are you sure you want to change the base?

*: move config file option tidb_txn_total_size_limit and tidb_txn_entry_size_limit to sysvar #34448

Conversation

Alkaagr81 commented May 7, 2022

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented May 7, 2022 • edited Loading

you06 left a comment

Choose a reason for hiding this comment

morgo commented May 12, 2022

morgo commented May 13, 2022

morgo commented May 16, 2022 • edited Loading

bb7133 May 17, 2022

Choose a reason for hiding this comment

morgo May 17, 2022

Choose a reason for hiding this comment

bb7133 May 17, 2022

Choose a reason for hiding this comment

bb7133 May 17, 2022

Choose a reason for hiding this comment

bb7133 commented May 17, 2022

ti-chi-bot commented May 31, 2022

codecov bot commented Jul 22, 2023 • edited Loading

Codecov Report

ti-chi-bot bot commented Apr 10, 2024

ti-chi-bot commented May 7, 2022 •

edited

Loading

morgo commented May 16, 2022 •

edited

Loading

codecov bot commented Jul 22, 2023 •

edited

Loading