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.

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

system-variables: update from generated source #5752

Merged
merged 5 commits into from
Jun 16, 2021

Conversation

morgo
Copy link
Contributor

@morgo morgo commented Jun 3, 2021

What is changed, added or deleted? (Required)

This updates the system variables, generated against the tidb source code. i.e. this PR is derived from #5720 , but the generator source code will be submitted separately.

This allows this PR to potentially be cherry picked and handled like a regular contribution.

Which TiDB version(s) do your changes apply to? (Required)

  • master (the latest development version)
  • v5.1 (TiDB 5.1 versions)
  • v5.0 (TiDB 5.0 versions)
  • v4.0 (TiDB 4.0 versions)
  • v3.1 (TiDB 3.1 versions)
  • v3.0 (TiDB 3.0 versions)
  • v2.1 (TiDB 2.1 versions)

What is the related PR or file link(s)?

  • This PR is translated from:
  • Other reference link(s):

Do your changes match any of the following descriptions?

  • Delete files
  • Change aliases
  • Need modification after applied to another branch
  • Might cause conflicts after applied to another branch

@morgo morgo requested a review from TomShawn June 3, 2021 20:17
@ti-chi-bot ti-chi-bot added missing-translation-status This PR does not have translation status info. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 3, 2021
@TomShawn TomShawn added needs-cherry-pick-release-5.0 require-LGT1 Indicates that the PR requires an LGTM. translation/doing This PR's assignee is translating this PR. labels Jun 4, 2021
@ti-chi-bot ti-chi-bot removed the missing-translation-status This PR does not have translation status info. label Jun 4, 2021
@TomShawn TomShawn added the type/enhancement The issue or PR belongs to an enhancement. label Jun 4, 2021
system-variables.md Outdated Show resolved Hide resolved
system-variables.md Outdated Show resolved Hide resolved
system-variables.md Outdated Show resolved Hide resolved
system-variables.md Outdated Show resolved Hide resolved
system-variables.md Outdated Show resolved Hide resolved
system-variables.md Outdated Show resolved Hide resolved
system-variables.md Outdated Show resolved Hide resolved
system-variables.md Outdated Show resolved Hide resolved
system-variables.md Outdated Show resolved Hide resolved
system-variables.md Outdated Show resolved Hide resolved
system-variables.md Outdated Show resolved Hide resolved
system-variables.md Outdated Show resolved Hide resolved
@@ -643,7 +678,7 @@ For a system upgraded to v5.0 from an earlier version, if you have not modified
### tidb_gc_life_time <span class="version-mark">New in v5.0</span>

- Scope: GLOBAL
- Default: `"10m0s"`
- Default value: 10m0s
Copy link
Contributor

Choose a reason for hiding this comment

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

This value was presented as a "string". If so, I think we can keep the "" quotes. The same for similar cases in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we change this to a string, then the other cases would need to be a string too. It's not that this is incorrect - it's just that it doesn't add any value (all sysvars are actually intepreted as strings). See: https://dev.mysql.com/doc/refman/5.7/en/innodb-parameters.html#sysvar_innodb_flush_method for example on how mysql does it.

system-variables.md Outdated Show resolved Hide resolved
system-variables.md Outdated Show resolved Hide resolved
system-variables.md Outdated Show resolved Hide resolved
@ti-chi-bot ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 8, 2021
@morgo morgo force-pushed the update-sysvars-from-source branch from eb52e17 to 3e6c4c0 Compare June 8, 2021 15:13
@morgo
Copy link
Contributor Author

morgo commented Jun 11, 2021

/cc @dveeden may I ask for a technical review?

This is generated from code if you'd like to take a look at #5720 - but we'll treat it as a my contribution for workflow simplicity.

system-variables.md Show resolved Hide resolved
system-variables.md Outdated Show resolved Hide resolved
- Enables garbage collection for TiKV. Disabling garbage collection will reduce system performance, as old versions of rows will no longer be purged.

### tidb_gc_life_time <span class="version-mark">New in v5.0</span>

- Scope: GLOBAL
- Default: `"10m0s"`
- Default value: `10m0s`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Default value: `10m0s`
- Default value: `10m0s`
- Range: `[10m, 2562047h]`
5.7.25-TiDB-v5.2.0-alpha-16-gf81ef5579-dirty 127.0.0.1:4000  SQL  set global tidb_gc_life_time='10m';
Query OK, 0 rows affected (0.0031 sec)
5.7.25-TiDB-v5.2.0-alpha-16-gf81ef5579-dirty 127.0.0.1:4000  SQL  set global tidb_gc_life_time='9m';
ERROR: 1232 (42000): Incorrect argument type to variable 'tidb_gc_life_time'
5.7.25-TiDB-v5.2.0-alpha-16-gf81ef5579-dirty 127.0.0.1:4000  SQL  set global tidb_gc_life_time='9m59s';
ERROR: 1232 (42000): Incorrect argument type to variable 'tidb_gc_life_time'
5.7.25-TiDB-v5.2.0-alpha-16-gf81ef5579-dirty 127.0.0.1:4000  SQL  set global tidb_gc_life_time='9m60s';
Query OK, 0 rows affected (0.0029 sec)

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'm going to take this up with the team responsible before fixing it. The range here is 292 years, which doesn't really make sense so Golang formats this in minutes with duration.String() method.

I think setting it in code to be a maximum of 1 year is the logical way to handle it.

system-variables.md Outdated Show resolved Hide resolved
- Controls the format version of the newly saved data in the table. In TiDB v4.0, the [new storage row format](https://github.com/pingcap/tidb/blob/master/docs/design/2018-07-19-row-format.md) version `2` is used by default to save new data.
- If you upgrade from a TiDB version earlier than 4.0.0 to 4.0.0, the format version is not changed, and TiDB continues to use the old format of version `1` to write data to the table, which means that **only newly created clusters use the new data format by default**.
- Note that modifying this variable does not affect the old data that has been saved, but applies the corresponding version format only to the newly written data after modifying this variable.

### tidb_scatter_region

- Scope: GLOBAL
- Default value: OFF
- Default value: `OFF`
- By default, Regions are split for a new table when it is being created in TiDB. After this variable is enabled, the newly split Regions are scattered immediately during the execution of the `CREATE TABLE` statement. This applies to the scenario where data need to be written in batches right after the tables are created in batches, because the newly split Regions can be scattered in TiKV beforehand and do not have to wait to be scheduled by PD. To ensure the continuous stability of writing data in batches, the `CREATE TABLE` statement returns success only after the Regions are successfully scattered. This makes the statement's execution time multiple times longer than that when you disable this variable.

### tidb_skip_ascii_check <span class="version-mark">New in v5.0</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we link to the similar setting for UTF-8?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I will do it in a separate PR. For this I'm aiming for parity through an auto-generator.

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • dveeden

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.

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 11, 2021
- This variable is used to set the concurrency of executing the `ANALYZE` statement.
- When the variable is set to a larger value, the execution performance of other queries is affected.

### tidb_capture_plan_baselines <span class="version-mark">New in v4.0</span>

- Scope: SESSION | GLOBAL
- Default value: OFF
- Default value: `OFF`
- This variable is used to control whether to enable the [baseline capturing](/sql-plan-management.md#baseline-capturing) feature. This feature depends on the statement summary, so you need to enable the statement summary before you use baseline capturing.
- After this feature is enabled, the historical SQL statements in the statement summary are traversed periodically, and bindings are automatically created for SQL statements that appear at least twice.

### tidb_check_mb4_value_in_utf8
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should link to tidb_skip_utf8_check and/or explain how these are related?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I will do it in a separate PR. For this I'm aiming for parity through an auto-generator.

@morgo
Copy link
Contributor Author

morgo commented Jun 16, 2021

@TomShawn can we merge this? It will likely start to get merge conflicts.

Copy link
Contributor

@TomShawn TomShawn left a comment

Choose a reason for hiding this comment

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

LGTM

@TomShawn
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 58cd968

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 16, 2021
@ti-chi-bot ti-chi-bot merged commit e1d5394 into pingcap:master Jun 16, 2021
ti-chi-bot pushed a commit to ti-chi-bot/docs that referenced this pull request Jun 16, 2021
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created: #5793.

@morgo morgo deleted the update-sysvars-from-source branch June 16, 2021 02:54
@Liuxiaozhen12
Copy link
Contributor

/remove-translation doing
/translation done

@ti-chi-bot ti-chi-bot added translation/done This PR has been translated from English into Chinese and updated to pingcap/docs-cn in a PR. and removed translation/doing This PR's assignee is translating this PR. labels Jul 1, 2021
@@ -1206,7 +1261,7 @@ This variable is an alias for _transaction_isolation_.
### version

- Scope: NONE
- Default value: 5.7.25-TiDB-(tidb version)
- Default value: `5.7.25-TiDB-`(tidb version)
Copy link
Contributor

@TomShawn TomShawn Jul 5, 2021

Choose a reason for hiding this comment

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

Follow-up: `5.7.25-TiDB-(tidb version)`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
require-LGT1 Indicates that the PR requires an LGTM. requires-followup This PR requires a follow-up task after being merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1. translation/done This PR has been translated from English into Chinese and updated to pingcap/docs-cn in a PR. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants