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