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

*: write system timezone into mysql.tidb in bootstrap stage. #7638

Merged
merged 63 commits into from
Sep 14, 2018
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
63 commits
Select commit Hold shift + click to select a range
ec6a822
get tz name from zoneinfo
zhexuany Sep 7, 2018
40f1caf
remove imports
zhexuany Sep 7, 2018
ad574a4
add comment
zhexuany Sep 7, 2018
e76c224
correct a spelling
zhexuany Sep 7, 2018
a74739c
called once for evalSys
zhexuany Sep 7, 2018
f0502c8
address comments
zhexuany Sep 7, 2018
ed136b9
remove unecessary import
zhexuany Sep 7, 2018
085dc79
Merge branch 'master' into get_tz_name_from_zoneinfo
zhexuany Sep 8, 2018
8ba623f
rewrite code
zhexuany Sep 8, 2018
13c7c1b
Merge branch 'get_tz_name_from_zoneinfo' of github.com:zhexuany/tidb …
zhexuany Sep 8, 2018
e962f82
print localStr out
zhexuany Sep 8, 2018
26f65f7
set time_zone in test
zhexuany Sep 8, 2018
d708186
align tz behavior with golang
zhexuany Sep 8, 2018
ceedc34
when tz is empty use localtime
zhexuany Sep 8, 2018
2adc1e8
adding timeutil package
zhexuany Sep 8, 2018
acc0262
make the use of time.Now in expression correct
zhexuany Sep 8, 2018
709c32c
remoeve unecessay file
zhexuany Sep 8, 2018
350d27d
adding test and debug print
zhexuany Sep 9, 2018
c95e6c3
print tz in initLocalStr
zhexuany Sep 9, 2018
21e9e22
move zone to timeutil
zhexuany Sep 9, 2018
a84c028
remove prev changes
zhexuany Sep 9, 2018
c1658d8
init localstr in bootstrap stage
zhexuany Sep 11, 2018
a517a43
init localstr in bootstrap stage
zhexuany Sep 11, 2018
b5e39b6
load system tz by select
zhexuany Sep 12, 2018
84b6e8f
address comment
zhexuany Sep 12, 2018
dae8a5a
correct test case
zhexuany Sep 12, 2018
5c373bf
fix conflict
zhexuany Sep 12, 2018
2d0aca2
fix ci
zhexuany Sep 12, 2018
e624443
remove shadow err
zhexuany Sep 12, 2018
f3ad3bc
remove err shadow
zhexuany Sep 12, 2018
692d4d5
address comment
zhexuany Sep 13, 2018
b32bdd2
remove extra print
zhexuany Sep 13, 2018
49454d5
add more comment
zhexuany Sep 13, 2018
318c12b
address comments
zhexuany Sep 13, 2018
cb5fd7d
address comments
zhexuany Sep 13, 2018
ec66759
fix build error
zhexuany Sep 13, 2018
a3c3cc4
fix test error
zhexuany Sep 13, 2018
3976eda
load system tz after bootstrap
zhexuany Sep 13, 2018
d522982
address comments
zhexuany Sep 13, 2018
dd2d0c9
move code around to avoid critical section
zhexuany Sep 13, 2018
29679ab
remove callback
zhexuany Sep 13, 2018
733331e
remove extra comment
zhexuany Sep 13, 2018
e9bb73b
polish comment of fn
zhexuany Sep 13, 2018
1d8ddfa
fix ci
zhexuany Sep 13, 2018
bd24bee
make code cleanner
zhexuany Sep 13, 2018
2f8018a
correct set time_zone behavior
zhexuany Sep 13, 2018
7d6ae04
add test
zhexuany Sep 13, 2018
d315b25
address comment
zhexuany Sep 13, 2018
5dc988b
fix comment spelling issue
zhexuany Sep 13, 2018
1f99ca1
use one hook
zhexuany Sep 13, 2018
e4418cf
remove boot
zhexuany Sep 13, 2018
44996c2
add extra space
zhexuany Sep 13, 2018
bd1693e
address comment
zhexuany Sep 14, 2018
fda4813
fix ci
zhexuany Sep 14, 2018
9986492
Merge branch 'master' into get_tz_name_from_zoneinfo
zhexuany Sep 14, 2018
1acd491
move code after checking errLoad
zhexuany Sep 14, 2018
65051bb
Merge branch 'get_tz_name_from_zoneinfo' of github.com:zhexuany/tidb …
zhexuany Sep 14, 2018
a37aeeb
add comment
zhexuany Sep 14, 2018
e20dc07
remove reset chk
zhexuany Sep 14, 2018
74d394a
rename local fn
zhexuany Sep 14, 2018
64a5b2e
fix ci
zhexuany Sep 14, 2018
e77ee43
fix ci
zhexuany Sep 14, 2018
1b53dff
Merge branch 'master' into get_tz_name_from_zoneinfo
zhexuany Sep 14, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions executor/distsql.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,8 @@ func zone(sctx sessionctx.Context) (string, int64) {
_, offset := time.Now().In(loc).Zone()
var name string
name = loc.String()
// when we found name is "Local", we have no chice but push down
Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

scCtx.GetSessionVars().Location() calls timeutil.Local() if timezone of session is nil. timeutil.Local() tried to retrieve IANA timezone name from TZ and /etc/localtime already.
If it still returns a time.Local, then we have no choice but to push down time.Local to tikv.

// "System" to tikv side.
if name == "Local" {
name = "System"
}
Expand Down
9 changes: 5 additions & 4 deletions expression/builtin_time_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
"github.com/pingcap/tidb/util/mock"
"github.com/pingcap/tidb/util/testleak"
"github.com/pingcap/tidb/util/testutil"
"github.com/pingcap/tidb/util/timeutil"
)

func (s *testEvaluatorSuite) TestDate(c *C) {
Expand Down Expand Up @@ -774,7 +775,7 @@ func (s *testEvaluatorSuite) TestNowAndUTCTimestamp(c *C) {
fc functionClass
now func() time.Time
}{
{funcs[ast.Now], func() time.Time { return time.Now() }},
{funcs[ast.Now], func() time.Time { return time.Now().In(s.ctx.GetSessionVars().Location()) }},
{funcs[ast.UTCTimestamp], func() time.Time { return time.Now().UTC() }},
} {
f, err := x.fc.getFunction(s.ctx, s.datumsToConstants(nil))
Expand Down Expand Up @@ -997,7 +998,7 @@ func (s *testEvaluatorSuite) TestSysDate(c *C) {
fc := funcs[ast.Sysdate]

ctx := mock.NewContext()
ctx.GetSessionVars().StmtCtx.TimeZone = time.Local
ctx.GetSessionVars().StmtCtx.TimeZone = timeutil.Local()
timezones := []types.Datum{types.NewDatum(1234), types.NewDatum(0)}
for _, timezone := range timezones {
// sysdate() result is not affected by "timestamp" session variable.
Expand All @@ -1011,7 +1012,7 @@ func (s *testEvaluatorSuite) TestSysDate(c *C) {
c.Assert(n.String(), GreaterEqual, last.Format(types.TimeFormat))
}

last := time.Now()
last := time.Now().In(s.ctx.GetSessionVars().Location())
f, err := fc.getFunction(ctx, s.datumsToConstants(types.MakeDatums(6)))
c.Assert(err, IsNil)
v, err := evalBuiltinFunc(f, chunk.Row{})
Expand Down Expand Up @@ -1156,7 +1157,7 @@ func (s *testEvaluatorSuite) TestCurrentTime(c *C) {
defer testleak.AfterTest(c)()
tfStr := "15:04:05"

last := time.Now()
last := time.Now().In(s.ctx.GetSessionVars().Location())
fc := funcs[ast.CurrentTime]
f, err := fc.getFunction(s.ctx, s.datumsToConstants(types.MakeDatums(nil)))
c.Assert(err, IsNil)
Expand Down
2 changes: 1 addition & 1 deletion expression/helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ func GetTimeValue(ctx sessionctx.Context, v interface{}, tp byte, fsp int) (d ty
}

func getSystemTimestamp(ctx sessionctx.Context) (time.Time, error) {
now := time.Now()
now := time.Now().In(ctx.GetSessionVars().Location())

if ctx == nil {
return now, nil
Expand Down
2 changes: 1 addition & 1 deletion expression/helper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func (s *testExpressionSuite) TestGetTimeValue(c *C) {
Ret interface{}
}{
{"2012-12-12 00:00:00", "2012-12-12 00:00:00"},
{ast.CurrentTimestamp, time.Unix(1234, 0).Format(types.TimeFormat)},
{ast.CurrentTimestamp, time.Unix(1234, 0).In(ctx.GetSessionVars().Location()).Format(types.TimeFormat)},
{types.ZeroDatetimeStr, "0000-00-00 00:00:00"},
{ast.NewValueExpr("2012-12-12 00:00:00"), "2012-12-12 00:00:00"},
{ast.NewValueExpr(int64(0)), "0000-00-00 00:00:00"},
Expand Down
3 changes: 2 additions & 1 deletion sessionctx/variable/session.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"github.com/pingcap/tidb/types"
"github.com/pingcap/tidb/util/auth"
"github.com/pingcap/tidb/util/chunk"
"github.com/pingcap/tidb/util/timeutil"
)

const (
Expand Down Expand Up @@ -426,7 +427,7 @@ func (s *SessionVars) GetNextPreparedStmtID() uint32 {
func (s *SessionVars) Location() *time.Location {
loc := s.TimeZone
if loc == nil {
loc = time.Local
loc = timeutil.Local()
}
return loc
}
Expand Down
50 changes: 3 additions & 47 deletions store/mockstore/mocktikv/cop_handler_dag.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@ package mocktikv

import (
"bytes"
"fmt"
"io"
"sync"
"time"

"github.com/golang/protobuf/proto"
Expand All @@ -37,6 +35,7 @@ import (
"github.com/pingcap/tidb/types"
"github.com/pingcap/tidb/util/codec"
mockpkg "github.com/pingcap/tidb/util/mock"
"github.com/pingcap/tidb/util/timeutil"
"github.com/pingcap/tipb/go-tipb"
"golang.org/x/net/context"
"google.golang.org/grpc"
Expand All @@ -45,50 +44,6 @@ import (

var dummySlice = make([]byte, 0)

// locCache is a simple map with lock. It stores all used timezone during the lifetime of tidb instance.
// Talked with Golang team about whether they can have some forms of cache policy available for programmer,
// they suggests that only programmers knows which one is best for their use case.
// For detail, please refer to: https://github.com/golang/go/issues/26106
type locCache struct {
sync.RWMutex
// locMap stores locations used in past and can be retrieved by a timezone's name.
locMap map[string]*time.Location
}

// init initializes `locCache`.
func init() {
LocCache = &locCache{}
LocCache.locMap = make(map[string]*time.Location)
}

// LocCache is a simple cache policy to improve the performance of 'time.LoadLocation'.
var LocCache *locCache

// getLoc first trying to load location from a cache map. If nothing found in such map, then call
// `time.LocadLocation` to get a timezone location. After trying both way, an error wil be returned
// if valid Location is not found.
func (lm *locCache) getLoc(name string) (*time.Location, error) {
if name == "System" {
name = "Local"
}
lm.RLock()
if v, ok := lm.locMap[name]; ok {
lm.RUnlock()
return v, nil
}

if loc, err := time.LoadLocation(name); err == nil {
lm.RUnlock()
lm.Lock()
lm.locMap[name] = loc
lm.Unlock()
return loc, nil
}

lm.RUnlock()
return nil, fmt.Errorf("invalid name for timezone %s", name)
}

type dagContext struct {
dagReq *tipb.DAGRequest
keyRanges []*coprocessor.KeyRange
Expand Down Expand Up @@ -169,8 +124,9 @@ func (h *rpcHandler) buildDAGExecutor(req *coprocessor.Request) (*dagContext, ex
// timezone offset in seconds east of UTC is used to constructed the timezone.
func constructTimeZone(name string, offset int) (*time.Location, error) {
if name != "" {
return LocCache.getLoc(name)
return timeutil.LoadLocation(name)
}

Copy link
Member

Choose a reason for hiding this comment

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

remove this empty line

return time.FixedZone("", offset), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

When will the code run to this branch and what happen then ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

user runs set time_zone="+8:00"?

Copy link
Contributor

Choose a reason for hiding this comment

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

But the time zone name is not "UTC" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, the name should be "empty".

Copy link
Contributor

Choose a reason for hiding this comment

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

Will TiKV handle that ? @breeswish

}

Expand Down
27 changes: 27 additions & 0 deletions util/logutil/trace.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// Copyright 2018 PingCAP, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// See the License for the specific language governing permissions and
// limitations under the License.

package logutil

import (
"github.com/opentracing/opentracing-go"
"golang.org/x/net/context"
)

// HasSpan return true if context has an active span
func HasSpan(ctx context.Context) bool {
if sp := opentracing.SpanFromContext(ctx); sp != nil {
return true
}
return false
}
134 changes: 134 additions & 0 deletions util/timeutil/time.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
// Copyright 2017 PingCAP, Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

2018

//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// See the License for the specific language governing permissions and
// limitations under the License.

package timeutil

import (
"errors"
"fmt"
"os"
"path/filepath"
"strings"
"sync"
"syscall"
"time"

log "github.com/sirupsen/logrus"
)

// init initializes `locCache`.
func init() {
LocCache = &locCache{}
LocCache.locMap = make(map[string]*time.Location)
}

// LocCache is a simple cache policy to improve the performance of 'time.LoadLocation'.
var LocCache *locCache
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to expose this variable ?

var localStr string
Copy link
Member

Choose a reason for hiding this comment

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

systemTZ

var localOnce sync.Once
var zoneSources = []string{
"/usr/share/zoneinfo/",
"/usr/share/lib/zoneinfo/",
"/usr/lib/locale/TZ/",
}

// locCache is a simple map with lock. It stores all used timezone during the lifetime of tidb instance.
// Talked with Golang team about whether they can have some forms of cache policy available for programmer,
// they suggests that only programmers knows which one is best for their use case.
// For detail, please refer to: https://github.com/golang/go/issues/26106
type locCache struct {
sync.RWMutex
// locMap stores locations used in past and can be retrieved by a timezone's name.
locMap map[string]*time.Location
}

func initLocalStr() {
// consult $TZ to find the time zone to use.
// no $TZ means use the system default /etc/localtime.
// $TZ="" means use UTC.
// $TZ="foo" means use /usr/share/zoneinfo/foo.
zhexuany marked this conversation as resolved.
Show resolved Hide resolved
tz, ok := syscall.Getenv("TZ")
if ok && tz != "" {
for _, source := range zoneSources {
if _, err := os.Stat(source + tz); os.IsExist(err) {
localStr = tz
break
}
}
// } else if tz == "" {
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 will remove it later.

// localStr = "UTC"
} else {
path, err := filepath.EvalSymlinks("/etc/localtime")
if err == nil {
localStr, err = getTZNameFromFileName(path)
if err == nil {
return
}
log.Errorln(err)
}
log.Errorln(err)
}
localStr = "System"
zhexuany marked this conversation as resolved.
Show resolved Hide resolved
}

// getTZNameFromFileName gets IANA timezone name from zoneinfo path.
// TODO It will be refined later. This is just a quick fix.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/TODO/TODO:

func getTZNameFromFileName(path string) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

inferTZNameFromFileName

// phase1 only support read /etc/localtime which is a softlink to zoneinfo file
substr := "zoneinfo"
if strings.Contains(path, substr) {
zhexuany marked this conversation as resolved.
Show resolved Hide resolved
idx := strings.Index(path, substr)
return string(path[idx+len(substr)+1:]), nil
}
return "", errors.New(fmt.Sprintf("path %s is not supported", path))
}

// Local returns time.Local's IANA timezone name.
func Local() *time.Location {
Copy link
Member

Choose a reason for hiding this comment

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

So this may be actually not a local location because it is loaded from system table? Maybe you can rename the function to reduce confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where is the confusion? Could you please be more explicit?

Copy link
Member

Choose a reason for hiding this comment

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

The name is called local, but actually its result might be not local.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please refer to the comment. Local means tidb's cluster global timezone.

localOnce.Do(initLocalStr)
Copy link
Member

Choose a reason for hiding this comment

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

Why not run initLocalStr in init?

loc, err := LoadLocation(localStr)
if err != nil {
return time.Local
}
return loc
}

// getLoc first trying to load location from a cache map. If nothing found in such map, then call
// `time.LocadLocation` to get a timezone location. After trying both way, an error wil be returned
zhexuany marked this conversation as resolved.
Show resolved Hide resolved
// if valid Location is not found.
func (lm *locCache) getLoc(name string) (*time.Location, error) {
if name == "System" {
zhexuany marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Define a constant for "System". Should we consider case insensitive comparison?

return time.Local, nil
zhexuany marked this conversation as resolved.
Show resolved Hide resolved
}
lm.RLock()
if v, ok := lm.locMap[name]; ok {
Copy link
Member

Choose a reason for hiding this comment

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

This would be cleaner.

lm.RLock()
v, ok := lm.locMap[name]
lm.RUlokc()
...

lm.RUnlock()
return v, nil
}

if loc, err := time.LoadLocation(name); err == nil {
lm.RUnlock()
lm.Lock()
lm.locMap[name] = loc
lm.Unlock()
return loc, nil
}

lm.RUnlock()
return nil, fmt.Errorf("invalid name for timezone %s", name)
}

// LoadLocation loads time.Location by IANA timezone time.
func LoadLocation(name string) (*time.Location, error) {
return LocCache.getLoc(name)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the result of LoadLocation("System") ? nil pointer ?

}
13 changes: 13 additions & 0 deletions util/timeutil/time_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package timeutil
zhexuany marked this conversation as resolved.
Show resolved Hide resolved

import (
. "github.com/pingcap/check"
)

type testSuite struct{}

func (s *testSuite) TestgetTZNameFromFileName(c *C) {
tz, err := getTZNameFromFileName("/user/share/zoneinfo/Asia/Shanghai")
c.Assert(err, IsNil)
c.Assert(tz, Equals, "Asia/Shanghai")
}