-
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
executor, store: fixed daylight saving time issue #6823
Changes from 28 commits
47f8e8e
8128b8b
07cd7e7
d37c337
2a0dfb8
249bd55
f9a3452
9f13196
8e9e421
3381e6f
81ca9a9
a5d7460
8725932
f67be51
a21231e
8ef9938
0010bb8
0ac216c
7dc6c84
4df2580
af8642b
7d5fd27
8e79d24
eb92e87
50383ec
0bc91cb
690c644
c64beb5
f3c8a3f
0179c5d
6d0b0e5
70b77dc
23247e8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1230,6 +1230,7 @@ func (b *executorBuilder) buildDelete(v *plan.Delete) Executor { | |
} | ||
|
||
func (b *executorBuilder) buildAnalyzeIndexPushdown(task plan.AnalyzeIndexTask) *AnalyzeIndexExec { | ||
_, offset := zone(b.ctx) | ||
e := &AnalyzeIndexExec{ | ||
ctx: b.ctx, | ||
tblInfo: task.TableInfo, | ||
|
@@ -1239,7 +1240,7 @@ func (b *executorBuilder) buildAnalyzeIndexPushdown(task plan.AnalyzeIndexTask) | |
Tp: tipb.AnalyzeType_TypeIndex, | ||
StartTs: math.MaxUint64, | ||
Flags: statementContextToFlags(b.ctx.GetSessionVars().StmtCtx), | ||
TimeZoneOffset: timeZoneOffset(b.ctx), | ||
TimeZoneOffset: offset, | ||
}, | ||
} | ||
e.analyzePB.IdxReq = &tipb.AnalyzeIndexReq{ | ||
|
@@ -1260,6 +1261,8 @@ func (b *executorBuilder) buildAnalyzeColumnsPushdown(task plan.AnalyzeColumnsTa | |
keepOrder = true | ||
cols = append([]*model.ColumnInfo{task.PKInfo}, cols...) | ||
} | ||
|
||
_, offset := zone(b.ctx) | ||
e := &AnalyzeColumnsExec{ | ||
ctx: b.ctx, | ||
tblInfo: task.TableInfo, | ||
|
@@ -1271,7 +1274,7 @@ func (b *executorBuilder) buildAnalyzeColumnsPushdown(task plan.AnalyzeColumnsTa | |
Tp: tipb.AnalyzeType_TypeColumn, | ||
StartTs: math.MaxUint64, | ||
Flags: statementContextToFlags(b.ctx.GetSessionVars().StmtCtx), | ||
TimeZoneOffset: timeZoneOffset(b.ctx), | ||
TimeZoneOffset: offset, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add TimeZoneName in Analyze request? @lamxTyler There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Already confirmed with him. No need to consider dst in statistics. |
||
}, | ||
} | ||
depth := int32(defaultCMSketchDepth) | ||
|
@@ -1335,7 +1338,7 @@ func constructDistExec(sctx sessionctx.Context, plans []plan.PhysicalPlan) ([]*t | |
func (b *executorBuilder) constructDAGReq(plans []plan.PhysicalPlan) (dagReq *tipb.DAGRequest, streaming bool, err error) { | ||
dagReq = &tipb.DAGRequest{} | ||
dagReq.StartTs = b.getStartTS() | ||
dagReq.TimeZoneOffset = timeZoneOffset(b.ctx) | ||
dagReq.TimeZoneName, dagReq.TimeZoneOffset = zone(b.ctx) | ||
sc := b.ctx.GetSessionVars().StmtCtx | ||
dagReq.Flags = statementContextToFlags(sc) | ||
dagReq.Executors, streaming, err = constructDistExec(b.ctx, plans) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -118,11 +118,20 @@ func closeAll(objs ...Closeable) error { | |
return errors.Trace(err) | ||
} | ||
|
||
// timeZoneOffset returns the local time zone offset in seconds. | ||
func timeZoneOffset(ctx sessionctx.Context) int64 { | ||
loc := ctx.GetSessionVars().GetTimeZone() | ||
// zone returns the current timezone name and timezone offset in seconds. | ||
// In compatible with MySQL, we change `Local` to `System`. | ||
// TODO: Golang team plan to return system timezone name intead of | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to create an issue for this TODO. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DONE |
||
// returning `Local` when `loc` is `time.Local`. We need keep an eye on this. | ||
func zone(sctx sessionctx.Context) (string, int64) { | ||
loc := sctx.GetSessionVars().Location() | ||
_, offset := time.Now().In(loc).Zone() | ||
return int64(offset) | ||
var name string | ||
name = loc.String() | ||
if name == "Local" { | ||
name = "System" | ||
} | ||
|
||
return name, int64(offset) | ||
} | ||
|
||
// statementContextToFlags converts StatementContext to tipb.SelectRequest.Flags. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1928,6 +1928,7 @@ func (s *testSuite) TestTimestampTimeZone(c *C) { | |
r.Check(testkit.Rows("123381351 1734 2014-03-31 08:57:10 127.0.0.1")) // Cover IndexLookupExec | ||
|
||
// For issue https://github.com/pingcap/tidb/issues/3485 | ||
tk.MustExec("set time_zone = 'Asia/Shanghai'") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why should we add this line? Is there any dst issue in the test case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please refer to line 1907. It sets timezone with offset -6. If we want to compare datetime correctly, we have to set timezone as |
||
tk.MustExec("drop table if exists t1") | ||
tk.MustExec(`CREATE TABLE t1 ( | ||
id bigint(20) NOT NULL AUTO_INCREMENT, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -411,8 +411,8 @@ func (s *SessionVars) GetNextPreparedStmtID() uint32 { | |
return s.preparedStmtID | ||
} | ||
|
||
// GetTimeZone returns the value of time_zone session variable. | ||
func (s *SessionVars) GetTimeZone() *time.Location { | ||
// Location returns the value of time_zone session variable. If it is nil, then return time.Local. | ||
func (s *SessionVars) Location() *time.Location { | ||
loc := s.TimeZone | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We may need to rename There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Location is better and also time package uses the same name. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we can do it in another PR, keep this PR focused on the daylight saving time issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Already did - -. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I saw a conflict and to avoid further change I undo my changes and I will file another PR for changing this once this PR gets merged into master. |
||
if loc == nil { | ||
loc = time.Local | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,9 @@ package mocktikv | |
|
||
import ( | ||
"bytes" | ||
"fmt" | ||
"io" | ||
"sync" | ||
"time" | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove this empty line. |
||
"github.com/golang/protobuf/proto" | ||
|
@@ -43,6 +45,50 @@ 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about locationCache? locCache looks like lockCache. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. loc is generally used everywhere in our codebase. Additionally, it is generally used in |
||
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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need read lock There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. This was already fixed. |
||
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, errors.New(fmt.Sprintf("invalid name for timezone %s", name)) | ||
} | ||
|
||
type dagContext struct { | ||
dagReq *tipb.DAGRequest | ||
keyRanges []*coprocessor.KeyRange | ||
|
@@ -100,7 +146,17 @@ func (h *rpcHandler) buildDAGExecutor(req *coprocessor.Request) (*dagContext, ex | |
return nil, nil, nil, errors.Trace(err) | ||
} | ||
sc := flagsToStatementContext(dagReq.Flags) | ||
sc.TimeZone = time.FixedZone("UTC", int(dagReq.TimeZoneOffset)) | ||
|
||
// retrieving timezone by name first. When name is set, it means we need | ||
// consider daylight saving time. If it is not, we can use offset. | ||
if dagReq.TimeZoneName != "" { | ||
if sc.TimeZone, err = LocCache.getLoc(dagReq.TimeZoneName); err != nil { | ||
return nil, nil, nil, errors.Trace(err) | ||
} | ||
dagReq.TimeZoneName = sc.TimeZone.String() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We set |
||
} else { | ||
sc.TimeZone = time.FixedZone("UTC", int(dagReq.TimeZoneOffset)) | ||
} | ||
ctx := &dagContext{ | ||
dagReq: dagReq, | ||
keyRanges: req.Ranges, | ||
|
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.
@lamxTyler For analyse executor, do we need timezone name too?
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.
Do not need.
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.
@lamxTyler Is offset needed in analyzer?
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.
ping @lamxTyler
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.
Seems not needned.