expression,table: fix insert partitioned table bug when the time zone change #14370

tiancaiamao · 2020-01-07T07:51:38Z

What problem does this PR solve?

mysql> set @@time_zone = 'UTC';                                                                                                                                                                       Query OK, 0 rows affected (0.00 sec)

mysql> CREATE TABLE `sbtest2` (
    ->        `id` int(11) NOT NULL,
    ->        `k` int(11) NOT NULL DEFAULT '0',
    ->        `c` char(120) NOT NULL DEFAULT '',
    ->        `pad` char(60) NOT NULL DEFAULT '',
    ->        `creation_dt` timestamp DEFAULT CURRENT_TIMESTAMP )  PARTITION BY RANGE ( unix_timestamp(`creation_dt`) )
    -> ( PARTITION p5 VALUES LESS THAN ( UNIX_TIMESTAMP('2020-01-03 15:10:00') ),
    ->   PARTITION p6 VALUES LESS THAN ( UNIX_TIMESTAMP('2020-01-03 15:15:00') ),
    ->   PARTITION p7 VALUES LESS THAN ( UNIX_TIMESTAMP('2020-01-03 15:20:00') ),
    ->   PARTITION p8 VALUES LESS THAN ( UNIX_TIMESTAMP('2020-01-03 15:25:00') ),
    ->   PARTITION p9 VALUES LESS THAN (MAXVALUE) )
    -> ;
Query OK, 0 rows affected (0.03 sec)

mysql> set @@time_zone = 'Asia/Shanghai';
Query OK, 0 rows affected (0.00 sec)

mysql> insert into sbtest2 values(1,1,'123','123','2020-01-03 15:16:59');                                                                                                                             Query OK, 1 row affected (0.00 sec)

mysql> select * from sbtest2 partition (p5);
+----+---+-----+-----+---------------------+
| id | k | c   | pad | creation_dt         |
+----+---+-----+-----+---------------------+
|  1 | 1 | 123 | 123 | 2020-01-03 15:16:59 |
+----+---+-----+-----+---------------------+
1 row in set (0.01 sec)

Create a partitioned table at timezone=X, then change timezone=Y, and insert data, the data doesn't locate in the correct partition.

What is changed and how it works?

The root cause of this bug is that expression should not cache the session context (which contains the timezone information).

When a partitioned table is loaded into memory, the partition expression is computed with the session context at that time.
If we change the session context (timezone) later and still use the partition expression, things would go wrong because expression uses the old session context.

Here for example:

insert into sbtest2 values(1,1,'123','123','2020-01-03 15:16:59');

When we're deciding which partition this data should belong to, we calculate the partition expression to get the answer.
The current timezone has already changed, but the partition expression still uses the old timezone.
So the result is wrong and we insert the data to the wrong partition.

Fix the expression is not a easy work. There are too much code involved.
This commit solely fixes the current table partition bug I meet.
We need to refactor a lot of codes to totally solve the problem:

define a builtinFuncNew interface
implement all evalXXXWithCtx for all builtinFuncSig
replace the builtinFunc with builtinFuncNew
remove ctx from the baseBuiltinFunc struct

Check List

Tests

Unit test

Code changes

Has interface methods change

Side effects

Possible performance regression

Related changes

Need to cherry-pick to the release branch

Release note

Write release note for bug-fix or new feature.

… change

tiancaiamao · 2020-01-07T08:03:14Z

PTAL @XuHuaiyu @bb7133

bb7133 · 2020-01-07T09:59:14Z

Hi @tiancaiamao , please update the release note in the PR Description.

Deardrops · 2020-01-07T11:20:29Z

expression/scalar_function.go

@@ -291,6 +291,9 @@ func (sf *ScalarFunction) Eval(row chunk.Row) (d types.Datum, err error) {

 // EvalInt implements Expression interface.
 func (sf *ScalarFunction) EvalInt(ctx sessionctx.Context, row chunk.Row) (int64, bool, error) {
+	if f, ok := sf.Function.(builtinFuncNew); ok {
+		return f.evalIntWithCtx(ctx, row)
+	}
 	return sf.Function.evalInt(row)


For using the context in current session. I think of another way:
How about add a new function setCtx for struct builtinFunc,
and change here as:

Suggested change

return sf.Function.evalInt(row)

sf.Function.setCtx(ctx)

return sf.Function.evalInt(row)

The setCtx way is ugly IMHO. @Deardrops
The real problem is, the session context is not something belongs to a function.
As long as function store the ctx, the function is invalid when the session context changes.
So the right way to fix the problem is removing ctx from the struct, instead of providing setCtx as a workaround.

go.sum

table/tables/partition_test.go

Co-Authored-By: bb7133 <bb7133@gmail.com>

Deardrops · 2020-01-08T02:02:38Z

In MySQL 8.0, It has same behavior with current TiDB version, does this really a bug?

mysql> set @@time_zone='+00:00';
Query OK, 0 rows affected (0.00 sec)

mysql> create table t1(dt timestamp) partition by range ( unix_timestamp(`dt`)) (
    partition p5 values less than (unix_timestamp('2020-01-03 15:10:00')), 
    partition p6 values less than (maxvalue)
);
Query OK, 0 rows affected (0.05 sec)

mysql> set @@time_zone='+08:00';
Query OK, 0 rows affected (0.00 sec)

mysql> insert into t1 values('2020-01-03 15:20:00');
Query OK, 1 row affected (0.01 sec)

mysql> select * from t1 partition (p5);
+---------------------+
| dt                  |
+---------------------+
| 2020-01-03 15:20:00 |
+---------------------+
1 row in set (0.04 sec)

mysql> select * from t1 partition (p6);
Empty set (0.00 sec)

tiancaiamao · 2020-01-08T03:48:37Z

Try this one and verify the result in TiDB and MySQL:

set @@time_zone='+08:00';
create table t1(dt timestamp) partition by range ( unix_timestamp(`dt`)) (
    partition p5 values less than (unix_timestamp('2020-01-03 15:10:00')), 
    partition p6 values less than (unix_timestamp('2020-01-03 15:20:00')), 
    partition p7 values less than (maxvalue)
);
set @@time_zone='+00:00';
insert into t1 values ('2020-01-03 15:15:00');
select * from t1 partition (p7);

@Deardrops

Deardrops · 2020-01-08T04:06:28Z

TiDB: (master branch)

mysql> set @@time_zone='+08:00';
Query OK, 0 rows affected (0.04 sec)

mysql> create table t1(dt timestamp) partition by range ( unix_timestamp(`dt`)) (
    ->     partition p5 values less than (unix_timestamp('2020-01-03 15:10:00')), 
    ->     partition p6 values less than (unix_timestamp('2020-01-03 15:20:00')), 
    ->     partition p7 values less than (maxvalue)
    -> );
Query OK, 0 rows affected (0.06 sec)

mysql> set @@time_zone='+00:00';
Query OK, 0 rows affected (0.00 sec)

mysql> insert into t1 values ('2020-01-03 15:15:00');
Query OK, 1 row affected (0.05 sec)

mysql> select * from t1 partition (p7);
Empty set (0.00 sec)

mysql> select * from t1 partition (p6);
+---------------------+
| dt                  |
+---------------------+
| 2020-01-03 15:15:00 |
+---------------------+
1 row in set (0.00 sec)

MySQL 8.0

mysql> set @@time_zone='+08:00';
Query OK, 0 rows affected (0.00 sec)

mysql> create table t1(dt timestamp) partition by range ( unix_timestamp(`dt`)) (
    ->     partition p5 values less than (unix_timestamp('2020-01-03 15:10:00')), 
    ->     partition p6 values less than (unix_timestamp('2020-01-03 15:20:00')), 
    ->     partition p7 values less than (maxvalue)
    -> );
Query OK, 0 rows affected (0.06 sec)

mysql> set @@time_zone='+00:00';
Query OK, 0 rows affected (0.00 sec)

mysql> insert into t1 values ('2020-01-03 15:15:00');
Query OK, 1 row affected (0.01 sec)

mysql> select * from t1 partition (p7);
+---------------------+
| dt                  |
+---------------------+
| 2020-01-03 15:15:00 |
+---------------------+
1 row in set (0.00 sec)

tiancaiamao · 2020-01-09T06:10:05Z

PTAL @bb7133 @XuHuaiyu @Deardrops

tiancaiamao · 2020-01-10T04:44:02Z

PTAL @bb7133 @XuHuaiyu @Deardrops

bb7133

LGTM

bb7133 · 2020-01-11T00:54:58Z

PTAL @Deardrops @XuHuaiyu

table/tables/partition_test.go

Deardrops

LGTM

tiancaiamao · 2020-01-14T09:25:48Z

/run-all-tests

jackysp · 2020-01-14T09:38:09Z

/merge

sre-bot · 2020-01-14T09:38:20Z

/run-all-tests

sre-bot · 2020-01-14T09:41:51Z

cherry pick to release-3.0 failed

… change (pingcap#14370)

… change (#14370) (#14476)

expression,table: fix insert partitioned table bug when the time zone…

9bba590

… change

tiancaiamao added type/bugfix This PR fixes a bug. component/expression labels Jan 7, 2020

tiancaiamao requested a review from a team as a code owner January 7, 2020 07:51

ghost requested review from qw4990 and XuHuaiyu and removed request for a team January 7, 2020 07:51

Merge branch 'master' into partition-timezone-change

003bc78

bb7133 added the needs-cherry-pick-3.0 label Jan 7, 2020

Deardrops reviewed Jan 7, 2020

View reviewed changes

bb7133 reviewed Jan 7, 2020

View reviewed changes

go.sum Outdated Show resolved Hide resolved

table/tables/partition_test.go Outdated Show resolved Hide resolved

table/tables/partition_test.go Show resolved Hide resolved

tiancaiamao and others added 2 commits January 7, 2020 23:38

Update table/tables/partition_test.go

58563fe

Co-Authored-By: bb7133 <bb7133@gmail.com>

address comment

19dc8a0

bb7133 reviewed Jan 10, 2020

View reviewed changes

bb7133 added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 10, 2020

Merge branch 'master' into partition-timezone-change

5e0b4ce

Deardrops reviewed Jan 12, 2020

View reviewed changes

table/tables/partition_test.go Show resolved Hide resolved

Deardrops reviewed Jan 14, 2020

View reviewed changes

Deardrops added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jan 14, 2020

Merge branch 'master' into partition-timezone-change

3b0e34f

tiancaiamao added the status/all tests passed label Jan 14, 2020

jackysp approved these changes Jan 14, 2020

View reviewed changes

sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 14, 2020

sre-bot merged commit c36f83e into pingcap:master Jan 14, 2020

tiancaiamao added a commit to tiancaiamao/tidb that referenced this pull request Jan 14, 2020

expression,table: fix insert partitioned table bug when the time zone…

ee26f83

… change (pingcap#14370)

tiancaiamao deleted the partition-timezone-change branch January 14, 2020 11:57

tiancaiamao mentioned this pull request Jan 14, 2020

expression,table: fix insert partitioned table bug when the time zone change (#14370) #14476

Merged

bb7133 pushed a commit that referenced this pull request Jan 16, 2020

expression,table: fix insert partitioned table bug when the time zone…

0d84245

… change (#14370) (#14476)

guo-shaoge mentioned this pull request Mar 10, 2021

the result of prepare is different from that of direct query #22949

Closed

lcwangchao mentioned this pull request Oct 25, 2023

proposal: remove ctx inner ScalarFunction and use sessionctx.Context as a input for all EvalXXX method #47958

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expression,table: fix insert partitioned table bug when the time zone change #14370

expression,table: fix insert partitioned table bug when the time zone change #14370

tiancaiamao commented Jan 7, 2020

tiancaiamao commented Jan 7, 2020

bb7133 commented Jan 7, 2020

Deardrops Jan 7, 2020

tiancaiamao Jan 7, 2020

Deardrops commented Jan 8, 2020

tiancaiamao commented Jan 8, 2020

Deardrops commented Jan 8, 2020 •

edited

Loading

tiancaiamao commented Jan 9, 2020

tiancaiamao commented Jan 10, 2020

bb7133 left a comment

bb7133 commented Jan 11, 2020

Deardrops left a comment

tiancaiamao commented Jan 14, 2020

jackysp commented Jan 14, 2020

sre-bot commented Jan 14, 2020

sre-bot commented Jan 14, 2020

	return sf.Function.evalInt(row)
	sf.Function.setCtx(ctx)
	return sf.Function.evalInt(row)

expression,table: fix insert partitioned table bug when the time zone change #14370

expression,table: fix insert partitioned table bug when the time zone change #14370

Conversation

tiancaiamao commented Jan 7, 2020

What problem does this PR solve?

What is changed and how it works?

Check List

tiancaiamao commented Jan 7, 2020

bb7133 commented Jan 7, 2020

Deardrops Jan 7, 2020

Choose a reason for hiding this comment

tiancaiamao Jan 7, 2020

Choose a reason for hiding this comment

Deardrops commented Jan 8, 2020

tiancaiamao commented Jan 8, 2020

Deardrops commented Jan 8, 2020 • edited Loading

tiancaiamao commented Jan 9, 2020

tiancaiamao commented Jan 10, 2020

bb7133 left a comment

Choose a reason for hiding this comment

bb7133 commented Jan 11, 2020

Deardrops left a comment

Choose a reason for hiding this comment

tiancaiamao commented Jan 14, 2020

jackysp commented Jan 14, 2020

sre-bot commented Jan 14, 2020

sre-bot commented Jan 14, 2020

Deardrops commented Jan 8, 2020 •

edited

Loading