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

Fix bit default value bug #7249

Merged
merged 20 commits into from
Aug 31, 2018

Conversation

crazycs520
Copy link
Contributor

@crazycs520 crazycs520 commented Aug 2, 2018

What have you changed? (mandatory)

  • the problem:
mysql > create table t_bit (c1 bit(10) default 250, c2 int)
Query OK, 0 rows affected
Time: 0.109s
mysql > show create table t_bit
+-------+-------------------------------------------------------+
| Table | Create Table                                          |
+-------+-------------------------------------------------------+
| t_bit | CREATE TABLE `t_bit` (                                |
|       |   `c1` bit(10) DEFAULT b'111011111011111110111101',   |
|       |   `c2` int(11) DEFAULT NULL                           |
|       | ) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_bin |
+-------+-------------------------------------------------------+
1 row in set
Time: 0.007s
mysql > insert into t_bit set c2=1;
(1690, "BIT value is out of range in '(10)'")
  • how I fix it
    add DefaultValueBit to ColumnInfo to store the bit default value.
type ColumnInfo struct {
	...
	DefaultValueBit     []byte              `json:"default_bit"`
	...
}
  • compatibility
    old -> new : Does not affect the old data.
    new -> old : May have compatibility problem when the byte of default bit value >= 128. Because the old version always have the bug.

new:

mysql root@127.0.0.1:test> create table t_new (c1 int, c2 bit(10) default 128);
Query OK, 0 rows affected
mysql root@127.0.0.1:test> show create table t_new
+-------+-------------------------------------------------------+
| Table | Create Table                                          |
+-------+-------------------------------------------------------+
| t_new | CREATE TABLE `t_new` (                                |
|       |   `c1` int(11) DEFAULT NULL,                          |
|       |   `c2` bit(10) DEFAULT b'10000000'                    |
|       | ) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_bin |
+-------+-------------------------------------------------------+
mysql root@127.0.0.1:test> insert into t_new set c1=1;
Query OK, 1 row affected
Time: 0.001s
mysql root@127.0.0.1:test> select * from t_new;
+----+--------+
| c1 | c2     |
+----+--------+
| 1  | 0x0080 |
+----+--------+

rollback to old:

mysql root@127.0.0.1:test> select * from t_new;
Reconnecting...
+----+--------+
| c1 | c2     |
+----+--------+
| 1  | 0x0080 |
+----+--------+
1 row in set
Time: 0.007s
mysql root@127.0.0.1:test> insert into t_new set c1=2;
(1690, "BIT value is out of range in '(10)'")

What is the type of the changes? (mandatory)

  • Bug fix (non-breaking change which fixes an issue)

How has this PR been tested? (mandatory)

unit test

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

No

Does this PR affect tidb-ansible update? (mandatory)

No

Does this PR need to be added to the release notes? (mandatory)

No

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

@crazycs520 crazycs520 added the type/bugfix This PR fixes a bug. label Aug 2, 2018
@crazycs520
Copy link
Contributor Author

@winkyao @zimulala @ciscoxll PTAL

@crazycs520
Copy link
Contributor Author

/run-all-tests

@@ -66,6 +66,7 @@ type ColumnInfo struct {
Offset int `json:"offset"`
OriginDefaultValue interface{} `json:"origin_default"`
DefaultValue interface{} `json:"default"`
DefaultValueBit []byte `json:"default_bit"`
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to use a uniform default value in cloumn info?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

It is better to write down how to do it in the description of PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. What's the problem
  2. How to do it
  3. Is there any compatibility problem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ciscoxll done. PTAL

Copy link
Contributor

Choose a reason for hiding this comment

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

Will it be incompatible when rolling update TiDB?
As follows case:
On new TiDB, we builds a "add column" job , then we handle the DDL job on Old TiDB.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@crazycs520 crazycs520 Aug 22, 2018

Choose a reason for hiding this comment

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

@zimulala No. It is compatible.
if old tidb to handle job that created by new tidb, old tidb will ignore the DefaultValueBit .
I have done an experiment in this scene.

@crazycs520
Copy link
Contributor Author

/run-all-tests

@crazycs520
Copy link
Contributor Author

/run-unit-test

@shenli
Copy link
Member

shenli commented Aug 6, 2018

Should we modify coprocessor?

@crazycs520
Copy link
Contributor Author

@shenli , currently we don't support pushdown the expr which contain bit column, So I think we don't need to modify coprocessor.

model/model.go Outdated
return nil
}
return types.ErrInvalidDefault.GenByArgs(c.Name)

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this useless line.

@zhexuany
Copy link
Contributor

zhexuany commented Aug 6, 2018

/run-all-tests

@crazycs520
Copy link
Contributor Author

/run-unit-test

@crazycs520
Copy link
Contributor Author

@winkyao @zimulala @shenli PTAL

@zz-jason
Copy link
Member

LGTM

@zz-jason zz-jason added status/LGT1 Indicates that a PR has LGTM 1. component/DDL-need-LGT3 labels Aug 10, 2018
model/model.go Outdated
return nil
}

// GetDefaultValue gets the default value.
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we need to add more comments. I understand that GetDefaultValue will return the default value of the column but I am more interested in the case handling logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GetDefaultValue just return the column's default value.
just use column.GetDefaultValue() instead of column.DefaultValue,
because the bit type default value will stored in DefaultValueBit field.

@crazycs520
Copy link
Contributor Author

/run-all-tests

model/model.go Outdated
// bit type default value will store in DefaultValueBit for fix bit default value decode/encode bug.
func (c *ColumnInfo) GetDefaultValue() interface{} {
if c.Tp == mysql.TypeBit && c.DefaultValueBit != nil {
return string(c.DefaultValueBit)
Copy link
Member

Choose a reason for hiding this comment

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

should we use hack.String()?

model/model.go Outdated
func (c *ColumnInfo) SetDefaultValue(value interface{}) error {
c.DefaultValue = value
if c.Tp == mysql.TypeBit {
if v, ok := value.(string); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add more description here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

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.

Reset LGTM, please address comment

// GetDefaultValue gets the default value of the column.
// Default value use to stored in DefaultValue field, but now,
// bit type default value will store in DefaultValueBit for fix bit default value decode/encode bug.
func (c *ColumnInfo) GetDefaultValue() interface{} {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to change all "c.DefaultValue" to "GetDefaultValue()"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ye~ I have already changed it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"if column.DefaultValue != nil" in column.go, the ColumnInfo in column.go is not the ColumnInfo in model.go

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to replace "c.DefaultValue = types.ZeroDatetimeStr" with "SetDefaultValue()" in ddl_api.go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh...
ye...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

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

LGTM

@crazycs520 crazycs520 added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 30, 2018
winkyao
winkyao previously approved these changes Aug 30, 2018
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

@crazycs520
Copy link
Contributor Author

/run-all-tests

@crazycs520
Copy link
Contributor Author

/run-all-tests

@crazycs520 crazycs520 merged commit 8d1acc2 into pingcap:master Aug 31, 2018
@you06 you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 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/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.