Brandon Nesterenko <brandon.nesterenko@mariadb.com> writes:

> were some additional
> synchronization changes that went alongside the use of LOCK_thd_data, so it
> would be good if
> you could go over at least the code changes again.

> diff --git a/sql/sql_repl.cc b/sql/sql_repl.cc
> index 18a52dc07ad..47f789de751 100644
> --- a/sql/sql_repl.cc
> +++ b/sql/sql_repl.cc
> @@ -2294,10 +2294,19 @@ send_event_to_slave(binlog_send_info *info, Log_event_type event_type,
> 
>    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;
> +    char *new_log_flie= info->log_file_name + info->dirlen;
> +    size_t log_file_size= strlen(info->log_file_name) - info->dirlen;

Hm, spelling mistake s/new_log_flie/new_log_file/.
And you can simpler do just:

  size_t log_file_size= strlen(new_log_file);

But more importantly, I almost didn't want to mention the need for locking
around this, it's just more overhead, that is going to be added for _every_
transaction in _every_ slave dump thread.

Is it really worth it to add this overhead? Isn't all of the info added to
SHOW SLAVE STATUS already available directly on the slave? Just query the
slave with SHOW SLAVE STATUS and all the information should already be
available, without introducing ever more overhead in the replication layer.

I browsed through the comments in
https://jira.mariadb.org/browse/MDEV-21322, but didn't see any justification
for this over just querying the slaves and avoiding the overhead. Also I
don't recall seeing any architecture review / discussion of this?

So is this MDEV even needed / justified to add to the server?

>> > +--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;
>>
>
> I like seeing the actual number value for cross referencing the values
> potentially
> in the table (e.g. if manually debugging and running a SELECT).

Ok, sure, then it's fine.

>> 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.
>>
>
> This one is guaranteed that the master saw the ACK implicitly, as the
> transaction
> has returned from commit (which only happens in the case of ACK received, or
> timeout (which would invalidate the test anyway)).

Ah, yes, I missed that. Ok, that probably apply in a number of places I
otherwise thought could be non-deterministic.

> Right.I tried pretty hard up-front to make the test as deterministic as
> possible, though
> I did fix one more spot based on your previous catch of
> rpl_semi_sync_master_clients
> not in sync with Sync_Status :)

One thing that I have found effective is to run eg:

  ./mysql-test-run.pl --mem rpl.rpl_show_slave_hosts{,,,} --parallel=12 --repeat=1000

and just let it run overnight or something, I've found that tends to trigger
most non-deterministic failures that will be seen in buildbot (though not
all). This for my 4-core/8-thread laptop.

 - Kristian.