-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
docs/design: Infer the System Timezone of a TiDB cluster via TZ environment variable #7656
Merged
Merged
Changes from 13 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
4738cda
adding tz env
zhexuany a8e2c78
change title and context
zhexuany f3c958b
fix a wrong word
zhexuany 71b643b
refine doc
zhexuany 419a8da
refine writing
zhexuany 779ad7c
rename titile
zhexuany 1c9d9c3
remove empty string
zhexuany 0f59b85
correct spelling
zhexuany b610589
refine doc
zhexuany f53f5dc
final refine
zhexuany 6308b99
rename title
zhexuany cabd543
correct grammar
zhexuany 7a2f6b2
add link for loadlocation
zhexuany f2de5a2
addding desc of upgrading process.
zhexuany 8669dd8
update writing logic
zhexuany 9c2526e
Merge branch 'master' into adding_a_design_doc
coocood File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
# Proposal: Infer the System Timezone of a TiDB cluster via TZ environment variable | ||
|
||
- Author(s): [Zhexuan Yang](www.github.com/zhexuany) | ||
- Last updated: 2018/09/09 | ||
- Discussion at: Not applicable | ||
|
||
## Abstract | ||
|
||
When it comes to time-related calculation, it is hard for the distributed system. This proposal tries to resolve two problems: 1. timezone may be inconsistent across multiple `TiDB` instances, 2. performance degradation casued by pushing `System` down to `TiKV`. The impact of this proposal is changing the way of `TiDB` inferring system's timezone name. Before this proposal, the default timezone name pushed down to TiKV is `System` when session's timezone is not set. After this, TiDB evaluates system's timezone name via `TZ` environment variable and the path of the soft link of `/etc/localtime`. If both of them are failed, `TiDB` then push `UTC` to `TiKV`. | ||
|
||
## Background | ||
|
||
After we solved the daylight saving time issue, we found the performance degradation of TiKV side. Thanks for the investigation done by engineers from TiKV. The root cause of such performance degradation is that TiKV infers `System` timezone name via a third party lib, which calls a syscall and costs a lot. In our internal benchmark system, after [this PR](https://github.com/pingcap/tidb/pull/6823), our codebase is 1000 times slower than before. We have to address this. | ||
|
||
Another problem needs also to be addressed is the potentially incosistent timezone name across multiple `TiDB` instances. `TiDB` instances may reside at different timezone which could cause incorrect calculation when it comes to time-related calculation. Just getting `TiDB`'s sytem timezone could be broken. We need find a way to ensure the uniqueness of global timezone name across multiple `TiDB`'s timezone name and also to leverage to resolve the performance degradation. | ||
|
||
## Proposal | ||
|
||
Firstly, we need to introduce the `TZ` environment. In POSIX system, the value of `TZ` variable can be one of the following three formats. A detailed description can be found in [this link](http://www.gnu.org/software/libc/manual/html_node/TZ-Variable.html) | ||
|
||
* std offset | ||
* std offset dst [offset], start[/time], end[/time] | ||
* :characters | ||
|
||
The std means the IANA timezone name; the offset means timezone offset; the dst indicates the leading timezone having daylight saving time. | ||
|
||
In our case, which means both `TiDB` and `TiKV`, we need care the first and third formats. For answering why we do not need the second format, we need to review how Golang evaluates timezone. In `time` package, the method [LoadLocation](https://golang.org/pkg/time/#LoadLocation) reads tzData from pre-specified sources(directories may contain tzData) and then builds `time.Location` from such tzData which already contains daylight saving time information. | ||
|
||
In this proposal, we suggest setting `TZ` to a valid IANA timezone name which can be read from `TiDB` later. If `TiDB` can't get `TZ` or the supply of `TZ` is invalid, `TiDB` just falls back to evaluate the path of the soft link of `/etc/localtime`. In addition, a warning message telling the user you should set `TZ` properly will be printed. Setting `TZ` can be done in our `tidb-ansible` project, it is also can be done at user side by `export TZ="Asia/Shanghai"`. If both of them are failed, `TiDB` will use `UTC` as timezone name. | ||
|
||
The positive side of this change is resolving performance degradation issue and ensuring the uniqueness of gloabl timezone name in multiple `TiDB` instances. | ||
|
||
The negative side is just adding a config item which is a very small matter and the user probably does not care it if we can take care of it and more importantly guarantee the correctness. | ||
|
||
|
||
## Rationale | ||
|
||
We tried to read system timezone name by checking the path of the soft link of `/etc/localtime` but, sadly, failed at a corner case. The failed case is docker. In docker image, it copies the real timezone file and links to `/usr/share/zoneinfo/utc`. The timezone data is correct but the path is not. Regarding of `UTC`, Golang just returns `UTC` instance and will not further read tzdata from sources. This leads to a fallback solution. When we cannot evaluate from the path, we fall back to `UTC`. | ||
|
||
## Compatibility | ||
|
||
It does not have compatibility issue as long as the user deploys by `tidb-ansible`. We may mention this in our release-node and the message printed before tidb quits, which must be easy to understand. | ||
|
||
|
||
## Implementation | ||
|
||
The implementation is relatively easy. We just get `TZ` environment from system and check whether it is valid or not. If it is invalid, TiDB evaluates the path of soft link of `/etc/localtime`. In addition, a warning message needs to be printed indicating user has to set `TZ` variable properly. For example, if `/etc/localtime` links to `/usr/share/zoneinfo/Asia/Shanghai`, then timezone name `TiDB` gets should be `Asia/Shangahi`. | ||
|
||
In order to ensure the uniqueness of global timezone across multiple `TiDB` instances, we need to write timezone name into `mysql.tidb` under column `system_tz`. This cached value can be read once `TiDB` finishs its bootstrap stage. A method `loadLocalStr` can do this job. | ||
zhexuany marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
## Open issues (if applicable) | ||
|
||
PR of this proposal: https://github.com/pingcap/tidb/pull/7638/files | ||
PR of change TZ loading logic of golang: https://github.com/golang/go/pull/27570 |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a note here: We should mention and emphasise it in the document.