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.
Correct.
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 @@ -41,6 +42,7 @@ select VARIABLE_NAME, VARIABLE_SCOPE, VARIABLE_TYPE, VARIABLE_COMMENT, ENUM_VALUE_LIST, READ_ONLY, COMMAND_LINE_ARGUMENT 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.
'have_openssl', 'have_symlink', 'hostname',
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'; +--disable_warnings +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'; +--enable_warnings + +# +# show that it's read-only +# +--error ER_INCORRECT_GLOBAL_LOCAL_VAR +set global explicit_defaults_for_timestamp=true; +--error ER_INCORRECT_GLOBAL_LOCAL_VAR +set session explicit_defaults_for_timestamp=true;
Regards, Sergei