[Maria-developers] MDEV-3929 Add system variable explicit_defaults_for_timestamp for compatibility with MySQL
Hi Sergei, Please review a patch that merges a new read-only variables --explicit-defaults-for-timestamp from MySQL. Note, the original MySQL patch updates many tests, to make the tests independent from the compiled-in default --explicit-defaults-for-timestamp value. I guess Oracle is planning to change the default value to ON eventually. I merged these changes for tests as well. As a result, all tests pass in both "mtr" and "mtr --mysqld=--explicit-defaults-for-timestamp", which I think is good. In addition to the original MySQL changes in mtr, I also added two tests that describe specific behavior: sys_vars.explicit_defaults_for_timestamp_off sys_vars.explicit_defaults_for_timestamp_on Note, the patch assumes that the server and the slave have the same --explicit-defaults-for-timeatamp. Monty suggested to make this variable dynamic and replication-safe. We added a separate task for this: MDEV-8455 Make --explicit-defaults-for-timestamp dynamic I plan to do it in the next 10.1 sprint round. Thanks.
Forgot to attach the patch. On 07/13/2015 12:54 PM, Alexander Barkov wrote:
Hi Sergei,
Please review a patch that merges a new read-only variables --explicit-defaults-for-timestamp from MySQL.
Note, the original MySQL patch updates many tests, to make the tests independent from the compiled-in default --explicit-defaults-for-timestamp value. I guess Oracle is planning to change the default value to ON eventually. I merged these changes for tests as well. As a result, all tests pass in both "mtr" and "mtr --mysqld=--explicit-defaults-for-timestamp", which I think is good.
In addition to the original MySQL changes in mtr, I also added two tests that describe specific behavior: sys_vars.explicit_defaults_for_timestamp_off sys_vars.explicit_defaults_for_timestamp_on
Note, the patch assumes that the server and the slave have the same --explicit-defaults-for-timeatamp. Monty suggested to make this variable dynamic and replication-safe. We added a separate task for this: MDEV-8455 Make --explicit-defaults-for-timestamp dynamic I plan to do it in the next 10.1 sprint round.
Thanks.
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
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
Hi, Alexander! On Sep 15, Alexander Barkov wrote:
- 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. ... At this point I just merged the changes from MySQL, and the variable is read only.
Hmm, in that case why did you add it to thd->variables? That only makes sense for session variables that can have different values in different connections. Global (and read-only) variables don't need to have a copy of itself in every THD.
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?
I mean that we have sysvar_* tests now. These test shows already that explicit_defaults_for_timestamp is a read-only variable. I'd say it's sufficient. Regards, Sergei
Hi Sergei, On 09/15/2015 10:19 AM, Sergei Golubchik wrote:
Hi, Alexander!
On Sep 15, Alexander Barkov wrote:
- 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. ... At this point I just merged the changes from MySQL, and the variable is read only.
Hmm, in that case why did you add it to thd->variables?
That only makes sense for session variables that can have different values in different connections. Global (and read-only) variables don't need to have a copy of itself in every THD.
Hmm. Right. The original MySQL changes have a replication related addon patch for the slave that tries to guess what was explicit_defaults_for_timestamp on the master, depending on its version and some other heuristic. So they do update explicit_defaults_for_timestamp per thread on the slave. As Monty proposed to make it dynamic (MDEV-8455), I did not include this code and planned to make all replication related changes in a single patch for MDEV-8455. But I forgot to exclude the variable from the session variables. Will do that.
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?
I mean that we have sysvar_* tests now. These test shows already that explicit_defaults_for_timestamp is a read-only variable. I'd say it's sufficient.
Regards, Sergei
Hi, Alexander! On Jul 13, Alexander Barkov wrote:
Hi Sergei,
Please review a patch that merges a new read-only variables --explicit-defaults-for-timestamp from MySQL.
Note, the original MySQL patch updates many tests, to make the tests independent from the compiled-in default --explicit-defaults-for-timestamp value. I guess Oracle is planning to change the default value to ON eventually. I merged these changes for tests as well.
Okay. I wouldn't do it, but it's your call...
As a result, all tests pass in both "mtr" and "mtr --mysqld=--explicit-defaults-for-timestamp", which I think is good.
I don't care much about it. That is, I don't think it's valuable to spend the time on making tests to pass for all possible initial values of server options.
In addition to the original MySQL changes in mtr, I also added two tests that describe specific behavior: sys_vars.explicit_defaults_for_timestamp_off sys_vars.explicit_defaults_for_timestamp_on
If they're functional tests, their file names typically end with _func. Although it's not that important now, when all_vars tests is disabled. Regards, Sergei
Hi Sergei, On 07/18/2015 01:01 PM, Sergei Golubchik wrote:
Hi, Alexander!
On Jul 13, Alexander Barkov wrote:
Hi Sergei,
Please review a patch that merges a new read-only variables --explicit-defaults-for-timestamp from MySQL.
Note, the original MySQL patch updates many tests, to make the tests independent from the compiled-in default --explicit-defaults-for-timestamp value. I guess Oracle is planning to change the default value to ON eventually. I merged these changes for tests as well.
Okay. I wouldn't do it, but it's your call...
As a result, all tests pass in both "mtr" and "mtr --mysqld=--explicit-defaults-for-timestamp", which I think is good.
I don't care much about it. That is, I don't think it's valuable to spend the time on making tests to pass for all possible initial values of server options.
In addition to the original MySQL changes in mtr, I also added two tests that describe specific behavior: sys_vars.explicit_defaults_for_timestamp_off sys_vars.explicit_defaults_for_timestamp_on
If they're functional tests, their file names typically end with _func. Although it's not that important now, when all_vars tests is disabled.
Should I rename them to: sys_vars.explicit_defaults_for_timestamp_func_off sys_vars.explicit_defaults_for_timestamp_func_on or sys_vars.explicit_defaults_for_timestamp_off_func sys_vars.explicit_defaults_for_timestamp_on_func ? Okey to push after rename? Thanks.
Regards, Sergei
Hi, Alexander! On Jul 20, Alexander Barkov wrote:
In addition to the original MySQL changes in mtr, I also added two tests that describe specific behavior: sys_vars.explicit_defaults_for_timestamp_off sys_vars.explicit_defaults_for_timestamp_on
If they're functional tests, their file names typically end with _func. Although it's not that important now, when all_vars tests is disabled.
Should I rename them to:
sys_vars.explicit_defaults_for_timestamp_func_off sys_vars.explicit_defaults_for_timestamp_func_on
or
sys_vars.explicit_defaults_for_timestamp_off_func sys_vars.explicit_defaults_for_timestamp_on_func
?
If you'd prefer to rename - please check how other functional sys_vars tests are named. But I suspect the distinction between basic and functional tests will start to disappear now, as we no longer create obligatory basic tests for every new variable. So, it's ok not to rename.
Okey to push after rename?
No, sorry, I haven't checked the patch yet, I only replied to this your email. Regards, Sergei
participants (2)
-
Alexander Barkov -
Sergei Golubchik