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

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

No, because setting errmsg would cause callers to call
my_error(ER_RELAY_LOG_FAIL, errmsg), which is not what we want, because
that would have been already called by MDL_context::acquire_lock().

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

For some reasons I wasn’t sure if the callers are able to handle errors
returned by Slave_worker::reset_recovery_info(), but I can’t recall
them. Anyway, this seems to be a rather corner case, so I changed to
return an error instead.

> - 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);
> }
> }

I have, yes. That would prevent some code duplication, but not too much,
as callers would still have to handle errors, the ‘protection_acquired’
flag + some of them also assert that the current connection has not
acquired the binlog lock, while the others do not. Which means the
wrapper code around those acquire/release helper functions would be
about the same number of lines replicating to every site. So I decided
to leave it as is for clarity.

--
Best regards,
Alexey.