Code review comment for lp:~akopytov/percona-server/backup_locks_bugs

Revision history for this message
Laurynas Biveinis (laurynas-biveinis) wrote :

    - Should Relay_log_info::purge_relay_logs set errmsg on failure to
      acquire binlog lock protection?

    - Why does Slave_worker::reset_recovery_info try to acquire the
      binlog protection and proceeds to call the setter without
      considering whether the acquisition succeeded? That will fail
      the DBUG_ASSERT in the setter in the case of failure at
      least. If I understand the context of the code correctly, it
      should be just like the other protection sites instead, with an
      early error return in the case of failure.

    - Have you considered extracting

bool rpl_acquire_backup_binlog_lock_protection(thd, rli)
{
  bool ret= true;
  if (thd && !thd->backup_binlog_lock.is_acquired())
  {
    const ulong timeout= info_thd->variables.lock_wait_timeout;

    DBUG_PRINT("debug", ("Acquiring binlog protection lock"));

    if (rli)
      mysql_mutex_assert_not_owner(&rli->data_lock);

   ret= !info_thd->backup_binlog_lock.acquire_protection(thd,
MDL_EXPLICIT, timeout);
  }
  return ret;
}

and void release(thd, bool protection_acquired)
{
  if (thd && prot_acquired)
  {
    DBUG_PRINT("debug", ("Releasing binlog protection lock"));
    thd->backup_binlog_lock.release_protection(thd);
  }
}

review: Needs Information

« Back to merge proposal