← Back to team overview

maria-developers team mailing list archive

Re: MDEV-3929 Add system variable explicit_defaults_for_timestamp for compatibility with MySQL


Hi Sergei,

Thanks for the review!
Please see comments and questions below:

On 09/11/2015 06:58 PM, Sergei Golubchik wrote:
Hi, Alexander!

On Jul 13, Alexander Barkov wrote:
Forgot to attach the patch.

Thanks, here's the review below.
I did not review all the test changes, I suppose you've mostly added
explicit DEFAULT NOW ON UPDATE NOW clause everywhere.


diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
index 1d31df4..80651ec 100644
--- a/sql/sql_yacc.yy
+++ b/sql/sql_yacc.yy
@@ -6316,8 +6316,10 @@ field_type:
                  Unlike other types TIMESTAMP fields are NOT NULL by default.
+                This behavior is deprecated now.
-              Lex->last_field->flags|= NOT_NULL_FLAG;
+              if (!thd->variables.explicit_defaults_for_timestamp)
+                Lex->last_field->flags|= NOT_NULL_FLAG;

Hmm, I don't like that. You're making a run-time decision during the parsing.
Generally this is wrong. Sometimes it might be safe, but not in this case, I think.
Here's the idea of a test case:

   1) prepare x from "create table t1 (...) as select ...t2 "; <--- parsing happens here
   2) execute x;                                               <--- no parsing
   3) alter table t2 ...                                       <--- trigger auto-reparsing
   4) drop table t1; execute x;                                <--- now the statement is parsed again

so, say, you do SET @@explicit_defaults_for_timestamp=1 before step 1 and
SET @@explicit_defaults_for_timestamp=0 after step 1.
in this case first execute (step 2) will use explicit_defaults_for_timestamp=1,
the second execute (step 4) will use explicit_defaults_for_timestamp=0.

                $$= opt_mysql56_temporal_format ? MYSQL_TYPE_TIMESTAMP2
                                                : MYSQL_TYPE_TIMESTAMP;

At this point I just merged the changes from MySQL,
and the variable is read only.

I guess you have on mind this task proposed by Monty:
MDEV-8455 Make explicit_defaults_for_timestamp dynamic
But this is another story.

Can we do these run-time related changes later,
under terms of MDEV-8455?

diff --git a/mysql-test/suite/sys_vars/inc/sysvars_server.inc b/mysql-test/suite/sys_vars/inc/sysvars_server.inc
index cb06b40..b98ebf5 100644
--- a/mysql-test/suite/sys_vars/inc/sysvars_server.inc
+++ b/mysql-test/suite/sys_vars/inc/sysvars_server.inc
    from information_schema.system_variables
    where variable_name in (
+          'explicit_defaults_for_timestamp',

No, please add explicit_defaults_for_timestamp in the first part of the test. With the value.


I made this way because I wanted both
"mtr" and "mtr --mysqld=--explicit-defaults-for-timestamp=1"
pass all tests.

But you're right, we need to have at least one test which makes
sure the default value is OFF. I'll fix this.

diff --git a/mysql-test/suite/sys_vars/t/explicit_defaults_for_timestamp_basic.test b/mysql-test/suite/sys_vars/t/explicit_defaults_for_timestamp_basic.test
new file mode 100644
index 0000000..372e37c
--- /dev/null
+++ b/mysql-test/suite/sys_vars/t/explicit_defaults_for_timestamp_basic.test
@@ -0,0 +1,19 @@

No need to add these boilerplate *_basic tests anymore.
if there's something worth testing there - yes, but if you just
copy another test like that and replace the variable name - don't bother

I put the test that it's a read-only variable here.
I had to put it somewhere anyway. Would you like to
to rename this test somehow?

+# show the global and session values;
+select @@global.explicit_defaults_for_timestamp;
+select @@session.explicit_defaults_for_timestamp;
+show global variables like 'explicit_defaults_for_timestamp';
+show session variables like 'explicit_defaults_for_timestamp';
+select * from information_schema.global_variables where variable_name='explicit_defaults_for_timestamp';
+select * from information_schema.session_variables where variable_name='explicit_defaults_for_timestamp';
+# show that it's read-only
+set global explicit_defaults_for_timestamp=true;
+set session explicit_defaults_for_timestamp=true;


Follow ups