Skip to content
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

alter table modify column to a time type may generate incorrect value #56051

Closed
lcwangchao opened this issue Sep 12, 2024 · 1 comment · Fixed by #56059
Closed

alter table modify column to a time type may generate incorrect value #56051

lcwangchao opened this issue Sep 12, 2024 · 1 comment · Fixed by #56059
Labels
component/ddl This issue is related to DDL of TiDB. severity/major type/bug The issue is confirmed as a bug.

Comments

@lcwangchao
Copy link
Collaborator

lcwangchao commented Sep 12, 2024

Bug Report

Please answer these questions before submitting your issue. Thanks!

1. Minimal reproduce step (Required)

My local time zone is Asia/Shanghai

> create table t(t int);
> insert into t values(NULL);
> set @@time_zone='UTC' --- set a different time zone with local
> alter table t modify column t timestamp not null;
> select now(),t from t;

2. What did you expect to see? (Required)

The value of now() and t should not differs too much.

3. What did you see instead (Required)

These two columns differs two hours

> select t, now() from t
+---------------------+---------------------+
| t                   | now()               |
+---------------------+---------------------+
| 2024-09-12 04:09:36 | 2024-09-12 12:13:47 |
+---------------------+---------------------+

4. What is your TiDB version? (Required)

@lcwangchao
Copy link
Collaborator Author

This bug is introduced by #55873 . In this PR, we use StaticEvalContext to replace mock.Context. However, these two contexts behaviors a little different for GetCurrentTime

For mock.Context, it uses SessionEvalContext as its inner expression context, with uses getStmtTimestamp to return a current time:

func (ctx *SessionEvalContext) CurrentTime() (time.Time, error) {
return getStmtTimestamp(ctx.sctx)
}

And getStmtCurrentTime returns a time with location time.Local:

return time.Unix(int64(seconds), int64(fractionalSeconds*float64(time.Second))), nil

For StaticEvalContext, it returns a current time with a location same with EvalContext.Location():

So, if the out side code has a strong assumption that the returned current time's location, it will fail if we replace mock.Context to StaticEvalContext. Unfortunately, there are at least two places have this strong assumption:

In getTimeCurrentTimeStamp, it can cause DDL inserting wrong values in reorging:

func getTimeCurrentTimeStamp(ctx EvalContext, tp byte, fsp int) (t types.Time, err error) {
value := types.NewTime(types.ZeroCoreTime, tp, fsp)
defaultTime, err := getStmtTimestamp(ctx)
if err != nil {
return value, err
}
value.SetCoreTime(types.FromGoTime(defaultTime.Truncate(time.Duration(math.Pow10(9-fsp)) * time.Nanosecond)))
if tp == mysql.TypeTimestamp || tp == mysql.TypeDatetime || tp == mysql.TypeDate {
err = value.ConvertTimeZone(time.Local, ctx.Location())
if err != nil {
return value, err
}
}
return value, nil
}

In evalNowWithFsp, it can cause lazy cursor fetch return wrong now() value:

func evalNowWithFsp(ctx EvalContext, fsp int) (types.Time, bool, error) {
nowTs, err := getStmtTimestamp(ctx)
if err != nil {
return types.ZeroTime, true, err
}
failpoint.Inject("injectNow", func(val failpoint.Value) {
nowTs = time.Unix(int64(val.(int)), 0)
})
// In MySQL's implementation, now() will truncate the result instead of rounding it.
// Results below are from MySQL 5.7, which can prove it.
// mysql> select now(6), now(3), now();
// +----------------------------+-------------------------+---------------------+
// | now(6) | now(3) | now() |
// +----------------------------+-------------------------+---------------------+
// | 2019-03-25 15:57:56.612966 | 2019-03-25 15:57:56.612 | 2019-03-25 15:57:56 |
// +----------------------------+-------------------------+---------------------+
result, err := convertTimeToMysqlTime(nowTs, fsp, types.ModeTruncate)
if err != nil {
return types.ZeroTime, true, err
}
err = result.ConvertTimeZone(time.Local, location(ctx))
if err != nil {
return types.ZeroTime, true, err
}
return result, false, nil
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/ddl This issue is related to DDL of TiDB. severity/major type/bug The issue is confirmed as a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants