docs/design: Infer the System Timezone of a TiDB cluster via TZ environment variable #7656

zhexuany · 2018-09-10T08:07:34Z

What problem does this PR solve?

This PR solves pushing System to TiKV sides which causes performance delegation.

What is changed and how it works?

doc specifies how this change work.

Check List

Tests

No code

Code changes

No code
Side effects

Related changes

zhexuany · 2018-09-10T08:08:10Z

@shenli @coocood

coocood · 2018-09-10T13:21:12Z

docs/adding_tz_env.md


-The positive side of this change is resolving performance delegation issue to avoid any potential time-related calculation. 
+The value of `localStr` should be initialized by method `initLocalStr()`at `TiDB`'s bootstrap stage. `localStr` will be stored in `mysql.tidb` under `system_tz` column. If system timezone can not get valid IANA timezone name from `mysql.tidb`, `UTC` will be used as default timezone name.


There is no system_tz column, we just add a row with VARIABLE_NAME = 'system_tz'

docs/adding_tz_env.md

shenli · 2018-09-10T13:44:19Z

docs/adding_tz_env.md

+
+## 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 then pushes the real name down to `TiKV` rather than `System`.


If the global timezone is set to UTC and the session's timezone is not set explicitly, we will use UTC.

the default timezone name pushed down to TiKV is System when session's timezone is not set

s/evaluates/infers/

shenli · 2018-09-10T13:47:02Z

docs/adding_tz_env.md

@@ -0,0 +1,55 @@
+# Proposal: adding TZ as an environment variable 


Infer the System Timezone of a TiDB cluster via TZ environment variable

shenli · 2018-09-10T13:50:00Z

docs/adding_tz_env.md

+
+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 produce incorrect calculation when it comes to time-related calculation. Just getting `TiDB`'s sytem timezone could be be broken. We need find a way to ensure the uniqueness of global timezone name across multiple `TiDB`'s timezone name.


could be be broken -> could be broken

shenli · 2018-09-10T13:50:20Z

docs/adding_tz_env.md

+
+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` 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 evaluates the path of the soft link of `/etc/localtime`. In addition, an 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.


to evaluates -> to evaluate

an warning -> a warning

If both of them are failed, TiDB will use UTC as timezone name and print a warning log.

shenli · 2018-09-10T13:52:50Z

docs/adding_tz_env.md

+
+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` 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. 


time. Location -> time.Location

Could you please add a link for LoadLocation?

shenli · 2018-09-10T13:53:49Z

docs/adding_tz_env.md

+
+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` 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 evaluates the path of the soft link of `/etc/localtime`. In addition, an 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.


which can be read from TiDB later -> which can be read by TiDB later

shenli · 2018-09-10T13:55:28Z

docs/adding_tz_env.md

+
+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. 


Add a note here: We should mention and emphasise it in the document.

shenli · 2018-09-10T13:56:54Z

docs/adding_tz_env.md

+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. For example, if `/etc/localtime` links to /usr/share/zoneinfo/Asia/Shanghai, then timezone name 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`. 


When should we insert/update the value of system_tz ?

docs/adding_tz_env.md

shenli · 2018-09-11T05:28:15Z

LGTM

shenli · 2018-09-11T05:28:23Z

@coocood PTAL

coocood

LGTM

adding tz env

4738cda

zhexuany added 4 commits September 10, 2018 17:21

change title and context

a8e2c78

fix a wrong word

f3c958b

refine doc

71b643b

refine writing

419a8da

coocood reviewed Sep 10, 2018

View reviewed changes

docs/adding_tz_env.md Outdated Show resolved Hide resolved

zhexuany added 4 commits September 10, 2018 21:36

rename titile

779ad7c

remove empty string

1c9d9c3

correct spelling

0f59b85

refine doc

b610589

zhexuany force-pushed the adding_a_design_doc branch from 83ed476 to b610589 Compare September 10, 2018 13:46

shenli reviewed Sep 10, 2018

View reviewed changes

zhexuany added 2 commits September 10, 2018 21:53

final refine

f53f5dc

rename title

6308b99

zhexuany changed the title ~~docs: adding tz env~~ docs: Infer the System Timezone of a TiDB cluster via TZ environment variable Sep 10, 2018

correct grammar

cabd543

shenli reviewed Sep 10, 2018

View reviewed changes

add link for loadlocation

7a2f6b2

shenli reviewed Sep 10, 2018

View reviewed changes

docs/adding_tz_env.md Outdated Show resolved Hide resolved

addding desc of upgrading process.

f2de5a2

zhexuany changed the title ~~docs: Infer the System Timezone of a TiDB cluster via TZ environment variable~~ docs/design: Infer the System Timezone of a TiDB cluster via TZ environment variable Sep 11, 2018

zhexuany added component/docs priority/P1 The issue has P1 priority. priority/release-blocker This issue blocks a release. Please solve it ASAP. labels Sep 11, 2018

update writing logic

8669dd8

coocood approved these changes Sep 11, 2018

View reviewed changes

Merge branch 'master' into adding_a_design_doc

9c2526e

coocood added the status/LGT2 Indicates that a PR has LGTM 2. label Sep 11, 2018

zhexuany merged commit e0de9ca into pingcap:master Sep 11, 2018

zhexuany deleted the adding_a_design_doc branch September 11, 2018 05:45

zhexuany mentioned this pull request Sep 11, 2018

sql: update time-zone.md pingcap/docs-cn#876

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs/design: Infer the System Timezone of a TiDB cluster via TZ environment variable #7656

docs/design: Infer the System Timezone of a TiDB cluster via TZ environment variable #7656

zhexuany commented Sep 10, 2018

zhexuany commented Sep 10, 2018

coocood Sep 10, 2018

shenli Sep 10, 2018

shenli Sep 10, 2018

shenli Sep 10, 2018

shenli Sep 10, 2018

shenli Sep 10, 2018

shenli Sep 10, 2018

shenli Sep 10, 2018

shenli Sep 10, 2018

shenli Sep 10, 2018

shenli Sep 10, 2018

shenli Sep 10, 2018

shenli Sep 10, 2018

shenli commented Sep 11, 2018

shenli commented Sep 11, 2018

coocood left a comment


		The positive side of this change is resolving performance delegation issue to avoid any potential time-related calculation.
		The value of `localStr` should be initialized by method `initLocalStr()`at `TiDB`'s bootstrap stage. `localStr` will be stored in `mysql.tidb` under `system_tz` column. If system timezone can not get valid IANA timezone name from `mysql.tidb`, `UTC` will be used as default timezone name.


		## 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 then pushes the real name down to `TiKV` rather than `System`.

		@@ -0,0 +1,55 @@
		# Proposal: adding TZ as an environment variable


		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 produce incorrect calculation when it comes to time-related calculation. Just getting `TiDB`'s sytem timezone could be be broken. We need find a way to ensure the uniqueness of global timezone name across multiple `TiDB`'s timezone name.


		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` 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 evaluates the path of the soft link of `/etc/localtime`. In addition, an 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 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` 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.


		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.

docs/design: Infer the System Timezone of a TiDB cluster via TZ environment variable #7656

docs/design: Infer the System Timezone of a TiDB cluster via TZ environment variable #7656

Conversation

zhexuany commented Sep 10, 2018

What problem does this PR solve?

What is changed and how it works?

Check List

zhexuany commented Sep 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shenli commented Sep 11, 2018

shenli commented Sep 11, 2018

coocood left a comment

Choose a reason for hiding this comment