maria-developers team mailing list archive

[Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>]

Sachin Setiya <sachin.setiya@xxxxxxxxxxx> writes:

> I am stuck at one problem in Mdev-10664. Suppose there is multisource
> replication('master1' and 'master2' )
> and we want to  update status var, How to know which master_info to
> update ?. Does slave threads have current
> replication channel name? (I was not able to find any , I have looked
> in rpl_group_info)

rgi->rli->mi

> diff --git a/mysql-test/suite/multi_source/multi_parallel.test b/mysql-test/suite/multi_source/multi_parallel.test
> new file mode 100644
> index 0000000..e27345d
> --- /dev/null
> +++ b/mysql-test/suite/multi_source/multi_parallel.test

> +--replace_result $SERVER_MYPORT_1 MYPORT_1
> +eval change master 'master1' to 
> +master_port=$SERVER_MYPORT_1, 
> +master_host='127.0.0.1', 
> +master_user='root',
> +master_heartbeat_period = 25;
> +
> +
> +
> +--replace_result $SERVER_MYPORT_2 MYPORT_2
> +eval change master 'master2' to
> +master_port=$SERVER_MYPORT_2,
> +master_host='127.0.0.1',
> +master_user='root',
> +master_heartbeat_period=35;

Why do you set heartbeat_period here?

> +show variables like 'slave%';

Why do you need to show all these variables? It is very likely to cause the
test to fail in some environments where the output may differ for whatever
reason.

> +# Cleanup
> +stop all slaves;
> +--source include/wait_for_slave_to_stop.inc

This will only wait for one slave to stop, won't it?

> diff --git a/sql/log_event.cc b/sql/log_event.cc
> index ced2626..c041116 100644
> --- a/sql/log_event.cc
> +++ b/sql/log_event.cc
> @@ -6780,6 +6780,17 @@ Gtid_log_event::do_apply_event(rpl_group_info *rgi)
>    }
>  
>    DBUG_ASSERT((bits & OPTION_GTID_BEGIN) == 0);
> +  mysql_mutex_lock(&LOCK_active_mi);

Ouch! You are now taking an expensive global lock for each and every
transaction :-( Surely this will hurt parallel replication performance.

This seems completely unnecessary. For example, if no flags are set you are
not doing anything here, so no lock needed at all. And even if a flag is
set, there is no need to take LOCK_active_mi, the Master_info cannot go away
in the middle of do_apply_event(). According to my understanding,
incremention a status variable should not require taking a global mutex at
all.

> +  Master_info *mi= master_info_index->get_master_info(&thd->variables.default_master_connection,
> +                   Sql_condition::WARN_LEVEL_NOTE);
> +  if (mi)
> +  {
> +    if (flags2 & FL_DDL)
> +      mi->total_ddl_statements++;
> +    if (!(flags & FL_TRANSACTIONAL))
> +      mi->total_non_trans_statements++;

The name "statements" is very confusing here, since you are counting event
groups, not individual statements.

Did you consider writing documentation for the feature? Writing the
documentation to explain to users exactly how the feature work could help
clarify such issues for yourself also, before coding...

> diff --git a/sql/mysqld.cc b/sql/mysqld.cc
> index adb1b27..5fcaf34 100644
> --- a/sql/mysqld.cc
> +++ b/sql/mysqld.cc

> @@ -8473,6 +8525,8 @@ SHOW_VAR status_vars[]= {
>    {"Slave_retried_transactions",(char*)&slave_retried_transactions, SHOW_LONG},
>    {"Slave_running",            (char*) &show_slave_running,     SHOW_SIMPLE_FUNC},
>    {"Slave_skipped_errors",     (char*) &slave_skipped_errors, SHOW_LONGLONG},
> +  {"Slave_DDL_statements",     (char*) &show_slave_ddl_stmts, SHOW_SIMPLE_FUNC},
> +  {"Slave_Non_transactional_statements", (char *) &show_slave_non_trans_stmts, SHOW_SIMPLE_FUNC},

Again, this is not statements, it is event groups. Or "transactions" if you
like, which is a term more familiar to users, though a "non-transactional
transaction" is also not perfect terminology.

 - Kristian.