Hi, Aleksey,
On Oct 24, Aleksey Midenkov wrote:
> > > diff --git a/storage/myisam/ha_myisam.cc b/storage/myisam/ha_myisam.cc
> > > index ddab8bcf741..27151548704 100644
> > > --- a/storage/myisam/ha_myisam.cc
> > > +++ b/storage/myisam/ha_myisam.cc
> > > @@ -714,6 +714,8 @@ static int compute_vcols(MI_INFO *info, uchar *record, int keynum)
> > > /* This mutex is needed for parallel repair */
> > > mysql_mutex_lock(&info->s->intern_lock);
> > > TABLE *table= (TABLE*)(info->external_ref);
> > > + if (!current_thd)
> > > + set_current_thd(table->in_use);
> >
> > why not to do it in thr_find_all_keys(), unconditionally?
> > you'd avoid an if() executed per row.
>
> It is done only for repair, isn't it, so what are we supposed to
> optimize by removing this if?
it's done for repair, for *every row* of the table, there can be many
millions of rows. It's the condition that will be true for the first row
and then it'll be always false. There is simply no reason to reevaluate
it for every row.
I understand this perfectly. There is just no good place for this set_current_thd(), so this was the choice between the two evils. I chosen code cleanliness instead of performance from the 3 facts:
1. This is only for repair operation that should happen less than once a week;
2. This is only for a table that contains virtual columns;
3. Specific weight of this condition is small against the execution of compute_vcols().
> > Also, I'd suggest a comment, like
> >
> > /*
> > To evaluate vcols we must have current_thd set.
> > This will set current_thd in all threads to the same THD, but it's
> > safe, because vcols are always evaluated under info->s->intern_lock.
> > */
>
> This is self-explanatory for the code in compute_vcols(). Another
> argument to keep that in compute_vcols().
If it would've been self-explanatory, I wouldn't have asked. Meaning it
wasn't self-explanatory to me.
Ok.
Regards,
Sergei
Chief Architect, MariaDB Server
and security@mariadb.org