ddl:In non-strict sql mode, fix column type is text /blob/json default value null not processed. #7230

ciscoxll · 2018-08-01T09:40:53Z

What have you changed? (mandatory)

In non-strict SQL mode, we can ignore that the default value of a text /blob column is an empty string.
In non-strict SQL mode, if the column type is json and the default value is null, it is initialized to an empty array.
Fix issue default value for text/blob/json type #7158

master branch

mysql> create table test_text_default_value(c1 text not null default '');
ERROR 1050 (42S01): Table 'test.test_text_default_value' already exists

mysql> create table test_text_default_json(c1 json not null default '');
ERROR 1101 (42000): BLOB/TEXT/JSON column 'c1' can't have a default value

mysql> create table test_text_default_blob(c1 blob not null default '');
ERROR 1101 (42000): BLOB/TEXT/JSON column 'c1' can't have a default value

mysql> set sql_mode='';
Query OK, 0 rows affected (0.00 sec)

mysql>  create table test_text_default_value(c1 text not null default '');
ERROR 1101 (42000): BLOB/TEXT/JSON column 'c1' can't have a default value

mysql> create table test_text_default_json(c1 json not null default '');
ERROR 1101 (42000): BLOB/TEXT/JSON column 'c1' can't have a default value
mysql> create table test_text_default_blob(c1 blob not null default '');

ERROR 1101 (42000): BLOB/TEXT/JSON column 'c1' can't have a default value

sql-mode-compatible branch

tidb> create table test_text_default_value(c1 text not null default '');
ERROR 1101 (42000): BLOB, TEXT, GEOMETRY or JSON column 'c1' can't have a default value

tidbl> set sql_mode='';
Query OK, 0 rows affected (0.00 sec)

tidb> create table test_text_default_json(c1 json not null default '');
Query OK, 0 rows affected, 1 warning (0.03 sec)

tidb> show create table test_text_default_json;
+------------------------+--------------------------------------------------------------------------------------------------------------------+
| Table                  | Create Table                                                                                                       |
+------------------------+--------------------------------------------------------------------------------------------------------------------+
| test_text_default_json | CREATE TABLE `test_text_default_json` (
  `c1` json NOT NULL DEFAULT 'null'
) ENGINE=InnoDB DEFAULT CHARSET=latin1 |
+------------------------+--------------------------------------------------------------------------------------------------------------------+
1 row in set (0.01 sec)

tidb> create table test_text_default_value(c1 text not null default '');
ERROR 1101 (42000): BLOB, TEXT, GEOMETRY or JSON column 'c1' can't have a default value

tidb> set sql_mode='';
Query OK, 0 rows affected (0.00 sec)

tidb> create table test_text_default_value(c1 text not null default '');
Query OK, 0 rows affected, 1 warning (0.02 sec)

tidb> show warnings;
+---------+------+---------------------------------------------------------------------+
| Level   | Code | Message                                                             |
+---------+------+---------------------------------------------------------------------+
| Warning | 1101 | BLOB, TEXT, GEOMETRY or JSON column 'c1' can't have a default value |
+---------+------+---------------------------------------------------------------------+
1 row in set (0.01 sec)

tidb> show create table test_text_default_value;
+-------------------------+------------------------------------------------------------------------------------------------------+
| Table                   | Create Table                                                                                         |
+-------------------------+------------------------------------------------------------------------------------------------------+
| test_text_default_value | CREATE TABLE `test_text_default_value` (
  `c1` text NOT NULL
) ENGINE=InnoDB DEFAULT CHARSET=latin1 |
+-------------------------+------------------------------------------------------------------------------------------------------+
1 row in set (0.01 sec)

What is the type of the changes? (mandatory)

Improvement

Improvement (non-breaking change which is an improvement to an existing feature)

How has this PR been tested? (mandatory)

Unit tests.

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

…ue null not processed.

crazycs520 · 2018-08-01T13:09:57Z

ddl/ddl_api.go

@@ -244,10 +244,15 @@ func buildColumnAndConstraint(ctx sessionctx.Context, offset int,

 // checkColumnCantHaveDefaultValue checks the column can have value as default or not.
 // Now, TEXT/BLOB/JSON can't have not null value as default.
-func checkColumnCantHaveDefaultValue(col *table.Column, value interface{}) (err error) {
+func checkColumnCantHaveDefaultValue(ctx sessionctx.Context, col *table.Column, value interface{}) (err error) {


how about this?

func checkColumnCantHaveDefaultValue(ctx sessionctx.Context, col *table.Column, value interface{}) (error, bool, interface{}) { if value != nil && (col.Tp == mysql.TypeJSON || col.Tp == mysql.TypeTinyBlob || col.Tp == mysql.TypeMediumBlob || col.Tp == mysql.TypeLongBlob || col.Tp == mysql.TypeBlob) { if !ctx.GetSessionVars().SQLMode.HasStrictMode() && value == "" { sc := ctx.GetSessionVars().StmtCtx sc.AppendWarning(errBlobCantHaveDefault.GenByArgs(col.Name.O)) if col.Tp == mysql.TypeJSON { return nil, true, `null` } return nil, false, nil } // TEXT/BLOB/JSON can't have not null default values. return errBlobCantHaveDefault.GenByArgs(col.Name.O), false, nil } return nil, true, value }

and in line 322:

err, hasDefaultValue, value = checkColumnCantHaveDefaultValue(ctx, col, value); if err != nil { return nil, nil, errors.Trace(err) } if hasDefaultValue { col.DefaultValue = value removeOnUpdateNowFlag(col) }

and in line 1443

err, hasDefaultValue, value = checkColumnCantHaveDefaultValue(ctx, col, value) if err != nil { return errors.Trace(err) } if hasDefaultValue { col.DefaultValue = value }

then we can remove ignoreNullDefaultValuesForColumns function

crazycs520 · 2018-08-01T13:11:26Z

ddl/db_test.go

+	is := domain.GetDomain(ctx).InfoSchema()
+	tblInfo, err := is.TableByName(model.NewCIStr("test"), model.NewCIStr("text_default_text"))
+	c.Assert(err, IsNil)
+	c.Assert(tblInfo.Meta().Columns[0].DefaultValue, Equals, "")


I think the DefaultValue should be nil, not be ""

DefaultValue should be ""

When the default value of the column is null, DefaultValue is "" .

crazycs520 · 2018-08-01T13:12:04Z

ddl/db_test.go

+	is = domain.GetDomain(ctx).InfoSchema()
+	tblInfo, err = is.TableByName(model.NewCIStr("test"), model.NewCIStr("text_default_blob"))
+	c.Assert(err, IsNil)
+	c.Assert(tblInfo.Meta().Columns[0].DefaultValue, Equals, "")


Have explained.

crazycs520 · 2018-08-01T13:12:20Z

ddl/db_test.go

+	is = domain.GetDomain(ctx).InfoSchema()
+	tblInfo, err = is.TableByName(model.NewCIStr("test"), model.NewCIStr("text_default_json"))
+	c.Assert(err, IsNil)
+	c.Assert(tblInfo.Meta().Columns[0].DefaultValue, Equals, `null`)


~~ditto~~
I got it, It should be null....

crazycs520 · 2018-08-01T15:25:10Z

ddl/ddl_api.go

+				// In non-strict SQL mode, if the column type is json and the default value is null, it is initialized to an empty array.
+				if col.Tp == mysql.TypeJSON && value == "" {
+					col.DefaultValue = `null`
+					hasDefaultValue = true


this line can be remove

tiancaiamao · 2018-08-02T03:20:43Z

ddl/ddl_api.go

 	if value != nil && (col.Tp == mysql.TypeJSON ||
 		col.Tp == mysql.TypeTinyBlob || col.Tp == mysql.TypeMediumBlob ||
 		col.Tp == mysql.TypeLongBlob || col.Tp == mysql.TypeBlob) {
+		if !ctx.GetSessionVars().SQLMode.HasStrictMode() && value == "" {


What will happen when SQL mode change, create a table with SQLMode A and use it wit SQLMode B ?

In non-strict SQL mode, if the default value of the column is an empty string, can ignore it, the table can be created successfully.

I understand.

tiancaiamao · 2018-08-02T03:38:22Z

/run-all-tests

winkyao · 2018-08-02T06:25:02Z

ddl/db_test.go

+
+	s.tk.MustExec("set sql_mode='';")
+	s.tk.MustExec("drop table if exists text_default_text;")
+	s.tk.MustExec("create table text_default_text(c1 text not null default '');")


Add test about show create table text_default_text and show create table test_text_default_value

@winkyao Done.

crazycs520

LGTM

ciscoxll · 2018-08-02T10:37:20Z

/rebuild

ciscoxll · 2018-08-02T11:16:11Z

/run-all-tests

ciscoxll · 2018-08-02T14:55:56Z

@tiancaiamao @winkyao PTAL.

tiancaiamao · 2018-08-03T02:45:49Z

ddl/ddl_api.go

 	if value != nil && (col.Tp == mysql.TypeJSON ||
 		col.Tp == mysql.TypeTinyBlob || col.Tp == mysql.TypeMediumBlob ||
 		col.Tp == mysql.TypeLongBlob || col.Tp == mysql.TypeBlob) {
+		if !ctx.GetSessionVars().SQLMode.HasStrictMode() && value == "" {
+			// In non-strict SQL mode, if the default value of the column is an empty string, can ignore it.


s/can ignore it/the default value can be ignored

tiancaiamao · 2018-08-03T02:46:34Z

ddl/ddl_api.go

 	if value != nil && (col.Tp == mysql.TypeJSON ||
 		col.Tp == mysql.TypeTinyBlob || col.Tp == mysql.TypeMediumBlob ||
 		col.Tp == mysql.TypeLongBlob || col.Tp == mysql.TypeBlob) {
+		if !ctx.GetSessionVars().SQLMode.HasStrictMode() && value == "" {
+			// In non-strict SQL mode, if the default value of the column is an empty string, can ignore it.
+			if col.Tp == mysql.TypeBlob || col.Tp == mysql.TypeLongBlob && value == "" {


&& value == "" is redundant because it has been tested at line 261

tiancaiamao · 2018-08-03T02:49:07Z

ddl/ddl_api.go

@@ -253,14 +253,24 @@ func buildColumnAndConstraint(ctx sessionctx.Context, offset int,

 // checkColumnCantHaveDefaultValue checks the column can have value as default or not.
 // Now, TEXT/BLOB/JSON can't have not null value as default.


Update comment... in strict mode, TEXT/BLOB/JSON can't have not null value as default.

IMHO, this function signature is very strange:
why checkColumnCantHaveDefaultValue return a boolean value hasDefaultValue

Do agree with tiancaiamao.

winkyao · 2018-08-03T03:02:16Z

ddl/db_test.go

+	s.tk.MustExec("drop table if exists text_default_text;")
+	s.tk.MustExec("create table text_default_text(c1 text not null default '');")
+	s.tk.MustQuery(`show create table text_default_text`).Check(testutil.RowsWithSep("|",
+		""+


remove this line?

winkyao · 2018-08-03T03:18:52Z

ddl/ddl_api.go

+			}
+			sc := ctx.GetSessionVars().StmtCtx
+			sc.AppendWarning(errBlobCantHaveDefault.GenByArgs(col.Name.O))
+			return hasDefaultValue, nil


You can return a specific error here, then remove hasDefaultValue. You can test the specific error in the caller to set the hasDefaultValue.

ciscoxll · 2018-08-03T08:06:39Z

@tiancaiamao @winkyao PTAL.

winkyao · 2018-08-06T02:09:02Z

ddl/ddl_api.go

+			sc.AppendWarning(errBlobCantHaveDefault.GenByArgs(col.Name.O))
+			return hasDefaultValue, value, nil
+		}
+		// In strict SQL mode.


In strict SQL mode or default value is not an empty string.

winkyao

reset LGTM

tiancaiamao · 2018-08-06T02:33:44Z

LGTM

winkyao

LGTM

ciscoxll added 2 commits August 1, 2018 17:22

in non-strict SQL mode,fix column type is text /blob/json default val…

fbbdde1

…ue null not processed.

update comments

7a57b63

ciscoxll added the component/DDL-need-LGT3 label Aug 1, 2018

ciscoxll changed the title ~~ddl:In non-strict SQL mode, fix column type is text /blob/json default value null not processed.~~ ddl:In non-strict sql mode, fix column type is text /blob/json default value null not processed. Aug 1, 2018

crazycs520 reviewed Aug 1, 2018

View reviewed changes

address comments

dbc1ac1

crazycs520 reviewed Aug 1, 2018

View reviewed changes

address comments

cc0babc

tiancaiamao reviewed Aug 2, 2018

View reviewed changes

shenli added the type/compatibility label Aug 2, 2018

winkyao reviewed Aug 2, 2018

View reviewed changes

address comments

40aad3e

ciscoxll force-pushed the sql-mode-compatible branch from dbf0876 to 40aad3e Compare August 2, 2018 08:30

crazycs520 reviewed Aug 2, 2018

View reviewed changes

ciscoxll added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 2, 2018

Merge branch 'master' into sql-mode-compatible

a3ab51a

ciscoxll added status/all tests passed and removed component/DDL-need-LGT3 labels Aug 2, 2018

ciscoxll added the status/all tests passed label Aug 2, 2018

tiancaiamao reviewed Aug 3, 2018

View reviewed changes

winkyao reviewed Aug 3, 2018

View reviewed changes

ciscoxll added the status/WIP label Aug 3, 2018

ciscoxll added 2 commits August 3, 2018 11:25

address comments

eaa5850

address comments

69ae665

ciscoxll removed the status/WIP label Aug 3, 2018

add testCheckColumnDefaultValue test

3b25e37

winkyao reviewed Aug 6, 2018

View reviewed changes

address comments

2226cb5

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

tiancaiamao approved these changes Aug 6, 2018

View reviewed changes

winkyao approved these changes Aug 6, 2018

View reviewed changes

Merge branch 'master' into sql-mode-compatible

8916387

ciscoxll merged commit d261b6a into pingcap:master Aug 6, 2018

ciscoxll deleted the sql-mode-compatible branch August 6, 2018 03:20

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl:In non-strict sql mode, fix column type is text /blob/json default value null not processed. #7230

ddl:In non-strict sql mode, fix column type is text /blob/json default value null not processed. #7230

ciscoxll commented Aug 1, 2018 •

edited

Loading

crazycs520 Aug 1, 2018 •

edited

Loading

crazycs520 Aug 1, 2018 •

edited

Loading

ciscoxll Aug 1, 2018

crazycs520 Aug 1, 2018

ciscoxll Aug 1, 2018

crazycs520 Aug 1, 2018

ciscoxll Aug 2, 2018

crazycs520 Aug 1, 2018 •

edited

Loading

crazycs520 Aug 1, 2018

tiancaiamao Aug 2, 2018

ciscoxll Aug 2, 2018

ciscoxll Aug 3, 2018

tiancaiamao commented Aug 2, 2018

winkyao Aug 2, 2018

ciscoxll Aug 2, 2018

crazycs520 left a comment

ciscoxll commented Aug 2, 2018

ciscoxll commented Aug 2, 2018

ciscoxll commented Aug 2, 2018

tiancaiamao Aug 3, 2018

tiancaiamao Aug 3, 2018

tiancaiamao Aug 3, 2018

tiancaiamao Aug 3, 2018

winkyao Aug 3, 2018

winkyao Aug 3, 2018

winkyao Aug 3, 2018

ciscoxll commented Aug 3, 2018

winkyao Aug 6, 2018

winkyao left a comment

tiancaiamao commented Aug 6, 2018

winkyao left a comment

		@@ -253,14 +253,24 @@ func buildColumnAndConstraint(ctx sessionctx.Context, offset int,

		// checkColumnCantHaveDefaultValue checks the column can have value as default or not.
		// Now, TEXT/BLOB/JSON can't have not null value as default.

ddl:In non-strict sql mode, fix column type is text /blob/json default value null not processed. #7230

ddl:In non-strict sql mode, fix column type is text /blob/json default value null not processed. #7230

Conversation

ciscoxll commented Aug 1, 2018 • edited Loading

What have you changed? (mandatory)

What is the type of the changes? (mandatory)

How has this PR been tested? (mandatory)

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

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

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

crazycs520 Aug 1, 2018 • edited Loading

Choose a reason for hiding this comment

crazycs520 Aug 1, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crazycs520 Aug 1, 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

Choose a reason for hiding this comment

tiancaiamao commented Aug 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crazycs520 left a comment

Choose a reason for hiding this comment

ciscoxll commented Aug 2, 2018

ciscoxll commented Aug 2, 2018

ciscoxll commented Aug 2, 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

ciscoxll commented Aug 3, 2018

Choose a reason for hiding this comment

winkyao left a comment

Choose a reason for hiding this comment

tiancaiamao commented Aug 6, 2018

winkyao left a comment

Choose a reason for hiding this comment

ciscoxll commented Aug 1, 2018 •

edited

Loading

crazycs520 Aug 1, 2018 •

edited

Loading

crazycs520 Aug 1, 2018 •

edited

Loading

crazycs520 Aug 1, 2018 •

edited

Loading