maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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