maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Monty!

Just a couple of minor comments

Note, the below is from the combined diff of this commit and your
earlier MDEV-21953 commit, so you can see some lines here that are not
part of the fc07306120f.

> diff --git a/sql/handler.cc b/sql/handler.cc
> index cc7eedd18f2..402db310ed6 100644
> --- a/sql/handler.cc
> +++ b/sql/handler.cc
> @@ -133,6 +133,23 @@ static plugin_ref ha_default_tmp_plugin(THD *thd)
>    return ha_default_plugin(thd);
>  }
>  
> +#if defined(WITH_ARIA_STORAGE_ENGINE) && MYSQL_VERSION_ID < 100500
> +void ha_maria_implicit_commit(THD *thd, bool new_trn)
> +{
> +  if (ha_maria::has_active_transaction(thd))
> +  {
> +    int error;
> +    MDL_request mdl_request;
> +    mdl_request.init(MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT, MDL_EXPLICIT);
> +    error= thd->mdl_context.acquire_lock(&mdl_request,
> +                                         thd->variables.lock_wait_timeout);
> +    ha_maria::implicit_commit(thd, new_trn);
> +    if (!error)
> +      thd->mdl_context.release_lock(mdl_request.ticket);

This looks rather fishy. You commit in aria unconditionally?
whether you got the lock or not?

> +  }
> +}
> +#endif
> +
>  
>  /** @brief
>    Return the default storage engine handlerton for thread
> @@ -1753,13 +1777,14 @@ int ha_commit_one_phase(THD *thd, bool all)
>      if ((res= thd->wait_for_prior_commit()))
>        DBUG_RETURN(res);
>    }
> -  res= commit_one_phase_2(thd, all, trans, is_real_trans);
> +  res= commit_one_phase_2(thd, all, trans, is_real_trans, rw_trans);
>    DBUG_RETURN(res);
>  }
>  
>  
>  static int
> -commit_one_phase_2(THD *thd, bool all, THD_TRANS *trans, bool is_real_trans)
> +commit_one_phase_2(THD *thd, bool all, THD_TRANS *trans, bool is_real_trans,
> +                   bool rw_trans)

rw_trans seems to be unused here
(also in ha_commit_one_phase())

>  {
>    int error= 0;
>    uint count= 0;
> diff --git a/sql/sql_class.cc b/sql/sql_class.cc
> index 40e606425c5..71b447fb920 100644
> --- a/sql/sql_class.cc
> +++ b/sql/sql_class.cc
> @@ -1383,7 +1384,11 @@ void THD::update_all_stats()
>  void THD::init_for_queries()
>  {
>    set_time(); 
> -  ha_enable_transaction(this,TRUE);
> +  /*
> +    We don't need to call ha_enable_transaction() as we can't have
> +    any active transactions that has to be commited
> +  */
> +  transaction.on= TRUE;

I just commented on the similar code in another commit of yours.
don't just say "can't have any active transactions",
add an assert instead, please.

>  
>    reset_root_defaults(mem_root, variables.query_alloc_block_size,
>                        variables.query_prealloc_size);

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