maria-developers team mailing list archive

[Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>]

Sachin Setiya <sachin.setiya@xxxxxxxxxxx> writes:

>> Why did you decide to put this information into a status variable?
>> Normally,
>> slave status is seen in SHOW SLAVE STATUS, which supports showing status
>> for
>> a particular connection without using @@default_master_connection.
>>
>> Sorry for this , I guess It was a confusion I was looking at Slave_Running
> this is avaliable in show status as well as
> show slave status. So I thought for if I want to show some information in
> SHOW slave status this has to be in show status.
> And this is wrong. Now we can see non trans events / ddl events in show
> slave status. Show I remove this vars from Show Status, or it is okay ?

But what do you think? What would you prefer, as a user? How about asking on
the mailing list to get the input of actual users of the database, how they
would use the feature and what they would prefer?

I really worry that MariaDB development is deteriorated to the point that
little thought goes into proper design anymore. It's all about "when can you
push" whatever was assigned in Jira to your sprint. And little concern about
_what_ is pushed.

I do not think the information should be duplicated in SHOW STATUS and SHOW
SLAVE STATUS, so yes, one should be removed. SHOW SLAVE STATUS seems most
appropriate, since it is naturally able to show per-master-connection
information. Looking at the documentation for SHOW STATUS:

  https://mariadb.com/kb/en/mariadb/show-status/

  "With the GLOBAL modifier, SHOW STATUS displays the status values for all
  connections to MariaDB. With SESSION, it displays the status values for
  the current connection."

Your patch makes SHOW STATUS display status for whatever is in
@@default_master_connection (if I understand it correctly?). That seems
inconsistent with how SHOW STATUS normally works.

> Attached a newer version of patch , please review it.

> 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..b4f60e3
> --- /dev/null
> +++ b/mysql-test/suite/multi_source/multi_parallel.test

> +--save_master_pos
> +
> +--connection slave
> +--sync_with_master 0,'master2'
> +show slave 'master2' status like 'Slave_DDL_Events';
> +show slave 'master2' status like 'Slave_Non_Transactional_Events';

I do not understand. This does not appear to be valid syntax. Your .result
file does not have this, instead it has:

> +set default_master_connection = 'master2';
> +show status like 'Slave_DDL_Events';
> +Variable_name	Value
> +Slave_ddl_events	18
> +show status like 'Slave_Non_Transactional_Events';
> +Variable_name	Value
> +Slave_non_transactional_events	36

Do I misunderstand, or did you not even run your test case before sending it
to me for review? If you did not, that simply is an unacceptable waste of
the reviewer's time.

> diff --git a/sql/slave.cc b/sql/slave.cc
> index 1846938..0feace4 100644
> --- a/sql/slave.cc
> +++ b/sql/slave.cc
> @@ -2823,6 +2823,15 @@ void show_master_info_get_fields(THD *thd, List<Item> *field_list,
>                                              gtid_pos_length),
>                            mem_root);
>    }
> +  field_list->push_back(new (mem_root)
> +                       Item_return_int(thd, "Slave_DDL_Events", 20,
> +                                       MYSQL_TYPE_LONGLONG),
> +                       mem_root);
> +
> +  field_list->push_back(new (mem_root)
> +                       Item_return_int(thd, "Slave_Non_Transactional_Events", 20,
> +                                       MYSQL_TYPE_LONGLONG),
> +                       mem_root);
>    DBUG_VOID_RETURN;
>  }

If I read this correctly, you seem to have put the new SHOW SLAVE STATUS
fields _after_ the extra fields output with the ALL SLAVES option. Doesn't
that seem a bit messy? Other new options were added _before_ those of the
ALL SLAVES option. See also this mailing list discussion:

  https://lists.launchpad.net/maria-developers/msg09993.html

Also, this seems to be based on MariaDB-10.1 code (since 10.2 has
Slave_SQL_Running_State at this point)? A new feature like this shouldn't go
into 10.1.

 - Kristian.