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: prohibit modification decimal precision to avoid inconsistency problem #10433

Merged
merged 4 commits into from
May 14, 2019

Conversation

crazycs520
Copy link
Contributor

What problem does this PR solve?

tidb> create table t1 (a decimal(2,1), index(a))
Query OK, 0 rows affected
Time: 0.070s
tidb> insert into t1 set a='-3.9'
Query OK, 1 row affected
Time: 0.009s
tidb> alter table t1 modify column a DECIMAL(22, 3);
Query OK, 0 rows affected
Time: 0.065s
tidb> delete from t1;
Query OK, 1 row affected
Time: 0.003s
tidb> admin check table t1
(8003, u't1 err:[admin:1]index:&admin.RecordData{Handle:1, Values:[]types.Datum{types.Datum{k:0x8, collation:0x0, decimal:0x1, length:0x2, i:0, b:[]uint8(nil), x:(*types.MyDecimal)(0xc000ab48a0)}}} != record:<nil>')

What is changed and how it works?

Because EncodeDecimal encodes the precision information, if change the decimal precision, need rewrite the record.

This PR just simple prohibit modification decimal precision to avoid inconsistency problem.

After TiDB support modifies column type with rewrite data, This problem will be completely solved.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Side effects

Related changes

  • Need to cherry-pick to the release branch

@crazycs520 crazycs520 added the type/bugfix This PR fixes a bug. label May 13, 2019
@crazycs520
Copy link
Contributor Author

/run-all-tests

@codecov
Copy link

codecov bot commented May 13, 2019

Codecov Report

Merging #10433 into master will increase coverage by 0.0039%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##             master     #10433        +/-   ##
================================================
+ Coverage   77.0483%   77.0522%   +0.0039%     
================================================
  Files           412        412                
  Lines         86508      86501         -7     
================================================
- Hits          66653      66651         -2     
+ Misses        14744      14740         -4     
+ Partials       5111       5110         -1

ddl/ddl_api.go Outdated
@@ -2192,6 +2192,10 @@ func modifiableCharsetAndCollation(toCharset, toCollate, origCharset, origCollat
// It returns true if the two types has the same Charset and Collation, the same sign, both are
// integer types or string types, and new Flen and Decimal must be greater than or equal to origin.
func modifiable(origin *types.FieldType, to *types.FieldType) error {
// Because EncodeDecimal encodes the precision information, if change the decimal precision, need rewrite the record.
Copy link
Member

Choose a reason for hiding this comment

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

The root cause is modifying decimal precision needs to rewrite binary representation of that decimal?

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.

c.Assert(err, NotNil)
c.Assert(err.Error(), Equals, "[ddl:203]unsupported modify decimal column precision")
tk.MustExec(`delete from t1;`)
tk.MustExec(`admin check table t1;`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why check table after all rows have been deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tidb> create table t1 (a decimal(2,1), index(a))
Query OK, 0 rows affected
Time: 0.070s
tidb> insert into t1 set a='-3.9'
Query OK, 1 row affected
Time: 0.009s
tidb> alter table t1 modify column a DECIMAL(22, 3);
Query OK, 0 rows affected
Time: 0.065s
tidb> delete from t1;
Query OK, 1 row affected
Time: 0.003s
tidb> admin check table t1
(8003, u't1 err:[admin:1]index:&admin.RecordData{Handle:1, Values:[]types.Datum{types.Datum{k:0x8, collation:0x0, decimal:0x1, length:0x2, i:0, b:[]uint8(nil), x:(*types.MyDecimal)(0xc000ab48a0)}}} != record:<nil>')

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label May 13, 2019
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

@zimulala zimulala added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 14, 2019
@ngaut ngaut merged commit 088603f into pingcap:master May 14, 2019
crazycs520 added a commit to crazycs520/tidb that referenced this pull request May 14, 2019
crazycs520 added a commit to crazycs520/tidb that referenced this pull request May 14, 2019
crazycs520 added a commit to crazycs520/tidb that referenced this pull request May 14, 2019
winkyao pushed a commit that referenced this pull request May 14, 2019
winkyao pushed a commit that referenced this pull request May 24, 2019
winkyao added a commit that referenced this pull request May 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants