Kristian Nielsen via commits <commits@lists.mariadb.org> writes:

> From: Brandon Nesterenko <brandon.nesterenko@mariadb.com>
>
> This patch extends the command `SHOW REPLICA HOSTS` with three
> columns:

Hi Brandon, please find my review of MDEV-21322 below.

>     This patch extends the command `SHOW REPLICA HOSTS` with three
>     columns:
>     
>      1) Gtid_State_Sent. This represents that latest GTIDs sent to the
>     replica in each domain. It will always be populated, regardless of
>     the semi-sync status (i.e. asynchronous connections will still
>     update this column with the latest GTID state sent to the replica).
>     
>      2) Gtid_State_Ack. For semi-synchronous connections (only), this
>     column represents the last GTID in each domain that the replica has
>     acknowledged.

Use "Pos" instead of "State", eg. Gtid_Pos_Sent and Gtid_Pos_Ack.
To be consistent with eg. gtid_binlog_pos, which has last GTID per
domain_id; vs. gtid_binlog_state, which has GTID per every (domain_id,
server_id) combination.

> @@ -2272,6 +2292,30 @@ send_event_to_slave(binlog_send_info *info, Log_event_type event_type,
>      return "Failed to run hook 'after_send_event'";
>    }
> 
> +  if (info->thd->slave_info)
> +  {
> +    strncpy(info->thd->slave_info->gtid_state_sent.log_file,
> +            info->log_file_name + info->dirlen,
> +            strlen(info->log_file_name) - info->dirlen);
> +    info->thd->slave_info->gtid_state_sent.log_pos= pos;
> +  }

Isn't there missing synchronisation here against concurrent access from
another thread?
I guess we need LOCK_thd_data around the strncpy().
Maybe we can avoid taking LOCK_thd_data except in the uncommon case where
the gile name changes. It would be best to avoid a mutex lock/unlock for
_every_ event sent to a slave.
The position doesn't need locking I think, you can use an atomic
std::memory_order_relaxed mostly for documentation if you like.

> @@ -125,8 +114,11 @@ int THD::register_slave(uchar *packet, size_t packet_length)
>    if (check_access(this, PRIV_COM_REGISTER_SLAVE, any_db.str, NULL,NULL,0,0))
>      return 1;
>    if (!(si= (Slave_info*)my_malloc(key_memory_SLAVE_INFO, sizeof(Slave_info),
> -                                   MYF(MY_WME))))
> +                                   MYF(MY_WME|MY_ZEROFILL))))
>      return 1;
> +  memset(si->gtid_state_sent.log_file, '\0', FN_REFLEN);
> +  memset(si->gtid_state_ack.log_file, '\0', FN_REFLEN);

I think it's good to add the MY_ZEROFILL here, but then the two memset() are
redundant and can be removed.

> @@ -1235,6 +1251,14 @@ int Repl_semi_sync_master::update_sync_header(THD* thd, unsigned char *packet,
>    *need_sync= sync;
> 
>   l_end:
> +  if (is_on())
> +  {
> +    thd->slave_info->sync_status=
> +        sync ? thd->slave_info->sync_status=
> +                   Slave_info::SYNC_STATE_SEMI_SYNC_ACTIVE
> +             : thd->slave_info->sync_status=
> +                   Slave_info::SYNC_STATE_SEMI_SYNC_STALE;
> +  }

There's something wierd here, double assignment of
thd->slave_info->sync_status, probably you just meant:

  thd->slave_info->sync_status= sync ?
    Slave_info::SYNC_STATE_SEMI_SYNC_ACTIVE : Slave_info::SYNC_STATE_SEMI_SYNC_STALE;

More importantly, update_sync_header() is called just _before_ writing the
event on the socket to the connected slave. Is that really the right place
to do so? Shouldn't the status be updated only _after_ the event has been
sent?

Or does it not matter, because the actual TCP sending is asynchroneous
anyway? In that case, should add a comment explaining why updating the
status here is ok.

> --- a/mysql-test/suite/rpl/r/rpl_mixed_ddl_dml.result
> +++ b/mysql-test/suite/rpl/r/rpl_mixed_ddl_dml.result
> @@ -11,8 +11,8 @@ n
>  2002
>  connection master;
>  show slave hosts;
> -Server_id    Host    Port    Master_id
> -2    127.0.0.1       9999    1
> +Server_id    Host    Port    Master_id       Gtid_State_Sent Gtid_State_Ack  Sync_Status
> +2    127.0.0.1       9999    1       0-1-2           Asynchronous

Just a remark that this output could be non-deterministic unless the test
guarantees that the state will be stable at this point. So please check that
the test will produce stable output; or if you already did so, then great!

> +--echo # state, and Gtid_State_Sent/Ack should start, and only Gtid_State_Sent

Typo, s/should start/should start empty/ ?

> +--connection server_2
> +--source include/stop_slave.inc
> +set global rpl_semi_sync_slave_enabled = 1;
> +--source include/start_slave.inc
> +
> +--connection server_1
> +let $status_var= Rpl_semi_sync_master_clients;
> +let $status_var_value= 1;
> +source include/wait_for_status_var.inc;
> +
> +--replace_result $SLAVE_MYPORT SLAVE_PORT $SERVER_MYPORT_3 SLAVE2_PORT $DEFAULT_MASTER_PORT DEFAULT_PORT
> +SHOW REPLICA HOSTS;

Hm, won't this output be non-deterministic?
From a quick look in the code, Rpl_semi_sync_master_clients is incremented
from the start of mysql_binlog_send(), but the state is only set from
update_sync_header() when the first event is sent to the slave. So won't
there be a (small) window where the state has not yet switched to ACTIVE
when this SHOW SLAVE HOSTS runs?
So better wait for the state to become ACTIVE instead. Or for the expected
value in Gtid_Pos_Sent, where that is non-empty.

Also, better use SHOW SLAVE HOSTS here, for consistency with rest of test
suite.

> +--let $nextval= `SELECT max(a)+1 from t1`
> +--eval insert into t1 values ($nextval)

Is there a reason here to use separate --let and --eval? And not just:

  INSERT INTO t1 SELECT max(a)+1 FROM t1;

> +--echo # Ensuring master gtid_binlog_pos matches Gtid_State_Ack
> +--let $gtid_ack= query_get_value(show slave hosts, Gtid_State_Ack, 1)
> +if (`SELECT strcmp("$master_gtid","$gtid_ack") != 0`)
> +{
> +  --echo # Master gtid_binlog_pos: $master_gtid
> +  --echo # Gtid_State_Ack: $gtid_ack
> +  --die Master's gtid_binlog_pos should match Gtid_State_Ack, but doesn't

Again, sync_with_master_gtid.inc on the slave will not guarantee that the
master saw the ack yet, even though it will be a rare race when it doesn't
happen.
So you'd need to wait for the expected value here with wait_condition.inc
or somilar instead, I think.
Probably the same in subsequent parts of the test case.

> +--echo #
> +--echo # 21322.5: When connecting a new slave (server_id 3) which initially has
> +--echo # semi-sync disabled, SHOW SLAVE HOSTS on the master should show its
> +--echo # Sync_Status as asynchronous (while server_id 2 is still semi-sync
> +--echo # active).

Sorry, it's a very long and complex test case, I didn't have the time today
to carefully read and check every detail in it after this point. Please
carefully check all the cases for possible races like described above.
Replication tests are notorious for having non-deterministic failures (as
I'm sure you've experienced first-hand), and it's important to try to avoid
that as much as possible.

A general remark on the test, the output of Gtid_Pos_Sent and Gtid_Pos_Ack
are full GTID positions (multiple domain_id). But there is no test of having
more than one domain in the output. I think you need to extend the test to
test at least one case where there are multiple domains. And also need to
test when the GTID state in the binlog has multiple server_id (though maybe
the test case already does this, I didn't check myself). 

 - Kristian.