Jonas Oreland <jonaso@google.com> writes:

> I light of recent "I did see any of your comments until by accident just
> now",
> I now mail you directly that I updated both MDEV-162 and MDEV-7257.

Sorry for the late answer, I do not follow _all_ Jira activity, so I did not
see your comments on those issues (I do read all of maria-developers@ though).

I think the patch looks good. That binlog send code has long needed a cleanup,
lots of duplicated code in there and other ugly stuff.

A few comments:

> diff --git a/mysql-test/suite/rpl/r/rpl_checksum.result b/mysql-test/suite/rpl/r/rpl_checksum.result
> index d88258f..b736dbc 100644
> --- a/mysql-test/suite/rpl/r/rpl_checksum.result
> +++ b/mysql-test/suite/rpl/r/rpl_checksum.result

> -Last_IO_Error = 'Got fatal error 1236 from master when reading data from binary log: 'Slave can not handle replication events with the checksum that master is configured to log; the first event 'master-bin.000009' at 367, the last event read from 'master-bin.000010' at 248, the last byte read from 'master-bin.000010' at 248.''
> +Last_IO_Error = 'Got fatal error 1236 from master when reading data from binary log: 'Slave can not handle replication events with the checksum that master is configured to log; the first event 'master-bin.000009' at 367, the last event read from 'master-bin.000010' at 367, the last byte read from 'master-bin.000010' at 411.''

Why does the binlog position change here? (It's not necessarily a problem, and
usually it's a bad idea anyway for test cases to depend on binlog
positions. But it was hard to follow the details of the patch due to so much
code moving around, so I want to understand why...)

> diff --git a/sql/log.cc b/sql/log.cc
> index 2c20ea3..d10c1ad 100644
> --- a/sql/log.cc
> +++ b/sql/log.cc

> +    /**
> +     * TODO(jonaso): Check with Kristian, should last_commit_pos really
> +     * be updated for relay logs!!!
> +     */
> +    if (!is_relay_log)
> +    {
> +      /* update binlog_end_pos so that it can be read by after sync hook */
> +      reset_binlog_end_pos(log_file_name, offset);
> +    }
> +
>      mysql_mutex_lock(&LOCK_commit_ordered);
>      strmake_buf(last_commit_pos_file, log_file_name);
> -    last_commit_pos_offset= my_b_tell(&log_file);
> +    last_commit_pos_offset= offset;
>      mysql_mutex_unlock(&LOCK_commit_ordered);

I agree, it is not necessary to update last_commit_pos for relay log. So it
could be moved into if (!is_relay_log) { ... }.

> @@ -4797,6 +4815,10 @@ void MYSQL_BIN_LOG::make_log_name(char* buf, const char* log_ident)
>
>  bool MYSQL_BIN_LOG::is_active(const char *log_file_name_arg)
>  {
> +  /**
> +   * there should/must be a mysql_mutex_assert_owner(&LOCK_log) here...
> +   * but code violates this all over! (scary monsters and super creeps!)
> +   */
>    return !strcmp(log_file_name, log_file_name_arg);
>  }

Hm. I guess this isn't a new problem? Though with the reduced locking of
LOCK_log, it might have a higher risk of triggering?

But if I understand the patch correctly, then it actually fixes this with
respect to the binlog. Because now it uses the new binlog_end_pos, it seems
there are no more calls to is_active() on the master.

And it looks like on the slave side, it does hold LOCK_log while doing
is_active().

So is this comment still appropriate? And if so, maybe it could be clarified
instead to specify exactly where the code is that violates

> @@ -7387,6 +7434,10 @@ MYSQL_BIN_LOG::trx_group_commit_leader(group_commit_entry *leader)
>
>    DEBUG_SYNC(leader->thd, "commit_before_get_LOCK_commit_ordered");
>    mysql_mutex_lock(&LOCK_commit_ordered);
> +    /**
> +     * TODO(jonaso): Check with Kristian,
> +     * if we rotate:d above, this offset is "wrong"
> +     */
>    last_commit_pos_offset= commit_offset;

Agree, good catch, I think.
If we rotate, we already update last_commit_pos_file and _offset. So the code
needs to remember if it rotated, and skip the errorneous offset update in this
case.

Do you want to fix this, as part of the patch? Or should I take it?

> @@ -811,6 +817,67 @@ class MYSQL_BIN_LOG: public TC_LOG, private MYSQL_LOG

> +    mysql_mutex_assert_not_owner(&LOCK_binlog_end_pos);
> +    lock_binlog_end_pos();

You shouldn't need the assert_not_owner() I think, safe_mutex will already
complain if a thread tries to multiple lock a mutex.

> +    The difference between this and last_commit_pos_{file,offset} is that
> +    the commit position is updated later. If semi-sync wait point is set
> +    to WAIT_AFTER_SYNC, the commit pos is update after semi-sync-ack has
> +    been received and the end point is updated after the write as it's needed
> +    for the dump threads to be able to semi-sync the event.
> +  */
> +  my_off_t binlog_end_pos;
> +  char binlog_end_pos_file[FN_REFLEN];

Right... last_commit_pos is about commits in the storage engine. While
binlog_end_pos is about writes to the binlog. Agree.

> +  strncpy(info->start_log_file_name, log_ident,
> +          sizeof(info->start_log_file_name));
> +  info->start_log_file_name[sizeof(info->start_log_file_name)-1]= 0;

I think the better way to do this in MySQL/MariaDB sources is

  strmake(info->start_log_file_name, log_ident,
          sizeof(info->start_log_file_name)-1);

------

So I think the patch looks good. And getting rid of LOCK_log for every event
sent from master to a slave, that can't be bad. So let's include it.

So how do we do it? You mentioned wanting to avoid large intrusive diff in
your tree relative to MariaDB upstream, which makes sense. But I suppose this
is a large change to put in stable 10.0... Is it sufficient to include in
10.1/10.2, so that any new changes (which will happen only on top of this
patch) will not conflict?

(I think I also will want it in my 10.0-based replication tree, which will not
be pushed to 10.0 main, but now includes the new parallel replication
stuff...).

 - Kristian.