Sachin Setiya <sachin.setiya@mariadb.com> 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.

> 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?

> +  mysql_mutex_lock(&LOCK_active_mi);
> +  if (master_info_index)
> +  {
> +    mi= master_info_index->
> +      get_master_info(&thd->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().

> +  }
> +  mysql_mutex_unlock(&LOCK_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?).

 - Kristian.