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 security@mariadb.org