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 ddl modify column from null to not null bug: check null value again before change to no null #10948

Merged
merged 14 commits into from Jul 3, 2019

Conversation

@crazycs520
Copy link
Contributor

commented Jun 26, 2019

What problem does this PR solve?

Fix bug of modifying column from null to not null.

Origin logic:
null -> check column null value count is 0 -> set prevent-null-flag -> not null.

The problem is after setting prevent-null-flag, but before TiDB wait schema changed, insert null will success. And we should check check column null value count is 0 again before changing to not null.

Current logic:

null -> check column null value count is 0 -> set prevent null flag -> check column null value count is 0 -> not null.

What is changed and how it works?

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Side effects

  • Possible performance regression

Related changes

  • Need to cherry-pick to the release branch

crazycs520 added some commits Jun 26, 2019

@crazycs520

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

/run-all-tests

@codecov

This comment has been minimized.

Copy link

commented Jun 26, 2019

Codecov Report

Merging #10948 into master will decrease coverage by 0.0108%.
The diff coverage is 90.909%.

@@               Coverage Diff                @@
##             master     #10948        +/-   ##
================================================
- Coverage   81.0123%   81.0015%   -0.0109%     
================================================
  Files           419        418         -1     
  Lines         89379      89265       -114     
================================================
- Hits          72408      72306       -102     
+ Misses        11749      11727        -22     
- Partials       5222       5232        +10

crazycs520 added some commits Jun 26, 2019

crazycs520 added some commits Jun 26, 2019

@crazycs520

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

/run-all-tests

@crazycs520 crazycs520 changed the title ddl: fix ddl modify column from null to not null bug ddl: fix ddl modify column from null to not null bug: check null value again before change to no null Jun 26, 2019

@winkyao
Copy link
Member

left a comment

LGTM

Show resolved Hide resolved ddl/column.go Outdated

crazycs520 added some commits Jun 28, 2019

Show resolved Hide resolved ddl/db_test.go Outdated
Show resolved Hide resolved ddl/db_test.go Outdated
@bb7133
Copy link
Contributor

left a comment

LGTM

@bb7133 bb7133 added the status/LGT2 label Jul 2, 2019

if !mysql.HasNotNullFlag(oldCol.Flag) && mysql.HasNotNullFlag(newCol.Flag) && !mysql.HasPreventNullInsertFlag(oldCol.Flag) {
// Column from null to not null.
if !mysql.HasNotNullFlag(oldCol.Flag) && mysql.HasNotNullFlag(newCol.Flag) {
noPreventNullFlag := !mysql.HasPreventNullInsertFlag(oldCol.Flag)
// Introduce the `mysql.HasPreventNullInsertFlag` flag to prevent users from inserting or updating null values.
ver, err = modifyColumnFromNull2NotNull(w, t, dbInfo, tblInfo, job, oldCol, newCol)

This comment has been minimized.

Copy link
@zimulala

zimulala Jul 2, 2019

Member

It seems that if noPreventNullFlag is false, we only need to do checkForNullValue. In other words, we needn't do updateVersionAndTableInfoWithCheck. Do we change this code?

This comment has been minimized.

Copy link
@crazycs520

crazycs520 Jul 2, 2019

Author Contributor

Great, Thanks.

}
c2 := getModifyColumn()
if mysql.HasPreventNullInsertFlag(c2.Flag) {
_, err := s.tk.Exec("insert into t1 values ();")

This comment has been minimized.

Copy link
@zimulala

zimulala Jul 2, 2019

Member

Using tk2 is better?

This comment has been minimized.

Copy link
@crazycs520

crazycs520 Jul 2, 2019

Author Contributor

ye~, Thanks.

crazycs520 added some commits Jul 2, 2019

@zimulala
Copy link
Member

left a comment

LGTM

@zimulala

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

/run-all-tests

@crazycs520 crazycs520 merged commit 1061d2a into pingcap:master Jul 3, 2019

14 checks passed

ci/circleci Your tests passed on CircleCI!
Details
idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/build_check_race Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev_2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/common-test job succeeded
Details
idc-jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/mybatis-test job succeeded
Details
idc-jenkins-ci-tidb/sqllogic-test-1 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/sqllogic-test-2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@crazycs520 crazycs520 deleted the crazycs520:fix-null-to-notnull branch Jul 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.