Hello Sergei, Thanks for review. Please find a modified version attached. See comments below.
On 10/13/2017 02:08 PM, Sergei Golubchik wrote: > Hi, Alexander! > > On Oct 07, Alexander Barkov wrote: >> Hi Sergei, >> >> Please review a patch for MDEV-10802. >> >> Thanks! > >> diff --git >> a/mysql-test/suite/sys_vars/inc/explicit_defaults_for_timestamp.inc >> b/mysql-test/suite/sys_vars/inc/explicit_defaults_for_timestamp.inc >> index 4cf3914..508363b 100644 >> --- a/mysql-test/suite/sys_vars/inc/explicit_defaults_for_timestamp.inc >> +++ b/mysql-test/suite/sys_vars/inc/explicit_defaults_for_timestamp.inc >> @@ -97,3 +97,17 @@ CREATE TABLE t1 (a INT); >> ALTER TABLE t1 ADD b TIMESTAMP; >> SHOW CREATE TABLE t1; >> DROP TABLE t1; >> + >> +if (`SELECT @@explicit_defaults_for_timestamp=1`) >> +{ >> + --echo # >> + --echo # MDEV-10802 TIMESTAMP NOT NULL field with >> explicit_defaults_for_timestamp and NO_ZERO_DATE shouldn't throw error >> + --echo # >> + >> + SET sql_mode='ANSI,NO_ZERO_DATE'; >> + CREATE TABLE t1 (a TIMESTAMP NOT NULL); >> + INSERT INTO t1 VALUES (); >> + SELECT * FROM t1; >> + DROP TABLE t1; >> + SET sql_mode=DEFAULT; >> +} > > Why is it inside if()? > > 1. if you want it to run only when @@explicit_defaults_for_timestamp is > set, you should put it directly into > explicit_defaults_for_timestamp_on.test. > > 2. But it's better to run this test for either value of > @@explicit_defaults_for_timestamp, so it rightfully belongs into this > .inc file, but not inside if(). I used examples from the beginning of this file: if (`SELECT @@explicit_defaults_for_timestamp=0`) { --error ER_INVALID_DEFAULT CREATE TABLE t1 (a TIMESTAMP DEFAULT NULL); } if (`SELECT @@explicit_defaults_for_timestamp=1`) { CREATE TABLE t1 (a TIMESTAMP DEFAULT NULL); SHOW CREATE TABLE t1; DROP TABLE t1; } But it's indeed better to have it outside of "if". Fixed. > >> diff --git a/sql/sql_table.cc b/sql/sql_table.cc >> index 2751c79..fa5eb2a 100644 >> --- a/sql/sql_table.cc >> +++ b/sql/sql_table.cc >> @@ -4161,7 +4161,8 @@ mysql_prepare_create_table(THD *thd, HA_CREATE_INFO >> *create_info, >> if (!sql_field->def && >> !sql_field->has_default_function() && >> (sql_field->flags & NOT_NULL_FLAG) && >> - !is_timestamp_type(sql_field->sql_type)) >> + (!is_timestamp_type(sql_field->sql_type) || >> + opt_explicit_defaults_for_timestamp)) >> { >> sql_field->flags|= NO_DEFAULT_VALUE_FLAG; >> sql_field->pack_flag|= FIELDFLAG_NO_DEFAULT; >> @@ -4170,6 +4171,7 @@ mysql_prepare_create_table(THD *thd, HA_CREATE_INFO >> *create_info, >> if (thd->variables.sql_mode & MODE_NO_ZERO_DATE && >> !sql_field->def && !sql_field->vcol_info && >> is_timestamp_type(sql_field->sql_type) && >> + !opt_explicit_defaults_for_timestamp && > > why opt_explicit_defaults_for_timestamp is relevant here? When explicit-defaults-for-timestamp is 0, this script: SET sql_mode=NO_ZERO_DATE; DROP TABLE IF EXISTS t1; CREATE TABLE t1 (a TIMESTAMP NOT NULL, b TIMESTAMP NOT NULL); is considered to be equal to: SET sql_mode=NO_ZERO_DATE; DROP TABLE IF EXISTS t1; CREATE TABLE t1 ( a TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, b TIMESTAMP NOT NULL DEFAULT '0000-00-00 00:00:00' ); Therefore both scripts should return this error: ERROR 1067 (42000): Invalid default value for 'b' When explicit-defaults-for-timestamp is 1, then the first script should not return any errors errors. > >> (sql_field->flags & NOT_NULL_FLAG) && >> (type == Field::NONE || type == Field::TIMESTAMP_UN_FIELD)) >> { > > Regards, > Sergei > Chief Architect MariaDB > and secur...@mariadb.org >
diff --git a/mysql-test/suite/sys_vars/inc/explicit_defaults_for_timestamp.inc b/mysql-test/suite/sys_vars/inc/explicit_defaults_for_timestamp.inc index 4cf3914..1fea4ca 100644 --- a/mysql-test/suite/sys_vars/inc/explicit_defaults_for_timestamp.inc +++ b/mysql-test/suite/sys_vars/inc/explicit_defaults_for_timestamp.inc @@ -97,3 +97,16 @@ CREATE TABLE t1 (a INT); ALTER TABLE t1 ADD b TIMESTAMP; SHOW CREATE TABLE t1; DROP TABLE t1; + +--echo # +--echo # MDEV-10802 TIMESTAMP NOT NULL field with explicit_defaults_for_timestamp and NO_ZERO_DATE shouldn't throw error +--echo # + +SET timestamp=UNIX_TIMESTAMP('2001-01-01 10:20:30'); +SET sql_mode='ANSI,NO_ZERO_DATE'; +CREATE TABLE t1 (a TIMESTAMP NOT NULL); +INSERT INTO t1 VALUES (); +SELECT * FROM t1; +DROP TABLE t1; +SET sql_mode=DEFAULT; +SET timestamp=DEFAULT; diff --git a/mysql-test/suite/sys_vars/r/explicit_defaults_for_timestamp_off.result b/mysql-test/suite/sys_vars/r/explicit_defaults_for_timestamp_off.result index cdf612e..61a4eb8 100644 --- a/mysql-test/suite/sys_vars/r/explicit_defaults_for_timestamp_off.result +++ b/mysql-test/suite/sys_vars/r/explicit_defaults_for_timestamp_off.result @@ -173,3 +173,16 @@ t1 CREATE TABLE `t1` ( `b` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP ) ENGINE=MyISAM DEFAULT CHARSET=latin1 DROP TABLE t1; +# +# MDEV-10802 TIMESTAMP NOT NULL field with explicit_defaults_for_timestamp and NO_ZERO_DATE shouldn't throw error +# +SET timestamp=UNIX_TIMESTAMP('2001-01-01 10:20:30'); +SET sql_mode='ANSI,NO_ZERO_DATE'; +CREATE TABLE t1 (a TIMESTAMP NOT NULL); +INSERT INTO t1 VALUES (); +SELECT * FROM t1; +a +2001-01-01 10:20:30 +DROP TABLE t1; +SET sql_mode=DEFAULT; +SET timestamp=DEFAULT; diff --git a/mysql-test/suite/sys_vars/r/explicit_defaults_for_timestamp_on.result b/mysql-test/suite/sys_vars/r/explicit_defaults_for_timestamp_on.result index 1c42da5..fb820dc 100644 --- a/mysql-test/suite/sys_vars/r/explicit_defaults_for_timestamp_on.result +++ b/mysql-test/suite/sys_vars/r/explicit_defaults_for_timestamp_on.result @@ -178,3 +178,18 @@ t1 CREATE TABLE `t1` ( `b` timestamp NULL DEFAULT NULL ) ENGINE=MyISAM DEFAULT CHARSET=latin1 DROP TABLE t1; +# +# MDEV-10802 TIMESTAMP NOT NULL field with explicit_defaults_for_timestamp and NO_ZERO_DATE shouldn't throw error +# +SET timestamp=UNIX_TIMESTAMP('2001-01-01 10:20:30'); +SET sql_mode='ANSI,NO_ZERO_DATE'; +CREATE TABLE t1 (a TIMESTAMP NOT NULL); +INSERT INTO t1 VALUES (); +Warnings: +Warning 1364 Field 'a' doesn't have a default value +SELECT * FROM t1; +a +0000-00-00 00:00:00 +DROP TABLE t1; +SET sql_mode=DEFAULT; +SET timestamp=DEFAULT; diff --git a/sql/sql_table.cc b/sql/sql_table.cc index 2751c79..fa5eb2a 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -4161,7 +4161,8 @@ mysql_prepare_create_table(THD *thd, HA_CREATE_INFO *create_info, if (!sql_field->def && !sql_field->has_default_function() && (sql_field->flags & NOT_NULL_FLAG) && - !is_timestamp_type(sql_field->sql_type)) + (!is_timestamp_type(sql_field->sql_type) || + opt_explicit_defaults_for_timestamp)) { sql_field->flags|= NO_DEFAULT_VALUE_FLAG; sql_field->pack_flag|= FIELDFLAG_NO_DEFAULT; @@ -4170,6 +4171,7 @@ mysql_prepare_create_table(THD *thd, HA_CREATE_INFO *create_info, if (thd->variables.sql_mode & MODE_NO_ZERO_DATE && !sql_field->def && !sql_field->vcol_info && is_timestamp_type(sql_field->sql_type) && + !opt_explicit_defaults_for_timestamp && (sql_field->flags & NOT_NULL_FLAG) && (type == Field::NONE || type == Field::TIMESTAMP_UN_FIELD)) {
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp