maria-developers team mailing list archive

[Michael Widenius <michael.widenius@xxxxxxxxx>]

Hi!

On Fri, Sep 25, 2020 at 12:11 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
>
> 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?

Yes, that is true. As Aria doesn't have rollback, we have always to commit.
That was true even in the original code, in which the aria commit was
done very early in ha_commit_trans().

In 10.5 this will be handled by the Aria rollback code (which does a
commit with a warning and we don't need the commit call above),
but in 10.4 we don't have that option.

<cut>

> > -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())

I originally decided to leave rw_trans as a not used parameter, as I
think that commit_one_phase2()
could later benefit of that. However I agree it's better to remove it
for now and add it back again
when it will be used.

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

I don't really know how to check that there are no active transactions.
The reason I think this is safe is that this function is called once
before doing any handler calls or starting any transactions.
The reason for the comment was mostly to make it clear why I removed
the original ha_enable_transaction() code
Do we really need an assert for a function that is called once during
startup of a thread?

Regards,
Monty