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; } 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', 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
+# +# 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