Hi, Aleksey,

On Oct 23, Aleksey Midenkov wrote:
> revision-id: 3aea3bd7422 (mariadb-10.4.30-176-g3aea3bd7422)
> parent(s): 4fa6383c937
> author: Aleksey Midenkov
> committer: Aleksey Midenkov
> timestamp: 2023-09-26 20:20:46 +0300
> message:
>
> MDEV-23294 Segfault or assertion upon MyISAM repair
>
> When computing vcol expression some items use current_thd and that was
> not set in MyISAM repair thread. Since all the repair threads belong
> to one connection and items should not write into THD we can utilize
> table THD for that.
>
> 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?

I think cleaner code is more important in this case. Putting this to thr_find_all_keys() makes some mess: THD is not the part of MyISAM, thr_find_all_keys() doesn't know anything about virtual keys and should not care. Keeping the logic of SQL layer in one place of storage layer (or as much in one place as possible) would be a wise idea. Of course if we didn't decide that this impacts performance.

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

  set_current_thd((TABLE*)(info->external_ref)->in_use);

>    table->move_fields(table->field, record, table->field[0]->record_ptr());
>    if (keynum == -1) // update all vcols
>    {

Regards,
Sergei
Chief Architect, MariaDB Server
and security@mariadb.org