executor, expression: fix current_timestamp/now not consistent in one stmt like MySQL #11342

cfzjywxk · 2019-07-19T13:16:09Z

What problem does this PR solve?

MySQL will set time to “”time_cache“ structure before doing one command(COM_XXX)
later expresission calculation will use this timestamp as time value

TiDB logic different and default column calculation and now expr calculation will try to get timestamp twice.

more details see jira issue#4391
eg:
TiDB

CREATE TABLE `tt4` (
  `c1` timestamp DEFAULT CURRENT_TIMESTAMP,
  `c2` int(11) DEFAULT NULL,
  `c3` timestamp DEFAULT CURRENT_TIMESTAMP
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin

insert into tt4 set c1 = current_timestamp, c2 = sleep(2);
mysql> select c1 = c3 from tt4;
+---------+
| c1 = c3 |
+---------+
|       0 |
+---------+
1 row in set (0.00 sec)

MySQL

insert into tt4 set c1 = current_timestamp, c2 = sleep(2);
select c1 = c3 from tt4;
+---------+
| c1 = c3 |
+---------+
|       1 |
+---------+
1 row in set (0.00 sec)

remove stmtCtx.sysTs unused variable
cache time value once in stmtCtx.nowTs
refactor nowTs usage, offer Unified function interface for get and reset nowTS. Generally, accessing variable directly is not a good way for further maintenance, refactor these usage code

What is changed and how it works?

Check List

Tests

Unit test

Code changes

Has exported function/method change
Has exported variable/fields change

Side effects

Related changes

Need to cherry-pick to the release branch

cfzjywxk · 2019-07-19T13:16:23Z

@jackysp PTAL

cfzjywxk · 2019-07-19T13:16:45Z

/run-all-tests

codecov · 2019-07-19T13:20:46Z

Codecov Report

Merging #11342 into master will decrease coverage by 0.1875%.
The diff coverage is 86.6666%.

@@               Coverage Diff                @@
##             master     #11342        +/-   ##
================================================
- Coverage   81.4278%   81.2403%   -0.1876%     
================================================
  Files           423        423                
  Lines         90851      90172       -679     
================================================
- Hits          73978      73256       -722     
- Misses        11588      11603        +15     
- Partials       5285       5313        +28

codecov · 2019-07-19T13:20:47Z

Codecov Report

Merging #11342 into master will decrease coverage by 0.0194%.
The diff coverage is 59.1836%.

@@               Coverage Diff                @@
##             master     #11342        +/-   ##
================================================
- Coverage   81.2969%   81.2774%   -0.0195%     
================================================
  Files           423        423                
  Lines         90381      90399        +18     
================================================
- Hits          73477      73474         -3     
- Misses        11598      11609        +11     
- Partials       5306       5316        +10

expression/helper.go

sessionctx/stmtctx/stmtctx.go

cfzjywxk · 2019-07-20T06:26:17Z

/run-all-tests

cfzjywxk · 2019-07-20T06:42:47Z

/run-all-tests

cfzjywxk · 2019-07-20T06:47:17Z

/run-all-tests

cfzjywxk · 2019-07-20T06:51:56Z

@jackysp PTAL
refactor nowTs usage, offer Unified function interface for get and reset nowTS.
there are two call paths to stmt timestamp [current_timestamp/now/date like function calls] and [time type col default value calculations]

jackysp · 2019-07-22T03:57:37Z

sessionctx/stmtctx/stmtctx.go

+}
+
+// ReSetNowTs resetter for nowTs, clear cached time flag
+func (sc *StatementContext) ReSetNowTs() {


Suggested change

func (sc *StatementContext) ReSetNowTs() {

func (sc *StatementContext) ResetNowTs() {

jackysp · 2019-07-22T04:03:13Z

sessionctx/stmtctx/stmtctx.go

@@ -393,6 +409,7 @@ func (sc *StatementContext) ResetForRetry() {
 	sc.mu.Unlock()
 	sc.TableIDs = sc.TableIDs[:0]
 	sc.IndexIDs = sc.IndexIDs[:0]
+	sc.ReSetNowTs()


I think we should not reset when retrying. Transaction retry is an automatic implicit behavior. It is possible that current_timestamp has been returned to the client. If it is reset here, the user may be bothered. For example,

begin; insert xxx current_timestamp; select now(); -- The user might think this value is bigger than the previous insert. commit; -- It is better do not reset current here, if the retry is needed.

zz-jason · 2019-07-23T01:27:10Z

expression/builtin_time.go

-	var nowTs = &b.ctx.GetSessionVars().StmtCtx.NowTs
-	if nowTs.Equal(time.Time{}) {
-		*nowTs = time.Now()
+	nowTs, err := getStmtTimestamp(b.ctx)


would this impact the prepare plan cache which has the NOW() builtin function?

plan cache will not cache now function results, every time execution will recaculate now() function. This change make now/current_time func call same results within one sql statement

jackysp · 2019-07-23T05:47:11Z

sessionctx/stmtctx/stmtctx.go

+}
+
+// ResetNowTs resetter for nowTs, clear cached time flag
+func (sc *StatementContext) ResetNowTs() {


It seems we do not need this function anymore. It is only used in the test.

maybe leave this as it's correspond to newly added Get interface. Future devs will not write "*timeNows = xxx/time.Time{}" like code and forget to reset stmtTimeCached flag

jackysp

rest LGTM

jackysp

LGTM

sessionctx/stmtctx/stmtctx.go

…unction results refactor related statement time related interface and some time function implementations

zz-jason

LGTM

sre-bot · 2019-07-23T09:04:47Z

cherry pick to release-2.1 failed

sre-bot · 2019-07-23T09:06:56Z

cherry pick to release-3.0 failed

…cap#11342) cherry-pick to release-2.1 branch

…cap#11342)

cfzjywxk added type/bug The issue is confirmed as a bug. type/compatibility component/expression sig/execution SIG execution needs-cherry-pick-2.1 labels Jul 19, 2019

cfzjywxk requested review from lysu, jackysp, zz-jason and qw4990 July 19, 2019 13:16

jackysp added the needs-cherry-pick-3.0 label Jul 19, 2019

jackysp reviewed Jul 19, 2019

View reviewed changes

expression/helper.go Outdated Show resolved Hide resolved

jackysp reviewed Jul 19, 2019

View reviewed changes

expression/helper.go Outdated Show resolved Hide resolved

jackysp reviewed Jul 19, 2019

View reviewed changes

sessionctx/stmtctx/stmtctx.go Show resolved Hide resolved

cfzjywxk force-pushed the p4391 branch from 3fa536c to ac54341 Compare July 20, 2019 06:42

jackysp reviewed Jul 22, 2019

View reviewed changes

zz-jason reviewed Jul 23, 2019

View reviewed changes

cfzjywxk requested review from jackysp and zz-jason July 23, 2019 03:28

jackysp reviewed Jul 23, 2019

View reviewed changes

cfzjywxk force-pushed the p4391 branch from f1158ad to 5515694 Compare July 23, 2019 06:45

cfzjywxk requested a review from jackysp July 23, 2019 07:00

jackysp reviewed Jul 23, 2019

View reviewed changes

zz-jason reviewed Jul 23, 2019

View reviewed changes

sessionctx/stmtctx/stmtctx.go Outdated Show resolved Hide resolved

zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 23, 2019

fix default current_timestamp calculation not consistent with now() f…

a4b3366

…unction results refactor related statement time related interface and some time function implementations

cfzjywxk force-pushed the p4391 branch from 5515694 to a4b3366 Compare July 23, 2019 08:58

cfzjywxk requested review from jackysp and zz-jason July 23, 2019 08:59

zz-jason approved these changes Jul 23, 2019

View reviewed changes

zz-jason merged commit ca70d74 into pingcap:master Jul 23, 2019

zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 23, 2019

cfzjywxk added a commit to cfzjywxk/tidb that referenced this pull request Jul 23, 2019

executor, expression: fix current_timestamp/now not consistent… (ping…

984d203

…cap#11342) cherry-pick to release-2.1 branch

cfzjywxk mentioned this pull request Jul 23, 2019

executor, expression: fix current_timestamp/now not consistent in one stmt like MySQL (#11342) #11392

Merged

AndrewDi mentioned this pull request Jul 23, 2019

expression: remove the NotNullFlag for aggregation func MAX/MIN when inferring type #11343

Merged

cfzjywxk added a commit to cfzjywxk/tidb that referenced this pull request Jul 23, 2019

executor, expression: fix current_timestamp/now not consistent… (ping…

f437ce8

…cap#11342)

cfzjywxk mentioned this pull request Jul 23, 2019

expressions: make time func now related unit tests stable(#11412)(#11342) #11394

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor, expression: fix current_timestamp/now not consistent in one stmt like MySQL #11342

executor, expression: fix current_timestamp/now not consistent in one stmt like MySQL #11342

cfzjywxk commented Jul 19, 2019 •

edited by qw4990

Loading

cfzjywxk commented Jul 19, 2019

cfzjywxk commented Jul 19, 2019

codecov bot commented Jul 19, 2019

codecov bot commented Jul 19, 2019 •

edited

Loading

cfzjywxk commented Jul 20, 2019

cfzjywxk commented Jul 20, 2019

cfzjywxk commented Jul 20, 2019

cfzjywxk commented Jul 20, 2019

jackysp Jul 22, 2019

jackysp Jul 22, 2019

jackysp Jul 22, 2019

zz-jason Jul 23, 2019

cfzjywxk Jul 23, 2019

jackysp Jul 23, 2019

cfzjywxk Jul 23, 2019

jackysp left a comment

jackysp left a comment

zz-jason left a comment

sre-bot commented Jul 23, 2019

sre-bot commented Jul 23, 2019

	func (sc *StatementContext) ReSetNowTs() {
	func (sc *StatementContext) ResetNowTs() {

executor, expression: fix current_timestamp/now not consistent in one stmt like MySQL #11342

executor, expression: fix current_timestamp/now not consistent in one stmt like MySQL #11342

Conversation

cfzjywxk commented Jul 19, 2019 • edited by qw4990 Loading

What problem does this PR solve?

What is changed and how it works?

Check List

cfzjywxk commented Jul 19, 2019

cfzjywxk commented Jul 19, 2019

codecov bot commented Jul 19, 2019

Codecov Report

codecov bot commented Jul 19, 2019 • edited Loading

Codecov Report

cfzjywxk commented Jul 20, 2019

cfzjywxk commented Jul 20, 2019

cfzjywxk commented Jul 20, 2019

cfzjywxk commented Jul 20, 2019

jackysp Jul 22, 2019

Choose a reason for hiding this comment

jackysp Jul 22, 2019

Choose a reason for hiding this comment

jackysp Jul 22, 2019

Choose a reason for hiding this comment

zz-jason Jul 23, 2019

Choose a reason for hiding this comment

cfzjywxk Jul 23, 2019

Choose a reason for hiding this comment

jackysp Jul 23, 2019

Choose a reason for hiding this comment

cfzjywxk Jul 23, 2019

Choose a reason for hiding this comment

jackysp left a comment

Choose a reason for hiding this comment

jackysp left a comment

Choose a reason for hiding this comment

zz-jason left a comment

Choose a reason for hiding this comment

sre-bot commented Jul 23, 2019

sre-bot commented Jul 23, 2019

cfzjywxk commented Jul 19, 2019 •

edited by qw4990

Loading

codecov bot commented Jul 19, 2019 •

edited

Loading