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 admin check bug #7359

Merged
merged 7 commits into from
Aug 21, 2018
Merged

Conversation

crazycs520
Copy link
Contributor

@crazycs520 crazycs520 commented Aug 11, 2018

What problem does this PR solve?

Use to executed below sqls will error:

drop table if exists t1;
CREATE TABLE t1 (c2 BOOL, PRIMARY KEY (c2));
INSERT INTO t1 SET c2 = '0';
ALTER TABLE t1 ADD COLUMN c3 DATETIME NULL DEFAULT '2668-02-03 17:19:31';
ALTER TABLE t1 ADD INDEX idx3 (c3);
ALTER TABLE t1 ADD COLUMN c4 bit(10) default 127;
ALTER TABLE t1 ADD INDEX idx4 (c4);
ALTER TABLE t1 ADD COLUMN c5 SET('a', 'b', 'c', 'd') default 'a';
ALTER TABLE t1 ADD INDEX idx5(c5);
admin check table t1;

(1105, 't1 err:[admin:1]index:&admin.RecordData{Handle:0, Values:[]types.Datum{types.Datum{k:0xd, collation:0x0, decimal:0x0, length:0x0, i:2440818046768513024, b:[]uint8(nil), x:types.Time{Time:types.MysqlTime{year:0xa6c, month:0x2, day:0x3, hour:17, minute:0x13, second:0x1f, microsecond:0x0}, Type:0xc, Fsp:0}}}} != record:&admin.RecordData{Handle:0, Values:[]types.Datum{types.Datum{k:0xd, collation:0x0, decimal:0x0, length:0x0, i:0, b:[]uint8(nil), x:types.Time{Time:types.MysqlTime{year:0xa6c, month:0x2, day:0x3, hour:17, minute:0x13, second:0x1f, microsecond:0x0}, Type:0xc, Fsp:0}}}}')

The bug is:
When tablecodec.unflatten() the datetime type, the flatten value still in datum.i,
we have to clear the flatten value to make sure a flatten datum go through unflatten is deep equal the origin datum

What is changed and how it works?

Check List

Tests

  • Unit test

Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM

@winkyao
Copy link
Contributor

winkyao commented Aug 11, 2018

@crazycs520 Please posts the error information in the description, and explain why your fix work.

@breezewish
Copy link
Member

Is it needed to clear other fields in datum as well, for example, field b?

@shenli
Copy link
Member

shenli commented Aug 11, 2018

Will this bug affect other parts except for AdminCheck?

@crazycs520
Copy link
Contributor Author

crazycs520 commented Aug 11, 2018

@shenli yes, It will affect other parts too.
em...
I mean the change will affect other parts.

@winkyao
Copy link
Contributor

winkyao commented Aug 12, 2018

@shenli No, it just affects the admin check table.

@crazycs520
Copy link
Contributor Author

@zimulala @ciscoxll PTAL

@shenli
Copy link
Member

shenli commented Aug 13, 2018

/run-all-tests

@shenli
Copy link
Member

shenli commented Aug 13, 2018

unflatten is a fundamental function. Is it a bug?

@crazycs520
Copy link
Contributor Author

crazycs520 commented Aug 13, 2018

@shenli the flatten and unflatten code use to be:

// for datetime
// origin
data1 := Datum{
    i:0,
    x: Time{
        ...
    },
    ...
}
// after flatten
data2 := Datum{
    i:2440818046768513024,
    x: nil,
    ...
}

// after unflatten
data3 := Datum{
    i:2440818046768513024,
     x: Time{
        ...
    },
    ...
}

admin check will compare datum then found datum.i not equal, then return error.
Maybe this is not exactly a bug. Because data3.GetMysqlTime() still return right result.
if use datum.CompareDatum() to compare in admin check, then do not change the unflatten function.

@@ -441,6 +441,7 @@ func unflatten(datum types.Datum, ft *types.FieldType, loc *time.Location) (type
return datum, errors.Trace(err)
}
}
datum.SetUint64(0)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to clean the i for TypeDuration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we needn't. d.SetMysqlDuration(x) will set i again.

Copy link
Contributor

Choose a reason for hiding this comment

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

@crazycs520 SetDuration -> SetMysqlDuration. Inside SetMysqlDuration, we assign the value of duration to i after type conversion. So we definitely need clean i of Datum in the case of TypeDuration.

@shenli
Copy link
Member

shenli commented Aug 14, 2018

@coocood PTAL

@crazycs520
Copy link
Contributor Author

@breeswish Sorry, I didn’t see yours comment before.
In flatten and unflatten, I think we can only care about i.

@zz-jason
Copy link
Member

@crazycs520 Seems like we need to clear other fields when we call Datum.SetXXXX ?

@crazycs520
Copy link
Contributor Author

@coocood What is your opinion?

@ciscoxll
Copy link
Contributor

/run-all-tests

@crazycs520
Copy link
Contributor Author

/run-all-tests

@coocood
Copy link
Member

coocood commented Aug 20, 2018

LGTM

@zhexuany
Copy link
Contributor

cc @crazycs520 please fix ci failure.

@alivxxx
Copy link
Contributor

alivxxx commented Aug 20, 2018

@zhexuany It will be fixed in #7434

@coocood
Copy link
Member

coocood commented Aug 20, 2018

/run-unit-test

@coocood coocood added status/LGT1 Indicates that a PR has LGTM 1. status/all tests passed labels Aug 20, 2018
@zimulala zimulala added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 20, 2018
@crazycs520
Copy link
Contributor Author

/run-all-tests

Copy link
Member

@shenli shenli left a comment

Choose a reason for hiding this comment

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

LGTM

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/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet