Re: [Maria-developers] Mdev-10664 Add statuses about optimistic parallel replication stalls.

[andrei . elkin]

Sachin, howdy.

While the latest patch is pretty I still see potential to improve it,
please find A[0-9]+ prefixed annotations below.
Thank you for addressing earlier comments!

> Hi Andrei, Kristian !
>
> Sorry for late reply (I was on vacation , and there was
> very less 10.3 sprint)
>
> On Sat, Feb 10, 2018 at 5:42 PM, 
> wrote:
>
> Sachin, hello.
>
> I've read through your latest patch as well as did so
> to the current
> mail thread.
> On previous rounds there were few notes raised by
> Kristian. I see that
> they got addressed.
>
> I got few requests of myself as we spoke yesterday
>
>  1. Let's count the transactional groups as well for
> consistency so that
>     the total count of all *taken* to execution groups
> would be gained
>     as simple as summing of the three kinds.
>
> Done. 
>
>  2. Suggest to name them all with '_groups' suffix:
>
>     DDL_groups, Non_transactiona_groups,
> Transactional_groups
>
> Done, But I have added Slave as prefix. 
>
>     'Event' is still defined as a sort of a statement,
> or the minimum
>      indivisible piece of work by a slave applier. 
> But the patch counts
>      Gtids that head any of the tree group of events,
> so it should be
>      the '_groups' suffix imo.  Sure, check with Ian
> (cc:d).
>      I see Kristian was somewhat sceptical about using
> 'group' as an offical
>      term, but that's the most natural way to name the
> object at hand.
>      For instace the transactional group is the same
> as just the
>      transaction (provided that `mysql.gtid_slave_pos`
> table is of the
>      same transactional type).
>
>   3. Make sure we document the counters are optimistic
> in sense they are
>      incremented in the beginning of a group execution
> before knowing
>      its outcome.
>
> I am counting events in Gtid_log_event::do_apply_event ,
> So counter is optimistic. 
>
>   4. Always head new [test] files with comments
> explaining top-level of the test content.
>      Specifcially to mention what is tested, and what
> are results of that.
>
> Done. 
>
> While the patch is certainly good,
> I would like to have another look at the final patch
> to approve
> formally.
>
> Cheers,
>
> Andrei
>
> Patch link 
> http://lists.askmonty.org/pipermail/commits/2018-March/012136.html

revision-id: 6b2b2a56f91b93ab2ac5e30fc32465631e0d0c47 
(mariadb-10.3.5-25-g6b2b2a5)
parent(s): ad647cc84ebf331d59b24e81bffe89be2f5b1ed7
author: Sachin Setiya
committer: Sachin Setiya
timestamp: 2018-03-26 12:23:18 +0530
message:

Mdev-10664 Add statuses about optimistic parallel replication stalls

In this commit we are adding three more status variable to SHOW SLAVE
STATUS.  Slave_DDL_Events and Slave_Non_Transactional_Events.

Slave_DDL_Groups:- This status variable counts the occurrence of DDL
statements

Slave_Non_Transactional_Groups:- This variable count the occurrence
of non-transnational event group.

Slave_Transactional_Groups:- This variable count the occurrence
of transnational event group.

Patch Credit:- Kristian Nielsen

---
 mysql-test/include/check-testcase.test |   5 +-
 mysql-test/suite/multi_source/info_logs.result |  12 +-
 .../suite/multi_source/multi_parallel.result   | 230 +
 mysql-test/suite/multi_source/multi_parallel.test  | 155 ++
 mysql-test/suite/multi_source/reset_slave.result   |   8 +-
 mysql-test/suite/multi_source/simple.result|  23 ++-
 mysql-test/suite/multi_source/syntax.result|   6 +-
 sql/log_event.cc   |  10 +
 sql/rpl_mi.cc  |   3 +-
 sql/rpl_mi.h   |  10 +
 sql/slave.cc   |  24 +++
 11 files changed, 461 insertions(+), 25 deletions(-)

diff --git a/mysql-test/include/check-testcase.test 
b/mysql-test/include/check-testcase.test
index 4ca5398..3c164ee 100644
--- a/mysql-test/include/check-testcase.test
+++ b/mysql-test/include/check-testcase.test
@@ -70,10 +70,13 @@ if ($tmp)
   --echo SQL_Delay 0
   --echo SQL_Remaining_Delay   NULL
   --echo Slave_SQL_Running_State   
+  --echo Slave_DDL_Groups  #
+  --echo Slave_Non_Transactional_Groups#
+  --echo Slave_Transactional_Groups#
 }
 if (!$tmp) {
   # Note: after WL#5177, fields 13-18 shall not be filtered-out.
-  --replace_column 4 # 5 # 6 # 7 # 8 # 9 # 10 # 13 # 14 # 15 # 16 # 17 # 18 # 
22 # 23 # 24 # 25 # 26 # 40 # 41 # 42 # 44 #
+  --replace_column 4 # 5 # 6 # 7 # 8 # 9 # 10 # 13 # 14 # 15 # 16 # 17 # 18 # 
22 # 23 # 24 # 25 # 26 # 40 # 41 # 42 # 44 # 51 # 52 # 53 #
   query_vertical
   SHOW SLAVE STATUS;
 }
diff --git a/mysql-test/suite/multi_source/info_logs.result 

Re: [Maria-developers] Mdev-10664 Add statuses about optimistic parallel replication stalls.

[Sachin Setiya]

Hi Andrei, Kristian !

Sorry for late reply (I was on vacation , and there was very less 10.3
sprint)


On Sat, Feb 10, 2018 at 5:42 PM,  wrote:

> Sachin, hello.
>
>
> I've read through your latest patch as well as did so to the current
> mail thread.
> On previous rounds there were few notes raised by Kristian. I see that
> they got addressed.
>
> I got few requests of myself as we spoke yesterday
>
>  1. Let's count the transactional groups as well for consistency so that
> the total count of all *taken* to execution groups would be gained
> as simple as summing of the three kinds.
>
Done.

>
>  2. Suggest to name them all with '_groups' suffix:
>
> DDL_groups, Non_transactiona_groups, Transactional_groups
>
Done, But I have added Slave as prefix.

>
> 'Event' is still defined as a sort of a statement, or the minimum
>  indivisible piece of work by a slave applier.  But the patch counts
>  Gtids that head any of the tree group of events, so it should be
>  the '_groups' suffix imo.  Sure, check with Ian (cc:d).
>  I see Kristian was somewhat sceptical about using 'group' as an
> offical
>  term, but that's the most natural way to name the object at hand.
>  For instace the transactional group is the same as just the
>  transaction (provided that `mysql.gtid_slave_pos` table is of the
>  same transactional type).
>
>   3. Make sure we document the counters are optimistic in sense they are
>  incremented in the beginning of a group execution before knowing
>  its outcome.
>
I am counting events in Gtid_log_event::do_apply_event , So counter is
optimistic.

>
>   4. Always head new [test] files with comments explaining top-level of
> the test content.
>  Specifcially to mention what is tested, and what are results of that.
>
Done.

>
> While the patch is certainly good,
> I would like to have another look at the final patch to approve
> formally.
>
> Cheers,
>
> Andrei
>
> Patch link
http://lists.askmonty.org/pipermail/commits/2018-March/012136.html
Regards
sachin

>
> > HI Kristian,
> >
> > On Thu, Jan 26, 2017 at 2:05 PM, Kristian Nielsen
> >  wrote:
> >> Sachin Setiya  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.
> > No , this is only me. Other people are doing great work.
> >>
> >> 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 000..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 

Re: [Maria-developers] Mdev-10664 Add statuses about optimistic parallel replication stalls.

[Kristian Nielsen]

andrei.el...@pp.inet.fi writes:

>  I see Kristian was somewhat sceptical about using 'group' as an offical
>  term, but that's the most natural way to name the object at hand.

Yeah... but I cannot think of a better term to use here, and "event group"
is the proper term for what is counted here, even if it is somewhat
internal.

It would surely be more confusing to have something like
"non_transactional_transactions" and "transactional_transactions ..."

 - Kristian.

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Mdev-10664 Add statuses about optimistic parallel replication stalls.

[andrei . elkin]

Sachin, hello.


I've read through your latest patch as well as did so to the current
mail thread.
On previous rounds there were few notes raised by Kristian. I see that
they got addressed.

I got few requests of myself as we spoke yesterday

 1. Let's count the transactional groups as well for consistency so that
the total count of all *taken* to execution groups would be gained
as simple as summing of the three kinds.

 2. Suggest to name them all with '_groups' suffix:

DDL_groups, Non_transactiona_groups, Transactional_groups

'Event' is still defined as a sort of a statement, or the minimum
 indivisible piece of work by a slave applier.  But the patch counts
 Gtids that head any of the tree group of events, so it should be
 the '_groups' suffix imo.  Sure, check with Ian (cc:d).
 I see Kristian was somewhat sceptical about using 'group' as an offical
 term, but that's the most natural way to name the object at hand.
 For instace the transactional group is the same as just the
 transaction (provided that `mysql.gtid_slave_pos` table is of the
 same transactional type).

  3. Make sure we document the counters are optimistic in sense they are
 incremented in the beginning of a group execution before knowing
 its outcome.

  4. Always head new [test] files with comments explaining top-level of the 
test content.
 Specifcially to mention what is tested, and what are results of that.

While the patch is certainly good,
I would like to have another look at the final patch to approve
formally.

Cheers,

Andrei


> HI Kristian,
>
> On Thu, Jan 26, 2017 at 2:05 PM, Kristian Nielsen
>  wrote:
>> Sachin Setiya  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.
> No , this is only me. Other people are doing great work.
>>
>> 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 000..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_nameValue
>>> +Slave_ddl_events 18
>>> +show status like 'Slave_Non_Transactional_Events';
>>> +Variable_nameValue
>>> +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.
>>
> Sorry for this. I run test cases in pc and sent patch from laptop. Pc
> was on older version ,
> I just forgot to sync them :(. 

Re: [Maria-developers] Mdev-10664 Add statuses about optimistic parallel replication stalls.

[Sachin Setiya]

HI Kristian,

On Thu, Jan 26, 2017 at 2:05 PM, Kristian Nielsen
 wrote:
> Sachin Setiya  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.
No , this is only me. Other people are doing great work.
>
> 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 000..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_nameValue
>> +Slave_ddl_events 18
>> +show status like 'Slave_Non_Transactional_Events';
>> +Variable_nameValue
>> +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.
>
Sorry for this. I run test cases in pc and sent patch from laptop. Pc
was on older version ,
I just forgot to sync them :(. Sorry for wasting your time, next time
I will be more careful.
>> 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 
>> *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
>
Changed. Yes you are right.
> 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.
>
Ported the patch to 10.2. Yes you are right I am doing coding in very
bad way , just
following the jira task :( .
>  - Kristian.
If you like to review , I have included a newer version of patch. This
time I have tested it on
pc. 

Re: [Maria-developers] Mdev-10664 Add statuses about optimistic parallel replication stalls.

[Kristian Nielsen]

Sachin Setiya  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 000..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_nameValue
> +Slave_ddl_events 18
> +show status like 'Slave_Non_Transactional_Events';
> +Variable_nameValue
> +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 
> *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.

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Mdev-10664 Add statuses about optimistic parallel replication stalls.

[Sachin Setiya]

Hi Kristian!

Sorry for late reply , I was on vacations.

On Tue, Jan 10, 2017 at 7:31 PM, Kristian Nielsen 
wrote:

> Sachin Setiya  writes:
>
> >> Right, but you still need _some_ way to synchronise multiple threads
>
> > I used atomic builtins my_atomic_add64_explicit.
>
> That seems fine (but see comment below).
>
> > Documentation:-
> >
> > Slave_DDL_Events:-
> >
> > Description: Number of DDL statements slave have executed in
> > replication channel X.
> > Scope: Per Replication Channel
> > Data Type: numeric
> > Introduced: MariaDB 10.1
> >
> > Slave_Non_Transactional_Events:-
> >
> > Description: Number of Non Transactional "no idea what should be the
> > next word " slave have executed in replication channel X.
> > Scope: Per Replication Channel
> > Data Type: numeric
> > Introduced: MariaDB 10.1
>
> I checked, the term "event group" is used already multiple places in the
> documentation, so I think it can be used here also. An event group can be a
> transaction (possibly with multiple transactional DML statements), or a DDL
> or non-transactional statement.
>
> What you are doing is introducing status variables to help understand
> optimistic (and aggressive) parallel replication modes.
>
> In optimistic parallel replication, a non-transactional event group is not
> safe to run in parallel with other non-transactional events. So that event
> is made to wait for all prior event groups to reach commit state, before
> being allowed to execute. However, other transactional events are not
> delayed by the non-transactional event group.
>
> Slave_Non_Transactional_Events counts the occurences of this.
>
> Then in case of a DDL statement, this can potentially be unsafe to run in
> parallel even with other transactional events. So for a DDL, the DDL has to
> wait for all prior event groups, _and_ subsequent event groups have to wait
> for the DDL.
>
> Slave_DDL_Events counts these occurences.
>
> This is what the documentation, and commit messages, should explain, I
> think.
>
> I think the status variables describe the values for
> @@default_master_connection?, this should also be explained in the
> documentation.
>
> 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 ?

> > diff --git a/sql/log_event.h b/sql/log_event.h
> > index 90900f6..579607f 100644
> > --- a/sql/log_event.h
> > +++ b/sql/log_event.h
> > @@ -50,6 +50,7 @@
> >  #endif
> >
> >  #include "rpl_gtid.h"
> > +#include "my_atomic.h"
>
> Why include my_atomic.h in the header file, when you only use it in the .cc
> file?
>
Changed.

>
> > +  mysql_mutex_lock(_active_mi);
> > +  if (master_info_index)
> > +  {
> > +mi= master_info_index->
> > +  get_master_info(>variables.default_master_connection,
> > +Sql_condition::WARN_LEVEL_NOTE);
> > +if (mi)
> > +  tmp= mi->total_ddl_events;
>
> > +if (mi)
> > +  tmp= mi->total_non_trans_events;
>
> Better use an atomic primitive here to read the value. Taking a mutex does
> not help synchronise against my_atomic_add64_explicit().
>
> Changed.

> > +  }
> > +  mysql_mutex_unlock(_active_mi);
> > +  if (mi)
> > +*((longlong *)buff)= tmp;
> > +  else
> > +var->type= SHOW_UNDEF;
> > +
> > +  return 0;
> > +}
> >  #endif /* HAVE_REPLICATION */
> >
> >  static int show_open_tables(THD *thd, SHOW_VAR *var, char *buff,
>
> > index 9365c06..a474aa5 100644
> > --- a/sql/rpl_mi.h
> > +++ b/sql/rpl_mi.h
> > @@ -303,6 +303,12 @@ class Master_info : public
> Slave_reporting_capability
> >
> >/* The parallel replication mode. */
> >enum_slave_parallel_mode parallel_mode;
> > +
> > +  /* No of DDL statements */
> > +  long total_ddl_events;
> > +
> > +  /* No of non-transactional statements*/
> > +  long total_non_trans_events;
>
> "long" is not the appropriate type here. Certainly not since you are using
> my_atomic_add64_explicit(). Use a type that is the same size on all
> platforms (and why use a signed type?).
>
> Changed to uint64.

>  - Kristian.
>


Attached a newer version of patch , please review it.
-- 
Regards
Sachin
diff --git a/mysql-test/include/check-testcase.test b/mysql-test/include/check-testcase.test
index 083f44c..9d8c0ec 100644
--- a/mysql-test/include/check-testcase.test
+++ b/mysql-test/include/check-testcase.test
@@ -67,10 +67,12 @@ if ($tmp)
   --echo Replicate_Do_Domain_Ids	
   --echo Replicate_Ignore_Domain_Ids	
   --echo 

Re: [Maria-developers] Mdev-10664 Add statuses about optimistic parallel replication stalls.

[Kristian Nielsen]

Sachin Setiya  writes:

> To my surprise slave_parallel_mode is replication channel specific ,
> while slave_parallel threadsis a global variable, Why So ?

There is a single pool of worker threads, to enable threads to be shared
among multi-source connections - for example so all threads can work on one
busy connection while another is idle. That is why slave_parallel_threads
must be global. In contrast, it is perfectly possible to run one
multi-source connection in conservative mode, and another in optimistic, for
example.

> And in mariadb documentation I was unable to find which variable is
> replication channel specific and which variable is global one.

Hm, if it's not documented that slave_parallel_mode is per-channel, that is
unfortunate (this is something that was introduced for 10.1).

> Yep, because if there is DDL statement or non transnational statement
> it is executed alone.
> So no need of mutex.

Right, but you still need _some_ way to synchronise multiple threads
incrementing the counter simultaneously (it is perfectly possible for two
non-transactional event groups to execute concurrently in conservative mode,
for example).

>> The name "statements" is very confusing here, since you are counting event
>> groups, not individual statements.
>>
> Changed It to event total_ddl_events , total_non_trans_events

It's not individual events. It is "event groups", though this is mostly a
terminology internally in the source code.

The term "transaction" is unfortunate for something that is
non-transactional. The name "event group" is also bad, as it is unfamiliar
to users, I think.

Maybe "total_ddl_events" is ok. I think in most cases only one statement at
a time can be in an event group, when it is non-transactional or DDL. Though
multiple binlog events will be involved for the single statement.

>From this, _statement might also be ok, though it is somewhat unfortunate in
case of row-based replication, where no SQL statements are involved at all
on the slave.

So baring better ideas, I guess *_events is ok, just be sure to make it
clear in the documentation exactly what is counted.

 - Kristian.

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Mdev-10664 Add statuses about optimistic parallel replication stalls.

[Sachin Setiya]

Hi Kristian!

On Sun, Jan 8, 2017 at 4:09 PM, Kristian Nielsen
 wrote:
> Sachin Setiya  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
Thanks.
>
>> 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 000..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?
>
I was copying from other tests in multi_source, so I copied it without
thinking, sorry
Reverted.
>> +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.
>
Sorry for this,I forgot to remove it , I was experimenting with
slave_parallel_mode and slave_parallel threads , so I added this
To my surprise slave_parallel_mode is replication channel specific ,
while slave_parallel threadsis a global variable, Why So ?
And in mariadb documentation I was unable to find which variable is
replication channel specific and which variable is global one.
Reverted
>> +# Cleanup
>> +stop all slaves;
>> +--source include/wait_for_slave_to_stop.inc
>
> This will only wait for one slave to stop, won't it?
>
yup, changed.
>> 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(_active_mi);
>
> Ouch! You are now taking an expensive global lock for each and every
> transaction :-( Surely this will hurt parallel replication performance.
>
Actually I was thinking the same, But I do not know how to get the master info
So I used the same approch as in method show_slave_ddl_stmts.
I no longer use mutex.
> 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.
Yep, because if there is DDL statement or non transnational statement
it is executed alone.
So no need of mutex.
>
>> +  Master_info *mi= 
>> master_info_index->get_master_info(>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.
>
Changed It to event total_ddl_events , total_non_trans_events
> 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...
>
Never tried it , Will try it for next bug/task.

>> 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*)_retried_transactions, 
>> SHOW_LONG},
>>{"Slave_running",(char*) _slave_running, 
>> SHOW_SIMPLE_FUNC},
>>{"Slave_skipped_errors", (char*) _skipped_errors, 
>> SHOW_LONGLONG},
>> +  {"Slave_DDL_statements", (char*) _slave_ddl_stmts, 
>> SHOW_SIMPLE_FUNC},
>> +  {"Slave_Non_transactional_statements", (char *) 
>> _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.
>
Used Event.
>  - Kristian.



-- 
Regards
sachin
commit d079dbb04cab34e63d4cef140735e543492178db
Author: Sachin Setiya 
Date:   Sun Jan 8 19:35:20 2017 +0530

Mdev-10664 Add statuses about 

Re: [Maria-developers] Mdev-10664 Add statuses about optimistic parallel replication stalls.

[Kristian Nielsen]

Sachin Setiya  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 000..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(_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(>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*)_retried_transactions, 
> SHOW_LONG},
>{"Slave_running",(char*) _slave_running, 
> SHOW_SIMPLE_FUNC},
>{"Slave_skipped_errors", (char*) _skipped_errors, SHOW_LONGLONG},
> +  {"Slave_DDL_statements", (char*) _slave_ddl_stmts, 
> SHOW_SIMPLE_FUNC},
> +  {"Slave_Non_transactional_statements", (char *) 
> _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.

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

[Maria-developers] Mdev-10664 Add statuses about optimistic parallel replication stalls.

[Sachin Setiya]

Hi Kristian,

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)

I am attaching patch file, only above problem need to solved in patch.

-- 
Regards
Sachin Setiya
Software Engineer at  MariaDB
diff --git a/mysql-test/suite/multi_source/multi_parallel.result b/mysql-test/suite/multi_source/multi_parallel.result
new file mode 100644
index 000..6aa0658
--- /dev/null
+++ b/mysql-test/suite/multi_source/multi_parallel.result
@@ -0,0 +1,184 @@
+set global slave_parallel_threads=10;
+change master 'master1' to 
+master_port=MYPORT_1, 
+master_host='127.0.0.1', 
+master_user='root',
+master_heartbeat_period = 25;
+change master 'master2' to
+master_port=MYPORT_2,
+master_host='127.0.0.1',
+master_user='root',
+master_heartbeat_period=35;
+start all slaves;
+Warnings:
+Note	1937	SLAVE 'master2' started
+Note	1937	SLAVE 'master1' started
+set default_master_connection = 'master1';
+include/wait_for_slave_to_start.inc
+set default_master_connection = 'master2';
+include/wait_for_slave_to_start.inc
+show variables like 'slave%';
+Variable_name	Value
+slave_compressed_protocol	OFF
+slave_ddl_exec_mode	IDEMPOTENT
+slave_domain_parallel_threads	0
+slave_exec_mode	STRICT
+slave_load_tmpdir	/home/sachin/mdev-10664/build/mysql-test/var/tmp/mysqld.3
+slave_max_allowed_packet	1073741824
+slave_net_timeout	120
+slave_parallel_max_queued	131072
+slave_parallel_mode	conservative
+slave_parallel_threads	10
+slave_run_triggers_for_rbr	NO
+slave_skip_errors	OFF
+slave_sql_verify_checksum	ON
+slave_transaction_retries	10
+slave_type_conversions	
+## Slave status variable
+set default_master_connection = 'master1';
+show status like 'slave%';
+Variable_name	Value
+Slave_connections	0
+Slave_ddl_statements	0
+Slave_heartbeat_period	25.000
+Slave_non_transactional_statements	0
+Slave_open_temp_tables	0
+Slave_received_heartbeats	0
+Slave_retried_transactions	0
+Slave_running	ON
+Slave_skipped_errors	0
+Slaves_connected	0
+Slaves_running	2
+set default_master_connection = 'master2';
+show status like 'slave%';
+Variable_name	Value
+Slave_connections	0
+Slave_ddl_statements	0
+Slave_heartbeat_period	35.000
+Slave_non_transactional_statements	0
+Slave_open_temp_tables	0
+Slave_received_heartbeats	0
+Slave_retried_transactions	0
+Slave_running	ON
+Slave_skipped_errors	0
+Slaves_connected	0
+Slaves_running	2
+#master 1
+create table t1(a int primary key);
+insert into t1 values(1);
+insert into t1 values(2);
+drop table t1;
+create table t1(a int primary key);
+insert into t1 values(1);
+insert into t1 values(2);
+drop table t1;
+create table t1(a int primary key);
+insert into t1 values(1);
+insert into t1 values(2);
+drop table t1;
+create table t1(a int primary key);
+insert into t1 values(1);
+insert into t1 values(2);
+drop table t1;
+create table t1(a int primary key);
+insert into t1 values(1);
+insert into t1 values(2);
+drop table t1;
+create table t1(a int primary key);
+insert into t1 values(1);
+insert into t1 values(2);
+drop table t1;
+create table t1(a int primary key);
+insert into t1 values(1);
+insert into t1 values(2);
+drop table t1;
+create table t1(a int primary key);
+insert into t1 values(1);
+insert into t1 values(2);
+drop table t1;
+create table t1(a int primary key);
+insert into t1 values(1);
+insert into t1 values(2);
+drop table t1;
+create table t1(a int primary key);
+insert into t1 values(1);
+insert into t1 values(2);
+drop table t1;
+set default_master_connection = 'master1';
+show status like 'Slave%';
+Variable_name	Value
+Slave_connections	0
+Slave_ddl_statements	0
+Slave_heartbeat_period	25.000
+Slave_non_transactional_statements	0
+Slave_open_temp_tables	0
+Slave_received_heartbeats	0
+Slave_retried_transactions	0
+Slave_running	ON
+Slave_skipped_errors	0
+Slaves_connected	0
+Slaves_running	2
+#master 2
+create table t2(a int primary key);
+insert into t2 values(1);
+insert into t2 values(2);
+drop table t2;
+create table t2(a int primary key);
+insert into t2 values(1);
+insert into t2 values(2);
+drop table t2;
+create table t2(a int primary key);
+insert into t2 values(1);
+insert into t2 values(2);
+drop table t2;
+create table t2(a int primary key);
+insert into t2 values(1);
+insert into t2 values(2);
+drop table t2;
+create table t2(a int primary key);
+insert into t2 values(1);
+insert into t2 values(2);
+drop table t2;
+create table t2(a int primary key);
+insert into t2 values(1);
+insert into t2 values(2);
+drop table t2;
+create table t2(a int primary key);
+insert into t2 values(1);
+insert into t2 values(2);
+drop table t2;
+create table t2(a int primary key);
+insert into t2 values(1);
+insert into t2 values(2);
+drop table t2;
+create table t2(a int primary key);
+insert into t2 values(1);
+insert into t2 values(2);
+drop table