maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

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