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

ddl: fix add index on timestamp column bug #7724

Merged
merged 3 commits into from
Sep 20, 2018

Conversation

crazycs520
Copy link
Contributor

@crazycs520 crazycs520 commented Sep 17, 2018

What problem does this PR solve?

reproduce SQL:

 drop table if exists t1;
 CREATE TABLE t1 (c2 YEAR, PRIMARY KEY (c2));
 INSERT INTO t1 SET c2 = '1912';
 ALTER TABLE t1 ADD COLUMN c3 TIMESTAMP NULL DEFAULT '1976-08-29 16:28:11';
 ALTER TABLE t1 ADD INDEX c5 (c2, c3);
 admin check table t1;

In old TiDB version, the SQL: admin check table t1 will return error cause by wrong index record.

What is changed and how it works?

In back fill record to index, if use the column default value, and if the column type is timestamp, We should convert the default timestamp value to UTC timestamp value.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Related changes

  • Need to cherry-pick to the release branch ?

@crazycs520
Copy link
Contributor Author

@winkyao @ciscoxll PTAL

@shenli
Copy link
Member

shenli commented Sep 17, 2018

/run-all-tests

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

ciscoxll
ciscoxll previously approved these changes Sep 20, 2018
Copy link
Contributor

@ciscoxll ciscoxll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@winoros winoros dismissed ciscoxll’s stale review September 20, 2018 05:58

Only two currently

@winoros winoros added the status/LGT2 Indicates that a PR has LGTM 2. label Sep 20, 2018
Copy link
Member

@coocood coocood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@coocood coocood added status/all tests passed status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Sep 20, 2018
@zz-jason zz-jason merged commit 4a444ef into pingcap:master Sep 20, 2018
@crazycs520 crazycs520 deleted the index-timestamp-bug branch September 21, 2018 06:06
@you06 you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Aug 24, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/sql-infra SIG: SQL Infra status/LGT3 The PR has already had 3 LGTM. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants