maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

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@xxxxxxxxxxx
> 

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))
     {