executor, store: fixed daylight saving time issue #6823

zhexuany · 2018-06-12T16:39:27Z

Thank you for working on TiDB! Please read TiDB's CONTRIBUTING document BEFORE filing this PR.

What have you changed? (mandatory)

During coprocessor dag task, it first uses timezone name, if non-empty, to get legitimate timezone variable. To achieve this, we need push down such data into tikv which leads to change the logic of building pushdown request. The logic I mentioned mainly resides in executor package.

I change timeZoneOffset to zone and add second return parameter name string. The intentioned of doing this to adopt the convention of time package.

For the same purpose, I change GetTimeZone to Location. As you can see, in time package, timezone was bind to Location.

What are the type of the changes (mandatory)?

Bug fix (non-breaking change which fixes an issue)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

How has this PR been tested (mandatory)?

zhexuany · 2018-06-12T16:41:04Z

executor/builder.go

@@ -1098,7 +1099,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),


@lamxTyler For analyse executor, do we need timezone name too?

Do not need.

@lamxTyler Is offset needed in analyzer?

ping @lamxTyler

Seems not needned.

shenli · 2018-06-12T17:02:46Z

DO NOT leave the description empty
Could you move the vendor updating into a separate commit?

zhexuany · 2018-06-12T18:15:48Z

@shenli I usually created PR via command-line. That is being said, I will fill info later via website.

For second question, I can certainly do this.

shenli · 2018-06-14T06:00:58Z

Please fix the CI and resolve the conflicts.

shenli · 2018-06-19T07:17:45Z

Please fix the CI.

zhexuany · 2018-06-24T05:08:26Z

/run-all-tests

zhexuany · 2018-06-24T06:35:19Z

@shenli I was on a vacation break. Sorry for the delay. Now, CI has been fixed. PTAL

coocood · 2018-06-27T09:26:51Z

@zhexuany
The default location name is Local, can tikv handle this?

zhexuany · 2018-06-28T08:30:06Z

Already checked with engineer from tikv. TiKV does not need handle this.

zhexuany · 2018-06-28T08:31:13Z

time.LoadLocation is super slow because it read zip file from disk. I am diving into this and trying to find an alternative way to improve this.

coocood · 2018-06-28T12:46:15Z

store/mockstore/mocktikv/cop_handler_dag.go

+var LocCache *locCache
+
+func (lm *locCache) getLoc(name string) (*time.Location, error) {
+	if v, ok := lm.locMap[name]; ok {


Need read lock

Yes. This was already fixed.

zz-jason · 2018-07-13T13:27:23Z

expression/builtin_time.go

@@ -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()


zz-jason · 2018-07-13T13:30:48Z

sessionctx/variable/session.go

-// 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


We may need to rename s.TimeZone to s.Location or any other proper names.

Location is better and also time package uses the same name.

Yes, we can do it in another PR, keep this PR focused on the daylight saving time issue.

Already did - -.

I changed to Loc eventually.

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.

zz-jason · 2018-07-13T13:35:40Z

store/tikv/gcworker/gc_worker.go

 		if err != nil {
 			_, err1 := se.Execute(ctx, "ROLLBACK")
 			terror.Log(errors.Trace(err1))
 			return false, errors.Trace(err)
 		}
-		err = w.saveTime(gcLeaderLeaseKey, time.Now().Add(gcWorkerLease), se)
+		fmt.Println("fuck" + time.Now().String())


remove debug log.

Hahahahaha! @zhexuany

I totally understand your mind!

zz-jason

LGTM

shenli · 2018-07-13T16:55:25Z

executor/builder.go

@@ -1272,7 +1275,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,


Should we add TimeZoneName in Analyze request? @lamxTyler

Already confirmed with him. No need to consider dst in statistics.

shenli · 2018-07-13T16:57:37Z

executor/executor_test.go

@@ -1938,6 +1938,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'")


Why should we add this line? Is there any dst issue in the test case?

Please refer to line 1907. It sets timezone with offset -6. If we want to compare datetime correctly, we have to set timezone as Asia/Shanghai.

shenli · 2018-07-13T16:59:28Z

store/mockstore/mocktikv/cop_handler_dag.go

+// 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 {


How about locationCache? locCache looks like lockCache.

loc is generally used everywhere in our codebase. Additionally, it is generally used in time package.

shenli · 2018-07-13T17:01:03Z

store/mockstore/mocktikv/cop_handler_dag.go

+		if sc.TimeZone, err = LocCache.getLoc(dagReq.TimeZoneName); err != nil {
+			return nil, nil, nil, errors.Trace(err)
+		}
+		dagReq.TimeZoneName = sc.TimeZone.String()


Why set dagReq.TimeZoneName ?

We set dagReq.TimeZoneOffset here, so do dagReq.TimeZoneName.

shenli · 2018-07-16T05:59:35Z

store/tikv/gcworker/gc_worker.go

@@ -240,7 +240,7 @@ func (w *GCWorker) leaderTick(ctx context.Context) error {
 	return nil
 }

-// prepare checks required conditions for starting a GC job. It returns a bool
+// prepare checks preconditions for starting a GC job. It returns a bool


Why change this file? Is it related to the dst issue?

No. It has nothing to do with dst issue. I change it mainly becuase GCoworker already has session as its variable so we do not need access such info with passing it as parameter around.

I found this on the way fixing dst, it should be done via another PR. I will be carefully next time.

shenli · 2018-07-16T06:27:47Z

LGTM

shenli · 2018-07-16T06:28:59Z

/run-all-tests

coocood · 2018-07-16T07:25:09Z

@zhexuany Please resolve the conflict.

zhexuany · 2018-07-16T09:07:57Z

@coocood conflict was already resolved.

zhexuany commented Jun 12, 2018

View reviewed changes

zhexuany force-pushed the fix_daylight_saving_time_issue branch from 2fcc3a2 to 4a59ac4 Compare June 12, 2018 16:59

zhexuany force-pushed the fix_daylight_saving_time_issue branch 8 times, most recently from 654c0d9 to 61e72f8 Compare June 17, 2018 08:51

zhexuany added 6 commits June 24, 2018 10:12

set both name and offset

47f8e8e

change method name which adopts go's time method's name convention

8128b8b

remove irrelevant code

07cd7e7

adds datetime test

d37c337

remove serssion parameter from function signature

2a0dfb8

set timezone name before testing

249bd55

zhexuany force-pushed the fix_daylight_saving_time_issue branch from e3d702f to 249bd55 Compare June 24, 2018 04:48

cache location to improve the performance

f9a3452

coocood reviewed Jun 28, 2018

View reviewed changes

zhexuany added 2 commits June 28, 2018 22:11

add read lock

9f13196

polish comments

8e9e421

zhexuany added 2 commits July 13, 2018 21:01

rename passing parameter

8e79d24

rename passing parameter

eb92e87

zhexuany force-pushed the fix_daylight_saving_time_issue branch from c8e3626 to eb92e87 Compare July 13, 2018 13:05

zhexuany added 2 commits July 13, 2018 21:05

remove redundant line

50383ec

fix ci failure

0bc91cb

zz-jason reviewed Jul 13, 2018

View reviewed changes

address reviewer's comment

690c644

zhexuany force-pushed the fix_daylight_saving_time_issue branch from 6573b27 to 690c644 Compare July 13, 2018 14:46

undo renaming

c64beb5

zz-jason reviewed Jul 13, 2018

View reviewed changes

Merge branch 'master' into fix_daylight_saving_time_issue

f3c8a3f

shenli reviewed Jul 13, 2018

View reviewed changes

shenli reviewed Jul 16, 2018

View reviewed changes

Merge branch 'master' into fix_daylight_saving_time_issue

0179c5d

shenli added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 16, 2018

zhexuany added 3 commits July 16, 2018 16:20

Merge branch 'master' into fix_daylight_saving_time_issue

6d0b0e5

fix ci failure

70b77dc

Merge branch 'master' into fix_daylight_saving_time_issue

23247e8

shenli approved these changes Jul 16, 2018

View reviewed changes

zhexuany added status/DNM and removed status/DNM labels Jul 16, 2018

zhexuany merged commit 7c18d24 into pingcap:master Jul 16, 2018

zhexuany deleted the fix_daylight_saving_time_issue branch July 16, 2018 10:15

zhexuany added a commit to zhexuany/tidb that referenced this pull request Aug 7, 2018

executor, store: fixed daylight saving time issue (pingcap#6823)

d27bab6

zhexuany mentioned this pull request Aug 7, 2018

cherry-pick: make tidb being aware of dst. #7303

Closed

executor, store: fixed daylight saving time issue #6823

executor, store: fixed daylight saving time issue #6823

Conversation

zhexuany commented Jun 12, 2018 • edited Loading

What have you changed? (mandatory)

What are the type of the changes (mandatory)?

How has this PR been tested (mandatory)?

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 Jun 12, 2018

zhexuany commented Jun 12, 2018

shenli commented Jun 14, 2018

shenli commented Jun 19, 2018

zhexuany commented Jun 24, 2018

zhexuany commented Jun 24, 2018

coocood commented Jun 27, 2018

zhexuany commented Jun 28, 2018

zhexuany commented Jun 28, 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

zhexuany Jul 13, 2018 • edited Loading

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

zz-jason left a 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

zhexuany Jul 16, 2018 • edited Loading

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 Jul 16, 2018

shenli commented Jul 16, 2018

coocood commented Jul 16, 2018

zhexuany commented Jul 16, 2018

zhexuany commented Jun 12, 2018 •

edited

Loading

zhexuany Jul 13, 2018 •

edited

Loading

zhexuany Jul 16, 2018 •

edited

Loading