maria-developers team mailing list archive

[Vicențiu Ciorbaru <vicentiu@xxxxxxxxxxx>]

Hi Daniel!

The patch looks good, some comments about the test cases.
About the commit message, we have --write-binlog not just --binlog.
Also, check for typos in the last commit message and also, I would squash
the
two test cases commits into just one.

> diff --git a/mysql-test/suite/rpl/t/rpl_mysql_upgrade.test
b/mysql-test/suite/rpl/t/rpl_mysql_upgrade.test
> index 1931e2eab2a..b836e889429 100644
> --- a/mysql-test/suite/rpl/t/rpl_mysql_upgrade.test
> +++ b/mysql-test/suite/rpl/t/rpl_mysql_upgrade.test
> @@ -14,8 +14,8 @@ call mtr.add_suppression("table or database name
'mysqltest-1'");
>
>  connection master;
>  --disable_warnings
> -DROP DATABASE IF EXISTS `#mysql50#mysqltest-1`;
> -CREATE DATABASE `#mysql50#mysqltest-1`;
> +DROP DATABASE IF EXISTS `mysqltest-1`;
> +CREATE DATABASE `mysqltest-1`;
Since you are doing cleanup here anyway, let's get rid of the DROP
statements
a test case should not leave any dirty context behind and if we drop things
just to be sure, we are masking potential problems.
>  --enable_warnings
>  sync_slave_with_master;
>
> @@ -34,13 +34,20 @@ if ($before_position == $after_position)
>    echo Master position is not changed;
>  }
>
> -#Some log events of the mysql_upgrade's will cause errors on slave.
> +# Some log events of the mysql_upgrade previously caused errors on slave,
> +# however with MDEV-4851 this should be ok, so we test it:
>  connection slave;
> -STOP SLAVE SQL_THREAD;
> -source include/wait_for_slave_sql_to_stop.inc;
> +SET @old_general_log_state = @@global.general_log;
> +SET @old_slow_log_state = @@global.slow_query_log;
> +SET @old_log_output = @@global.log_output;
> +SET GLOBAL general_log = 'ON';
> +SET GLOBAL slow_query_log = 'ON';
> +SET GLOBAL log_output = 'FILE';
>
>  connection master;
>  #With '--force' option, mysql_upgrade always executes all sql statements
for upgrading.
> +ALTER TABLE mysql.slow_log DROP COLUMN thread_id, DROP COLUMN
rows_affected;
> +DROP DATABASE IF EXISTS `mysqltest-1`;
Why IF EXISTS? It *should* be there already.
Can you add some syncs between master and slave and then show from the slave
connection that the log table table is missing those columns and also that
they get applied *after* mysql_upgrade is run?
>  --exec $MYSQL_UPGRADE --skip-verbose --write-binlog --force --user=root
> $MYSQLTEST_VARDIR/log/mysql_upgrade.log 2>&1
>
>  let $datadir= `select @@datadir`;
> @@ -55,8 +62,14 @@ if ($before_position != $after_position)
>    echo Master position has been changed;
>  }
>
> -DROP DATABASE `mysqltest-1`;
Notice that the original code didn't use "IF EXISTS".
> +sync_slave_with_master;
>  connection slave;
> -DROP DATABASE `#mysql50#mysqltest-1`;
> ---let $rpl_only_running_threads= 1
> +SET GLOBAL general_log = 'OFF';
> +SET GLOBAL slow_query_log = 'OFF';
> +truncate mysql.slow_log;
> +truncate mysql.general_log;
Why do you need to clear the tables? I tested without clearing and it still
needs to pass.
> +SET GLOBAL general_log = @old_general_log_state;
> +SET GLOBAL slow_query_log = @old_slow_log_state;
> +SET GLOBAL log_output = @old_log_output;
> +
>  --source include/rpl_end.inc
> diff --git a/mysql-test/t/log_tables.test b/mysql-test/t/log_tables.test
> index f822ec8d758..0196e62f12f 100644
> --- a/mysql-test/t/log_tables.test
> +++ b/mysql-test/t/log_tables.test