ddl:support the definition of `null` change to `not null` using `alter table` #7771

ciscoxll · 2018-09-23T16:30:44Z

What problem does this PR solve?

Before this PR , we don't support modifying the type definitions of 'null ' to 'not null '.

Fix issue #5035.

What is changed and how it works?

Refer to https://dev.mysql.com/doc/refman/5.7/en/alter-table.html.

Before the job is executed.
Check if the field to be modified has a null value, cannot be modified if a null value exists.
In the job execution.
1. Query whether the field to be modified has a null value.
2. Then introduce another flag to prevent users from inserting or updating null values.
3. Modify the type defined Flag to NotNullFlag.
4. Query field definition whether there is a null value inserted after modification.
5. If there is a null value inserted , it cannot be modified and needs to be rollback.

Check List

Tests

Unit test

alivxxx · 2018-09-25T09:45:59Z

ddl/column.go

+		tblInfo.Columns[oldCol.Offset].Flag = newCol.Flag
+		ver, err = updateVersionAndTableInfo(t, job, tblInfo, true)
+		// Wait for two leases to ensure that all nodes in the cluster are updated successfully.
+		ver, err = updateVersionAndTableInfo(t, job, tblInfo, true)


Why update it again? What if tidb panics after this?

This question needs to be discussed.

alivxxx · 2018-09-25T09:47:12Z

ddl/column.go

@@ -379,6 +422,20 @@ func doModifyColumn(t *meta.Meta, job *model.Job, newCol *model.ColumnInfo, oldN
 	return ver, nil
 }

+// CheckForNullValue check for null values of the field.
+func CheckForNullValue(ctx sessionctx.Context, schema, table, oldCol, newCol model.CIStr) error {
+	sql := fmt.Sprintf(`select * from %s.%s where %s is null; `, schema.L, table.L, oldCol.L)


It is better to add quote around table/schema name, in case there are names like a b.

Use select count(*) from %s.%s where %s is null; instead, if the null values count is very large, this will oom.

winkyao · 2018-09-26T02:52:12Z

ddl/column.go

@@ -379,6 +422,20 @@ func doModifyColumn(t *meta.Meta, job *model.Job, newCol *model.ColumnInfo, oldN
 	return ver, nil
 }

+// CheckForNullValue check for null values of the field.


CheckForNullValue ensures there are no null values in the column of this table.

winkyao · 2018-09-26T08:38:30Z

any update?

zimulala · 2018-09-27T03:23:27Z

ddl/column.go

+		// Modify the type defined Flag to NotNullFlag.
+		tblInfo.Columns[oldCol.Offset].Flag = newCol.Flag
+		ver, err = updateVersionAndTableInfo(t, job, tblInfo, true)
+		// Wait for two leases to ensure that all nodes in the cluster are updated successfully.


This function can not do it.
This transaction is not committed, so no new schema needs to be updated by other nodes.

@zimulala I am doing this for the purpose, modify the field definition to NotNullFlag to prevent insertion of null values.

winkyao · 2018-09-29T03:28:02Z

ddl/column.go

@@ -267,7 +272,7 @@ func onSetDefaultValue(t *meta.Meta, job *model.Job) (ver int64, _ error) {
 	return updateColumn(t, job, newCol, &newCol.Name)
 }

-func onModifyColumn(t *meta.Meta, job *model.Job) (ver int64, _ error) {
+func (w *worker) onModifyColumn(t *meta.Meta, job *model.Job) (ver int64, _ error) {


Why we need to change this to member function?

@winkyao I need to get sessionctx from context resource pool.

winkyao · 2018-09-29T03:32:01Z

ddl/column.go

 	tblInfo, err := getTableInfo(t, job, job.SchemaID)
 	if err != nil {
 		return ver, errors.Trace(err)
 	}

 	oldCol := model.FindColumnInfo(tblInfo.Columns, oldName.L)
+	if job.IsRollingback() {
+		// field flag reset to null.
+		tblInfo.Columns[oldCol.Offset].Flag = oldCol.Flag &^ mysql.NotNullFlag


How can you confirm the oldCol definitely set to not null flag? What if the job is not alter column from not null to null?

zimulala · 2018-09-29T03:37:39Z

ddl/column.go

+		// Wait for two leases to ensure that all nodes in the cluster are updated successfully.
+		ver, err = updateVersionAndTableInfo(t, job, tblInfo, true)
+		if err != nil {
+			job.State = model.JobStateRollingback


If "err != nil" is true, I don't think we need this state.

@zimulala Here need to rollback the second time you enter the function.

@ciscoxll This comment is for your old commit.

alivxxx · 2018-09-29T05:45:06Z

ddl/ddl_api.go

 	if !mysql.HasNotNullFlag(col.Flag) && mysql.HasNotNullFlag(newCol.Flag) {
-		return nil, errUnsupportedModifyColumn.GenWithStackByArgs("null to not null")
+		if err = CheckForNullValue(ctx, ident.Schema, ident.Name, col.Name, newCol.Name); err != nil {


Why check it here? We will check it two times.

Job entering the ddl queue requires two checks on the job.

alivxxx · 2018-09-29T05:49:51Z

ddl/db_test.go

+	s.mustExec(c, "create table t4 (a int default '0', b varchar(10), d int not null default '0')")
+	s.tk.MustExec("insert into t4(a) values (null);")
+	sql = "alter table t4 change a a1 int not null"
+	s.testErrorCode(c, sql, tmysql.WarnDataTruncated)


Is this the same with mysql? In my environment:

mysql> alter table t4 change a a1 int not null; ERROR 1138 (22004): Invalid use of NULL value

@lamxTyler Done

mysql

mysql> create table test2 (c1 int, c2 int, c3 int default 1, index (c1)); Query OK, 0 rows affected (0.03 sec) mysql> insert into test2(c2) values (null); Query OK, 1 row affected (0.00 sec) mysql> alter table test2 change c2 a bigint not null; ERROR 1265 (01000): Data truncated for column 'a' at row 1

Your test also changes the column type, but if you use alter table test2 change c2 a int not null;, you can get a different result.

mysql> create table test2 (c1 int, c2 int, c3 int default 1, index (c1)); Query OK, 0 rows affected (0.01 sec) mysql> insert into test2(c2) values (null); Query OK, 1 row affected (0.00 sec) mysql> alter table test2 change c2 a bigint not null; ERROR 1265 (01000): Data truncated for column 'a' at row 1 mysql> alter table test2 change c2 a int not null; ERROR 1138 (22004): Invalid use of NULL value

FYI @ciscoxll

alivxxx · 2018-09-29T05:50:27Z

ddl/column.go

@@ -379,6 +431,20 @@ func doModifyColumn(t *meta.Meta, job *model.Job, newCol *model.ColumnInfo, oldN
 	return ver, nil
 }

+// CheckForNullValue ensure there are no null values of the column of this table.
+func CheckForNullValue(ctx sessionctx.Context, schema, table, oldCol, newCol model.CIStr) error {
+	sql := fmt.Sprintf("select count(*) from `%s`.`%s` where `%s` is null;", schema.L, table.L, oldCol.L)


Do we need to know the number of rows? If not, use limit 1 is better.

Please add limit 1

winkyao · 2018-09-29T06:13:36Z

ddl/column.go

+		defer w.sessPool.put(ctx)
+		err = CheckForNullValue(ctx, dbInfo.Name, tblInfo.Name, oldCol.Name, newCol.Name)
+		// If there is a null value inserted, it cannot be modified and needs to be rollback.
+		if ErrWarnDataTruncated.Equal(err) {


what if err != nil and err != ErrWarnDataTruncated

winkyao · 2018-10-08T01:55:27Z

ddl/column.go

+	if job.IsRollingback() {
+		if IsNull2Notnull {
+			// field flag reset to null.
+			tblInfo.Columns[oldCol.Offset].Flag = oldCol.Flag &^ mysql.NotNullFlag


please use another flag to indicate this column cannot insert null value. Otherwise, if there are some null value are inserted in the interval before the job is handled by the owner, decode a null value in column with mysql.NotNullFlag will return "Miss Column" error.

@winkyao Done.

ddl/column.go

crazycs520 · 2018-10-11T05:46:55Z

ddl/column.go

+	if !mysql.HasNotNullFlag(oldCol.Flag) && mysql.HasNotNullFlag(newCol.Flag) {
+		err = CheckForNullValue(ctx, dbInfo.Name, tblInfo.Name, oldCol.Name, newCol.Name)
+		if err != nil {
+			job.State = model.JobStateCancelled


set job.State = model.JobStateRollingback for rollback to clear PreventNullInsertFlag.
The reason is: If you insert null after CheckForNullValue but before set PreventNullInsertFlag, the ddl will failed and return in next CheckForNullValue, but have not clear the PreventNullInsertFlag.

crazycs520 · 2018-10-11T05:50:37Z

ddl/column.go

+			}
+		} else {
+			// Modify the type defined Flag to NotNullFlag.
+			tblInfo.Columns[oldCol.Offset].Flag = newCol.Flag


I think only modify the Flag to NotNullFlag and clear the PreventNullInsertFlag maybe better.

tblInfo.Columns[oldCol.Offset].Flag |= mysql.NotNullFlag tblInfo.Columns[oldCol.Offset].Flag = oldCol.Flag &^ mysql.PreventNullInsertFlag

winkyao

Please resolve the data race in test case.

winkyao

LGTM

reset approve

ciscoxll · 2018-10-16T05:15:09Z

/run-all-tests

winkyao · 2018-10-16T06:19:07Z

@zimulala PTAL

zimulala · 2018-10-17T02:58:12Z

ddl/column.go

+	}
+
+	if err != nil {
+		return ver, errors.Trace(err)


Maybe here use this code is enough.

zimulala · 2018-10-17T07:18:46Z

ddl/column.go

+		tblInfo.Columns[oldCol.Offset].Flag |= mysql.PreventNullInsertFlag
+	} else {
+		// Modify the type defined Flag to NotNullFlag.
+		tblInfo.Columns[oldCol.Offset].Flag |= mysql.NotNullFlag


Can we remove this situation? Then we can reduce the waiting time( 2*lease).

zimulala · 2018-10-17T07:43:03Z

ddl/column.go

+			return ver, nil
+		}
+		// Modify the type defined Flag to NotNullFlag.
+		tblInfo.Columns[oldCol.Offset].Flag |= mysql.NotNullFlag


Can we remove this code？
Because after this，we will assign newCol to the original column.

zimulala

LGTM

zimulala · 2018-10-17T08:01:28Z

/run-all-tests

bugfix fixed pingcap#7518 expression: MySQL compatible current_user function (pingcap#7801) plan: propagate constant over outer join (pingcap#7794) - extract `outerCol = const` from join conditions and filter conditions, substitute `outerCol` in join conditions with `const`; - extract `outerCol = innerCol` from join conditions, derive new join conditions based on this column equal condition and `outerCol` related expressions in join conditions and filter conditions; util/timeutil: fix data race caused by forgetting set stats lease to 0 (pingcap#7901) stats: handle ddl event for partition table (pingcap#7903) plan: implement Operand and Pattern of cascades planner. (pingcap#7910) planner: not convert to TableDual if empty range is derived from deferred constants (pingcap#7808) plan: move projEliminate behind aggEliminate (pingcap#7909) admin: fix admin check table bug of byte compare (pingcap#7887) * admin: remove reflect deepEqual stats: fix panic caused by empty histogram (pingcap#7912) plan: fix panic caused by empty schema of LogicalTableDual (pingcap#7906) * fix drop view if exist error (pingcap#7833) executor: refine `explain analyze` (pingcap#7888) executor: add an variable to compatible with MySQL insert for OGG (pingcap#7863) expression: maintain `DeferredExpr` in aggressive constant folding. (pingcap#7915) stats: fix histogram boundaries overflow error (pingcap#7883) ddl:support the definition of `null` change to `not null` using `alter table` (pingcap#7771) * ddl:support the definition of null change to not null using alter table ddl: add check when create table with foreign key. (pingcap#7885) * ddl: add check when create table with foreign key planner: eliminate if null on non null column (pingcap#7924) executor: fix a bug in point get (pingcap#7934) planner, executor: refine ColumnPrune for LogicalUnionAll (pingcap#7930) executor: fix panic when limit is too large (pingcap#7936) ddl: add TiDB version to metrics (pingcap#7902) stats: limit the length of sample values (pingcap#7931) vendor: update tipb (pingcap#7893) planner: support the Group and GroupExpr for the cascades planner (pingcap#7917) store/tikv: log more information when other err occurs (pingcap#7948) types: fix date time parse (pingcap#7933) ddl: just print error message when ddl job is normal to calcel, to eliminate noisy log (pingcap#7875) stats: update delta info for partition table (pingcap#7947) explaintest: add explain test for partition pruning (pingcap#7505) util: move disjoint set to util package (pingcap#7950) util: add PreAlloc4Row and Insert for Chunk and List (pingcap#7916) executor: add the slow log for commit (pingcap#7951) expression: add builtin json_keys (pingcap#7776) privilege: add USAGE in `show grants` for mysql compatibility (pingcap#7955) ddl: fix invailid ddl job panic (pingcap#7940) *: move ast.NewValueExpr to standalone parser_driver package (pingcap#7952) Make the ast package get rid of the dependency of types.Datum server: allow cors http request (pingcap#7939) *: move `Statement` and `RecordSet` from ast to sqlexec package (pingcap#7970) pr suggestion update executor/aggfuncs: split unit tests to corresponding file (pingcap#7993) store/tikv: fix typo (pingcap#7990) executor, planner: clone proj schema for different children in buildProj4Union (pingcap#7999) executor: let information_schema be the first database in ShowDatabases (pingcap#7938) stats: use local feedback for partition table (pingcap#7963) executor: add unit test for aggfuncs (pingcap#7966) server: add log for binary execute statement (pingcap#7987) admin: refine admin check decoder (pingcap#7862) executor: improve wide table insert & update performance (pingcap#7935) ddl: fix reassigned partition id in `truncate table` does not take effect (pingcap#7919) fix reassigned partition id in truncate table does not take effect add changelog for 2.1.0 rc4 (pingcap#8020) *: make parser package dependency as small as possible (pingcap#7989) parser: support `:=` in the `set` syntax (pingcap#8018) According to MySQL document, `set` use the = assignment operator, but the := assignment operator is also permitted stats: garbage collect stats for partition table (pingcap#7962) docs: add the proposal for the column pool (pingcap#7988) expression: refine built-in func truncate to support uint arg (pingcap#8000) stats: support show stats for partition table (pingcap#8023) stats: update error rate for partition table (pingcap#8022) stats: fix estimation for out of range point queries (pingcap#8015) *: move parser to a separate repository (pingcap#8036) executor: fix wrong result when index join on union scan. (pingcap#8031) Do not modify Plan of dataReaderBuilder directly, because it would impact next batch of outer rows, as well as other concurrent inner workers. Instead, build a local child builder to store the child plan. planner: fix a panic of a cached prepared statement with IndexScan (pingcap#8017) *: fix the issue of executing DDL after executing SQL failure in txn (pingcap#8044) * ddl, executor: fix the issue of executing DDL after executing SQL failure in txn add unit test remove debug info add like evaluator case sensitive test ddl, domain: make schema correct after canceling jobs (pingcap#7997) unit test fix code format proposal: maintaining histograms in plan. (pingcap#7605) support _tidb_rowid for table scan range (pingcap#8047) var rename fix

ddl:support the definition of null change to not null using alter table

f16f213

ciscoxll added the component/DDL-need-LGT3 label Sep 23, 2018

ciscoxll and others added 6 commits September 24, 2018 01:29

Update function comment

e9d24dd

Merge branch 'master' into modify-column

7e2cea2

Fix ci error

478e2d4

Fix ci error

63ebfe5

Fix ci error

96d7210

Update function comment

10aa28f

alivxxx reviewed Sep 25, 2018

View reviewed changes

ciscoxll added the status/WIP label Sep 25, 2018

winkyao reviewed Sep 26, 2018

View reviewed changes

ciscoxll added 2 commits September 26, 2018 17:36

Address comment

41b003e

Address comment

e4485ce

ciscoxll removed the status/WIP label Sep 26, 2018

zimulala reviewed Sep 27, 2018

View reviewed changes

winkyao reviewed Sep 29, 2018

View reviewed changes

zimulala reviewed Sep 29, 2018

View reviewed changes

ciscoxll added 2 commits September 29, 2018 12:11

Address comment

8be9eec

Change function entry condition

f5910fd

alivxxx reviewed Sep 29, 2018

View reviewed changes

winkyao reviewed Sep 29, 2018

View reviewed changes

ciscoxll added 3 commits September 29, 2018 14:23

Address comment

14f8559

Address comment

936d81f

Address comment

78c29f5

winkyao reviewed Oct 8, 2018

View reviewed changes

Address comment

fab2c06

crazycs520 reviewed Oct 11, 2018

View reviewed changes

Address comment

afc1f01

winkyao suggested changes Oct 16, 2018

View reviewed changes

Fix CI

3b638bf

winkyao approved these changes Oct 16, 2018

View reviewed changes

winkyao previously approved these changes Oct 16, 2018

View reviewed changes

ciscoxll added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 16, 2018

Merge branch 'master' into modify-column

64c83b6

ciscoxll added the status/all tests passed label Oct 16, 2018

Merge branch 'master' into modify-column

6b1beef

zimulala reviewed Oct 17, 2018

View reviewed changes

ddl/column.go Outdated

}

if err != nil {

return ver, errors.Trace(err)

Copy link

Contributor

zimulala Oct 17, 2018

Choose a reason for hiding this comment

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

Maybe here use this code is enough.

ciscoxll added 3 commits October 17, 2018 12:07

Address comment

d8f5133

Address comment

84c3c2e

Address comment

f87f096

zimulala reviewed Oct 17, 2018

View reviewed changes

Address comment

869f9b8

zimulala reviewed Oct 17, 2018

View reviewed changes

Address comment

44a592e

zimulala approved these changes Oct 17, 2018

View reviewed changes

ciscoxll added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Oct 17, 2018

Merge branch 'master' into modify-column

e16ed3c

ciscoxll merged commit e544882 into pingcap:master Oct 17, 2018

ciscoxll deleted the modify-column branch October 17, 2018 09:13

sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019

you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020

ddl:support the definition of null change to not null using alter table #7771

ddl:support the definition of null change to not null using alter table #7771

Conversation

ciscoxll commented Sep 23, 2018 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winkyao Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winkyao commented Sep 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winkyao Oct 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winkyao Sep 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crazycs520 Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winkyao left a comment

Choose a reason for hiding this comment

winkyao left a comment

Choose a reason for hiding this comment

ciscoxll commented Oct 16, 2018

winkyao commented Oct 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zimulala left a comment

Choose a reason for hiding this comment

zimulala commented Oct 17, 2018

ddl:support the definition of `null` change to `not null` using `alter table` #7771

ddl:support the definition of `null` change to `not null` using `alter table` #7771

ciscoxll commented Sep 23, 2018 •

edited

Loading

winkyao Sep 26, 2018 •

edited

Loading

winkyao Oct 15, 2018 •

edited

Loading

winkyao Sep 29, 2018 •

edited

Loading

crazycs520 Oct 11, 2018 •

edited

Loading