maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Nikita,

On May 03, Nikita Malyavin wrote:
> revision-id: c5ce597f06a (mariadb-11.0.1-115-gc5ce597f06a)
> parent(s): da5a72f32d4
> author: Nikita Malyavin
> committer: Nikita Malyavin
> timestamp: 2023-05-01 01:15:41 +0300
> message:
> 
> MDEV-31043 ER_KEY_NOT_FOUND upon concurrent ALTER and transaction
> 
> Non-transactional engines changes are visible immediately the row operation
> succeeds, so we should apply the changes to the online buffer immediately
> after the statement ends.
> 
> diff --git a/sql/log.cc b/sql/log.cc
> index f30ce3bcbd1..1d98455c2b3 100644
> --- a/sql/log.cc
> +++ b/sql/log.cc
> @@ -7701,23 +7701,35 @@ static int binlog_online_alter_end_trans(THD *thd, bool all, bool commit)
>      auto *binlog= cache.sink_log;
>      DBUG_ASSERT(binlog);
>  
> -    bool do_commit= commit || !cache.hton->rollback ||
> -                    cache.hton->flags & HTON_NO_ROLLBACK; // Aria
> +    bool do_commit= (commit && is_ending_transaction)
> +                    || cache.hton->flags & HTON_NO_ROLLBACK // Aria
> +                    || !cache.hton->rollback;
> +
> +    error= binlog_flush_pending_rows_event(thd, false, true, binlog, &cache);

you've lost the useful comment about STMT_END.
and you flush events for rollback too that seems like a waste.

I personally would've kept the old structure of if()'s. Like

 -    bool do_commit= commit || !cache.hton->rollback ||
 +    bool non_trans=           !cache.hton->rollback ||

and

 - if (do_commit)
 + if (commit || non_trans)

with

  -      if (is_ending_transaction)
  +      if (is_ending_transaction || non_trans)

That would've called binlog flush only where it needs to be.

> +
>      if (do_commit)
>      {
> -      // do not set STMT_END for last event to leave table open in altering thd
> -      error= binlog_flush_pending_rows_event(thd, false, true, binlog, &cache);
> -      if (is_ending_transaction)
> +      /*
> +        If the cache wasn't reinited to write, then it remains empty after
> +        the last write.
> +      */
> +      if (cache.cache_log.type != READ_CACHE && !error)

this is a confusing new condition. are you trying to avoid locking a
mutex for an empty cache? If yes, you can check my_b_bytes_in_cache(),
that'd be more clear.

>        {
>          mysql_mutex_lock(binlog->get_log_lock());
>          error= binlog->write_cache(thd, &cache.cache_log);
>          mysql_mutex_unlock(binlog->get_log_lock());
>        }
> -      else
> -        cache.store_prev_position();
>      }
> -    else if (!is_ending_transaction)
> +    else if (!commit) // rollback
> +    {
>        cache.restore_prev_position();
> +    }
> +    else

add // trans engine, end of statement

> +    {
> +      DBUG_ASSERT(!is_ending_transaction);
> +      cache.store_prev_position();
> +    }

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx