-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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 12 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,11 @@ 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 local time zone offset in seconds. | ||
func zone(ctx sessionctx.Context) (string, int64) { | ||
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. This function should be the place to change 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. Got it. 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 revisit codebase. I does not think Changing "Local" to "System" should only happens when we need push timezone name and timezone offset down to tikv side. |
||
loc := ctx.GetSessionVars().Location() | ||
_, offset := time.Now().In(loc).Zone() | ||
return int64(offset) | ||
return loc.String(), 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 |
---|---|---|
|
@@ -1889,7 +1889,7 @@ func (b *builtinSysDateWithFspSig) evalTime(row types.Row) (d types.Time, isNull | |
return types.Time{}, isNull, errors.Trace(err) | ||
} | ||
|
||
tz := b.ctx.GetSessionVars().GetTimeZone() | ||
tz := b.ctx.GetSessionVars().Location() | ||
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. s/tz/loc/? |
||
now := time.Now().In(tz) | ||
result, err := convertTimeToMysqlTime(now, int(fsp)) | ||
if err != nil { | ||
|
@@ -1911,7 +1911,7 @@ func (b *builtinSysDateWithoutFspSig) Clone() builtinFunc { | |
// evalTime evals SYSDATE(). | ||
// See https://dev.mysql.com/doc/refman/5.7/en/date-and-time-functions.html#function_sysdate | ||
func (b *builtinSysDateWithoutFspSig) evalTime(row types.Row) (d types.Time, isNull bool, err error) { | ||
tz := b.ctx.GetSessionVars().GetTimeZone() | ||
tz := b.ctx.GetSessionVars().Location() | ||
now := time.Now().In(tz) | ||
result, err := convertTimeToMysqlTime(now, 0) | ||
if err != nil { | ||
|
@@ -1947,7 +1947,7 @@ func (b *builtinCurrentDateSig) Clone() builtinFunc { | |
// evalTime evals CURDATE(). | ||
// See https://dev.mysql.com/doc/refman/5.7/en/date-and-time-functions.html#function_curdate | ||
func (b *builtinCurrentDateSig) evalTime(row types.Row) (d types.Time, isNull bool, err error) { | ||
tz := b.ctx.GetSessionVars().GetTimeZone() | ||
tz := b.ctx.GetSessionVars().Location() | ||
year, month, day := time.Now().In(tz).Date() | ||
result := types.Time{ | ||
Time: types.FromDate(year, int(month), day, 0, 0, 0, 0), | ||
|
@@ -2002,7 +2002,7 @@ func (b *builtinCurrentTime0ArgSig) Clone() builtinFunc { | |
} | ||
|
||
func (b *builtinCurrentTime0ArgSig) evalDuration(row types.Row) (types.Duration, bool, error) { | ||
tz := b.ctx.GetSessionVars().GetTimeZone() | ||
tz := b.ctx.GetSessionVars().Location() | ||
dur := time.Now().In(tz).Format(types.TimeFormat) | ||
res, err := types.ParseDuration(dur, types.MinFsp) | ||
if err != nil { | ||
|
@@ -2026,7 +2026,7 @@ func (b *builtinCurrentTime1ArgSig) evalDuration(row types.Row) (types.Duration, | |
if err != nil { | ||
return types.Duration{}, true, errors.Trace(err) | ||
} | ||
tz := b.ctx.GetSessionVars().GetTimeZone() | ||
tz := b.ctx.GetSessionVars().Location() | ||
dur := time.Now().In(tz).Format(types.TimeFSPFormat) | ||
res, err := types.ParseDuration(dur, int(fsp)) | ||
if err != nil { | ||
|
@@ -2310,7 +2310,7 @@ func evalNowWithFsp(ctx sessionctx.Context, fsp int) (types.Time, bool, error) { | |
return types.Time{}, true, errors.Trace(err) | ||
} | ||
|
||
err = result.ConvertTimeZone(time.Local, ctx.GetSessionVars().GetTimeZone()) | ||
err = result.ConvertTimeZone(time.Local, ctx.GetSessionVars().Location()) | ||
if err != nil { | ||
return types.Time{}, true, errors.Trace(err) | ||
} | ||
|
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 |
---|---|---|
|
@@ -16,6 +16,7 @@ package mocktikv | |
import ( | ||
"bytes" | ||
"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 +44,39 @@ import ( | |
|
||
var dummySlice = make([]byte, 0) | ||
|
||
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 | ||
} | ||
|
||
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 | ||
|
||
func (lm *locCache) getLoc(name string) (*time.Location, error) { | ||
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("invalid name for timezone") | ||
} | ||
|
||
type dagContext struct { | ||
dagReq *tipb.DAGRequest | ||
keyRanges []*coprocessor.KeyRange | ||
|
@@ -100,7 +134,21 @@ 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 |
||
if dagReq.TimeZoneName == "Local" { | ||
dagReq.TimeZoneName = "System" | ||
} | ||
|
||
} 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.